Not a bug Current Sidebar Modification Methods Invite Conflict

[pegasus]

Currently there are 3 ways to modify the sidebar:
1. Pass to container as controller->responseView(..., ..., ..., $containerParams_with_Sidebar_key)
2. Set local $__extraData in template_post_render for PAGE_CONTAINER ($containerData in a listener)
3. XenForo_Template_Public::_mergeExtraContainerData(array('sidebar' => $html))

There are inherent problems with these methods.

First, every add-on must know to check all 3 for existing modifications in order to avoid breaking them. Even popular mods like [bd] Widget Framework currently don't check all 3.

More importantly, if method #3 or #2 is used by a different mod, XenForo internally overwrites sidebar changes made by method #1.

This could easily be avoided by including even a rudimentary Sidebar factory in the XenForo core. e.g.

class XenForo_Helper_Sidebar
{
protected static $sidebar = array();

public static function push($html)
{
self::$sidebar[] = $html;
}

/* for internally populating PAGE_CONTAINER $sidebar */
public static function getHtml()
{
return implode('', self::$sidebar);
}
}

 

[Mike]

#1 isn't considered a valid method. It would involve HTML in the controller which is really just plain wrong.

I'm not sure if #2 was description error, but if you're calling post render on PAGE_CONTAINER, I think it'd be too late to setup the sidebar.

#3 is a protected method so that won't really be callable outside where it's called right now.

From our perspective, the only correct ways to setup a sidebar is via the template itself with a sidebar tag or possibly by modifying it via template_post_render (both of which would only be changing the extra data). Any other method is basically incidental and not an expected behavior.

It's not something I totally agree with, but the widget framework shows a case where blanking out the existing sidebar is desireable

 

[xfrocks]

Yeah, I don't even acknowledge anyone that does any of these 3 methods. Always thought the only (proper?) way is to use <xen:sidebar />...

 

[pegasus]

I completely forgot about <xen:sidebar />
That counts as a "factory" in my opinion. It works the same as the class I posted.

#1 isn't considered a valid method. It would involve HTML in the controller which is really just plain wrong.

You may also be able do #1 this way (more semantic MVC use of $containerParams), by extending a View class (load_class_view), then pushing the sidebar HTML onto. e.g.

mySidebar_ForumList extends XFCP_XenForo_ViewPublic_Forum_List
{
public function renderHtml()
{
// do PHP stuff since sidebar needs it
if (!isset($this->_response->containerParams['sidebar']))
{
$this->_response->containerParams['sidebar'] = '';
}
$this->_response->containerParams['sidebar'] .= $mySidebar;
// expect it to be pushed to PAGE_CONTAINER $sidebar
// but any <xen:sidebar /> or template_post_render (on other templates) would erase it
parent::renderHtml();
}
}

I haven't tested this particular variant, so it is possible that ::renderHtml is called after $containerParams is already pushed to PAGE_CONTAINER. I just want to make sure all our bases are covered here.

 

[pegasus]

@Mike this was closed a while back, but after some more testing, it seems that <xen:sidebar> is not a workable solution, or the behavior has changed in more recent releases...

After experiencing sidebar conflicts between many more add-ons, it seems that only the most recent <xen:sidebar> call adds HTML to the sidebar. Any previous calls are lost.

While this is a simple example in the same template, the problem DOES seem to extend across templates and modifications. Try this in a view template that would be the subview of PAGE_CONTAINER:

<xen:sidebar>my sidebar blocks</xen:sidebar>
<xen:sidebar>only this sidebar block shows</xen:sidebar>

A more practical example is if the template uses <xen:include> on another template that also uses <xen:sidebar>. The culprit seems to be in XenForo_Template_Compiler::compileIntoVariable, which reinitializes $var = ''; on each call.

Again this invites conflict, and seems to be doing just that. If <xen:sidebar> is truly meant to SET the sidebar, and losing a previous set is intentional, then please implement my original suggestion of a factory so that add-ons can safely ADD to the sidebar in an obvious way without affecting previous adds. Document that <xen:sidebar> only works one time.

The other "methods" I posted in the first post either don't work or are not considered appropriate, and the template_post_render method you suggested seems like a hack to me, still requires modifications to be aware of each other to prevent conflicts, and is not obvious for something so typical as adding custom sidebar blocks.

I feel like it would be much more appropriate and safe to simply push sidebar HTML into a XenForo_Helper_Sidebar in the renderHTML method of ViewPublic classes (or any other time we want), and have XenForo insert that into PAGE_CONTAINER either above or below the (only visible) <xen:sidebar> call (that would be a design decision).

Another possibility is to have a "sidebar" template with sidebar_first, sidebar_top, sidebar_middle, sidebar_bottom, sidebar_end easy-grab positions that mods can just use template modifications on to add their stuff. PAGE_CONTAINER could just xen:include the new "sidebar" template.

Even so, any new method would require mods to be aware that <xen:sidebar> is not safe if another mod happens to use it later in the same parse. I would recommend also fixing <xen:sidebar> so that it's a template-based push to the new helper class, and use a final get from the helper for the PAGE_CONTAINER. This would prevent anyone from blanking things they aren't supposed to or even don't intend to.

Of course, mods like WidgetFramework might still want to explicitly blank the whole sidebar. I would say just to let them do that with a template modification that removes the default {xen:raw $sidebar} from PAGE_CONTAINER if their criteria is met.

 

[Jeff Berry]

I am also a bit concerned with the way the sidebar is implemented in XF.

I am developing an add-on similar to the Widget Framework add-on, just a little bit more robust. This add-on requires me to completely remove the sidebar from the site and instead we turn the entire layout into a grid, so they can have multiple sidebars, move their sidebars, change the sidebar's width, etc.

What @pegasus is suggesting with the factory is something that I believe should be considered. It would help with the conflicts that myself and I'm sure other developers are running into when trying to make our add-ons compatible with other add-ons that modify or adjust the sidebar.

I currently have a listener that acts kind of like a factory, it hooks to template_post_render and will take the 'sidebar' data (from inside <xen:sidebar>)out of each template that is rendered and concatenate it to a global sidebar variable. This happens before it makes it to XF's sidebar variable, so I become responsible in re-injecting this concatenated sidebar string back into the template. You can see a code snippet here:

public static function template_post_render($templateName, &$content, array &$containerData, XenForo_Template_Abstract $template)
{
    if (!XRGrid_Core::isGridEnabled())
    {
        return;
    }

   if (isset($containerData['sidebar']))
   {
        if (empty(self::$sidebar))
        {
            $visitorPanel = $template->create('sidebar_visitor_panel', $template->getParams());
            self::$sidebar = $visitorPanel->render();
        }
    
        self::$sidebar .= $containerData['sidebar'];
        unset($containerData['sidebar']);
   }
}

I still run into issues when a developer tries to add widgets in a way that is not through <xen:sidebar> though. I think it would be very easy to convert your stock sidebar implementation into more of a "stack" so that it can be better worked with by developers. Currently having the entire sidebar contents overwritten any time <xen:sidebar> is used and also offering no programatic way to add content to the sidebar is very limiting.

 

[Jay]

One of you should make a suggestion detailing the design flaws of the current implementation and what you feel should be changed and why.

 

[Alpha1]

@Brogan Maybe this this thread can be moved to suggestions?