More modular code

Robbo

Well-known member
I would like some extensions done to controllers for more modular code. In almost all situations when saving data (usually actionSave()) you have to replace the whole function when extending the initial controller and this just feels oh so wrong! Then obviously you have the issues of add-on compatibility. What I suggest is methods to be called from within the save actions. Usually you will have something like $input = $this->_input->filter(...) which is then added to the datawriter with $dataWriterObject->bulkSet($input). Now in a lot of cases all you want your add-on to do is save something else and I have had to do this many times either for clients modifications or released add-ons. Every time I have had to copy and paste the whole method. This could be something as simple as an empty method added to XenForo_Controller... array getInputForSaveAction(string $action, array &$input) . Then have this called after you do your filter specified above. So you would get something like (taken from category admin controller in actionSave)...
Code:
$writerData = $this->_input->filter(array(
	'title' => XenForo_Input::STRING,
	'node_type_id' => XenForo_Input::BINARY,
	'parent_node_id' => XenForo_Input::UINT,
	'display_order' => XenForo_Input::UINT,
	'display_in_list' => XenForo_Input::UINT,
	'description' => XenForo_Input::STRING,
	'style_id' => XenForo_Input::UINT,
));
$this->getInputForSaveAction('save', $input);
Then do the normal data writer stuff which would work as the data writer would be extended like normal.

Now extending all you would have to do is...
Code:
public function getInputForSaveAction($action, array &input)
{
	parent::getInputForSaveAction($action, $input);
	
	if ($action == 'save')
	{
		$input['my_extra_database_field'] = $this->_input->filterSingle('my_field_yo');
	}
}
}

After writing that out I can think of a better way to do it. Have that method protected (and possibly final) for starters and have it check for _action{$action}ExtraInput(array &$input). This would/could raise an issue for the extended method again though, they would have to check if the parent method existed before calling it. Still, anything is better than copying a whole function and essentially making add-ons incompatible.

Thoughts? Anyone got a better method?
 
Upvote 10
This would be very useful. If this was implemented, addon developers won't have to create their own save page in order to make sure actionSave()s don't clash.
 
Because of the missing tutorials and having still several addons overriding the whole save method to add their own input variables to the dw and not calling the parent method) ( which will break all other addons overriding the method) i would really appreciate a change here in 1.2


Solution1 (probably the fastest and simplest way;) ):
Create a official howto here: http://xenforo.com/community/forums/official-development-tutorials-and-resources.41/ ;)

or probably a cleaner way:

Solution2 :
Create a new Event "dw_presave" which will be called each time before the datawriter executes presave/ save inside the model/controller.

e.g.
PHP:
public function actionAddThread()
{
.....
XenForo_CodeEvent::fire('datawriter_presave' ,  array(&$writer, $this));
$writer->preSave();
...
}

