More modular code

[Robbo]

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)...

$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...

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?

 

[Chris D]

I've created about 5 add-ons using your exact example of a boolean field added to the xf_forum table and in every scenario I've used, it works fine.

protected function _preSave()
{
     parent::_preSave();

     if (!empty($_POST['disallow_polls']))
     {
          $this->set('disallow_polls', $_POST['disallow_polls']);
     }
}

I've done many things that would call the Forum DataWriter in varying contexts and there's no problem. The value remains set as required.

 

[Lior.]

I've created about 5 add-ons using your exact example of a boolean field added to the xf_forum table and in every scenario I've used, it works fine.

protected function _preSave()
{
     parent::_preSave();

     if (!empty($_POST['disallow_polls']))
     {
          $this->set('disallow_polls', $_POST['disallow_polls']);
     }
}

I've done many things that would call the Forum DataWriter in varying contexts and there's no problem. The value remains set as required.

How would this work in case disallow_polls is a checkbox?

$_POST['disallow_polls'] would be empty if it's unchecked, therefore it'd be impossible to set the value to false.

 

[Chris D]

You're right, of course. Oversight on my part.

That's where it has to get even more hacky...

We now have to have in the forum edit form:

<input type="hidden" name="forum_edit_form" value="true" />

And check for that value in the DataWriter.

If that value isn't in $_POST then that field shouldn't be changed.

If that is in $_POST and the disallow_polls flag IS empty then the disallow_polls flag can be set to 0.

Yuck. But, we have to make the best of what we have right now. I flagged this thread up to Mike a few weeks ago and at the time it wasn't planned for 1.2...

 

[Lior.]

You're right, of course. Oversight on my part.

That's where it has to get even more hacky...

We now have to have in the forum edit form:

<input type="hidden" name="forum_edit_form" value="true" />

And check for that value in the DataWriter.

If that value isn't in $_POST then that field shouldn't be changed.

If that is in $_POST and the disallow_polls flag IS empty then the disallow_polls flag can be set to 0.

Yuck. But, we have to make the best of what we have right now. I flagged this thread up to Mike a few weeks ago and at the time it wasn't planned for 1.2...

Hehe, yeah, this is similar to what I ended up doing, but I don't think it's a satisfying solution at all, and I simply don't understand why we don't have something as basic as adding custom fields in 1.2.

This thread was opened 16 months ago, and it's a shame it doesn't get any attention whatsoever. Disappointing.

 

[AlexT]

The way I dealt with this stuff... check for the field in the respective controller first (XenForo_ControllerPublic_Thread), save the result in a global, then check the global in the datawriter. Hacky (since globals are evil), but it works.

So

if ($this->_request->isPost())
{
   $hidden_results = $this->_input->filterSingle('hidden_results', XenForo_Input::UINT);
   $GLOBALS['mr_hpr_hidden_results'] = (isset($hidden_results)) ? $hidden_results : 0;
}

In the controller.

Then

protected function _preSave()
{
   if (isset($GLOBALS['mr_hpr_hidden_results']))
   {
      $hidden_results = ($GLOBALS['mr_hpr_hidden_results'] === 1) ? 1 : 0;
      $this->set('hidden_results', $hidden_results);
   }

   parent::_preSave();
}

In the datawriter.

 

[Dan]

We're doing the "same" but instead of using $GLOBALS we're using using the xenforo registry object (XenForo_Application)

Inside the Controller:

XenForo_Application::set('myKey', $inputData);

and in the datawriter:

protected function _preSave()
{

if (XenForo_Application::isRegistered('myKey')) {
         $inputData = XenForo_Application::get('myKey');
      $this->set('hidden_results', $inputData );
}

parent::_preSave();
}

 

[AlexT]

@ExtraLicense , wouldn't that add extra queries to it? Given that the information is volatile and doesn't need to survive more than the particular pageview, I think it should be avoided.

To avoid globals, as in my example above, one could also add a singleton class and use that singleton as a global storage. Again, it's nothing pretty.

 

[Dan]

Sorry, it's not the XenForo Data Registry which i was talking about.

I mean XenForo_Application (which extends Zend_Registry) => exactly what you're talking about ( a singleton storage object )

/**
* Base XenForo application class. Sets up the environment as necessary and acts as the
* registry for the application. Can broker to the autoload as well.
*
* @package XenForo_Core
*/
class XenForo_Application extends Zend_Registry




/**
* Generic storage class helps to manage global data.
*
* @category  Zend
* @package  Zend_Registry
* @copyright  Copyright (c) 2005-2010 Zend Technologies USA Inc. (http://www.zend.com)
* @license  http://framework.zend.com/license/new-bsd  New BSD License
*/
class Zend_Registry extends ArrayObject

 

[AlexT]

Ahh, OK. Yes, that's good. I'll keep your solution in mind.

 

[Mike]

@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?

You will need to find a workaround. That's really all I could say. There may be places that change over time, but there are so many potential locations so it's not like I could say "yes, everything will change" or anything like that.

Hacks they may be, but there are possible workarounds and if you're trying to override bits of behavior within the existing system, that's roughly what's going to be required if the code structure doesn't fit your needs.