Inside the event we could then do whatever we want(e.g.

PHP:
public static function addDataToThread(XenForo_DataWriter $dw, XenForo_Controller_Absract $controller){
if ($dw instanceOf  XenForo_DataWriter_Discussion_Thread && $dw->isInsert()){
$additionalData = $controller->getInput()->filter('foo');
 
$dw->set('my_new_field', $additinalData);
$dw->setExtraData(MY_Data::FOO, 'whatever i need for later user inside the dw');
//we could even manipulate already set data
}

Solution3: (similar to solution2 but introduces new methods inside the controllers, instead of the new event)
create a own method inside the controller, which will be called before the save action is called, with the dw as reference

then we could use the proxysystem to extend the controller, make our dw operations before the save gets called. (IMO THE BEST SOLUTION, if you don't want to start to introduce a new layer (form object) which is handling the controller <=> DW communication.





I really hope to see here some bigger changes in 2.0
http://symfony.com/doc/master/cookbook/form/index.html
http://community.invisionpower.com/blog/4445/entry-8660-40-forms/
 
st all situations when saving data (usually actionSave()) you have to replace the whole function when extending the initial controller and this just feels oh so wrong!

that's exactly what i mean:D
nobody needs to replace the whole function, it's working without with help of using $_POST, $_GLOBAL inside the DW, or using the Registry Object to save the variables inside the controller (to keep the code separated) and use them then inside the DW preSave function.
but yes, it's not really a clean way.
a class(e.g. form helper class with events, see symfony form) or a extramethod inside the controller called before the save action would help here extreme.




thx, yes it's a duplicate thread, i've reported it already

thx for the link:)
 
Thoughts? Anyone got a better method?

My solutions 2 & 3 are similar to your request (extra method inside the controller, or just a extra event, which is fired before preSave/save is being called in the controller).

But i would request to send the whole DW reference to these method or event and not only the input array, because this would give you more possibilities (the most important for me would be, that we're able to change already set input fields and that we could also set the extraData and extraOptions for the dw or call datawriter methods)
 
Because of
Code event speed improvements
This suggestion comes courtesy of digitalpoint. Many code events now offer a "key". When creating a listener, you can optionally specify the key to listen to. If you specify a key, your listener will only be called when the key matches.
i would really suggest to just use a new event for this.
Because of the "key" it wouldn't even be too much overhead and we wouldn't need to test the dw class instance
PHP:
XenForo_CodeEvent::fire('datawriter_presave' ,  array(&$writer, $this) 'XenForo_DataWriter_Discussion_Thread'); // i assume the key will be the 3. parameter because of BC
...
public static function addDataToThread(XenForo_DataWriter $dw, XenForo_Controller_Absract $controller){
if ($dw instanceOf  XenForo_DataWriter_Discussion_Thread
because it would be called for all "datawriter_presave" events.

and if the data could be created in ACP and Frontend, we wouldn't need to have the same code/method in the public and admin controller because the event listener could be used everywhere
 
And what's better with your code?
There's still the problem with adding additional input fields
PHP:
 public function actionSave()
    {
        if ( ! $this->repository->getById($this->_id))
        {
            return $this->responseNoPermission();
        }

        $this->repository->save($this->_id,  $this->_input->filter(array('one' => self::UINT, 'two' => self::STRING)));
 
I believe that the controllers needs to have all the data input in their own function.
Like, in your example, if one needs to send additional data to save(), it is not possible to do so, at least not without breaking the MVC contract, which sucks. Let me give a couple examples.

1) You added a new field to the create_thread, now you want to save that
2) You can overwrite the DataWriter for getFields() to add the new field
3) You need to send the field. Oh-oh, how? Not possible. Irritating

This leaves you with two options. An ok one, and a worse one.

Ok one

This is the approach I am taking. I override the controller (e.g. the actionUpdate()), I let it do it's thing by calling parent::actionUpdate, but right when it returns I capture it (has to be done with both a try-catch and the response), if it is NOT an error then I proceed with my update. I basically get the data from the input, and I call update() on the thread. Yeah .. this means an additional query. But at least I am not rewriting the function, it is modular, it does not break other functionality ....

This can be done with almost anything, member preferences updates, posts ...

Worse one

You could always override the preSave() method in the datawriter and check for your parameter in $_REQUEST, making sure to check with isset() in the chance that it is not set or the datawriter was called in another context
If it is there, then assume it is needed in the current transaction and call ->set()

I don't like this one, it moves the logic into the DataWriter, which should be context agnostic (but it will work)
 
This is the approach I am taking. I override the controller (e.g. the actionUpdate()), I let it do it's thing by calling parent::actionUpdate, but right when it returns I capture it (has to be done with both a try-catch and the response), if it is NOT an error then I proceed with my update. I basically get the data from the input, and I call update() on the thread. Yeah .. this means an additional query. But at least I am not rewriting the function, it is modular, it does not break other functionality ....
but doesn't work always.

e.g. if you creating a thread which is moderated:
actionAddThread
PHP:
        if (!$this->_getThreadModel()->canViewThread($thread, $forum))
        {
            $return = XenForo_Link::buildPublicLink('forums', $forum, array('posted' => 1));
        }

what now?;)
you don't have access to the last thread data,fetching the last threadid in the forum is also not 100% secure, what if another thread was created "at the same time"

you still ahve to hook into the dw and e.g. save the thread id to be able to use it later
 
but doesn't work always.

e.g. if you creating a thread which is moderated:
actionAddThread
PHP:
        if (!$this->_getThreadModel()->canViewThread($thread, $forum))
        {
            $return = XenForo_Link::buildPublicLink('forums', $forum, array('posted' => 1));
        }

what now?;)
you don't have access to the last thread data,fetching the last threadid in the forum is also not 100% secure, what if another thread was created "at the same time"

you still ahve to hook into the dw and e.g. save the thread id to be able to use it later

You basically replicate what the controller did, mainly 1) get the threadId from the $this->_input, and call the threadHelper to get the thread
 
The registration controller implements now a own method _getRegistrationDataFromInput where we can add own fields to the data array, which will be passed to dw->bulkSet

i hope the other controllers will implement this in 1.2 gold too. (Still not optimal but better then the current situation :P )
 
Ok one

This is the approach I am taking. I override the controller (e.g. the actionUpdate()), I let it do it's thing by calling parent::actionUpdate, but right when it returns I capture it (has to be done with both a try-catch and the response), if it is NOT an error then I proceed with my update. I basically get the data from the input, and I call update() on the thread. Yeah .. this means an additional query. But at least I am not rewriting the function, it is modular, it does not break other functionality ....

This can be done with almost anything, member preferences updates, posts ...

Worse one

You could always override the preSave() method in the datawriter and check for your parameter in $_REQUEST, making sure to check with isset() in the chance that it is not set or the datawriter was called in another context
If it is there, then assume it is needed in the current transaction and call ->set()

I don't like this one, it moves the logic into the DataWriter, which should be context agnostic (but it will work)
In my personal opinion...

I tend to gravitate more towards using $_POST ... on that point ... isn't $_POST more appropriate than $_REQUEST? Parameters passed using $_GET can potentially overwrite the $_POST values, can't they? Maybe that's another discussion for another day...

But it just seems a lot cleaner to do it this way.

Of course, I'm a big advocate of having something properly implemented in the core, but trying and catching and performing another query just doesn't seem right in my opinion.

I guess there isn't a right or wrong way and you'll carry on doing it your way, and I'll carry on doing it my way... but yeah, I wouldn't call my way "worse"... And I'm not saying your way is worse either...
 
In my personal opinion...

I tend to gravitate more towards using $_POST ... on that point ... isn't $_POST more appropriate than $_REQUEST? Parameters passed using $_GET can potentially overwrite the $_POST values, can't they? Maybe that's another discussion for another day...

But it just seems a lot cleaner to do it this way.

Of course, I'm a big advocate of having something properly implemented in the core, but trying and catching and performing another query just doesn't seem right in my opinion.

I guess there isn't a right or wrong way and you'll carry on doing it your way, and I'll carry on doing it my way... but yeah, I wouldn't call my way "worse"... And I'm not saying your way is worse either...
About your request question, the default order is GPCS (Get, Post, Cookie, Server).
 
That's the default order, sure, but that could be different depending on the variables_order directive in PHP.ini.

I would guess it would be pretty rare for the order to be different, but it just seems to be to be wiser simply to use $_POST if that's where the data is coming from.
 
I have to take back
i hope the other controllers will implement this in 1.2 gold too. (Still not optimal but better then the current situation :p )

If we have just a method returning the inputfields and no access to the dw, it's still not powerful enough and we have to hack around if we need to set e.g. datawriter options / dw extradata or even call a dw method
 
but doesn't work always.

e.g. if you creating a thread which is moderated:
actionAddThread
PHP:
        if (!$this->_getThreadModel()->canViewThread($thread, $forum))
        {
            $return = XenForo_Link::buildPublicLink('forums', $forum, array('posted' => 1));
        }

what now?;)
you don't have access to the last thread data,fetching the last threadid in the forum is also not 100% secure, what if another thread was created "at the same time"

you still ahve to hook into the dw and e.g. save the thread id to be able to use it later
What if the DataWriter is called in another context?

For instance, let's say we'd like to add a boolean field to xf_forum and have it editable by the admin in the Forum Options.

Now, if the field is set to false, it wouldn't be set in $_POST, and every time the forum DataWriter is saved, the custom field would be set to false if $_POST['custom_field'] is not set.

This is a problem because the forum DataWriter is not used exclusively when editing forums, but in some other cases (e.g. when rebuilding forums).

@Mike can you please address this?
 
Last edited:
@Mike @Kier The way I see it this is a serious issue, as it's currently impossible for add-on developers to add custom fields to datawriters in a reliable way.

Can you please at least respond to this?
 
Top Bottom