1. This site uses cookies. By continuing to use this site, you are agreeing to our use of cookies. Learn More.

Not a Bug Current Sidebar Modification Methods Invite Conflict

Discussion in 'Resolved Bug Reports' started by pegasus, Mar 11, 2014.

  1. pegasus

    pegasus Well-Known Member

    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);
    Alfa1 and Rob like this.
  2. Mike

    Mike XenForo Developer Staff Member

    #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
  3. xfrocks

    xfrocks Well-Known Member

    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 likes this.
  4. pegasus

    pegasus Well-Known Member

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

    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
    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.
    Last edited: Mar 11, 2014
  5. pegasus

    pegasus Well-Known Member

    @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.
    Last edited: Jul 18, 2014
    Alfa1 likes this.
  6. Jeff Berry

    Jeff Berry Well-Known Member

    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 &$containerDataXenForo_Template_Abstract $template)
        if (!

       if (isset(
            if (empty(
    $visitorPanel $template->create('sidebar_visitor_panel'$template->getParams());
    self::$sidebar $visitorPanel->render();
    self::$sidebar .= $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.
    Alfa1 and pegasus like this.
  7. Jay

    Jay Active Member

    One of you should make a suggestion detailing the design flaws of the current implementation and what you feel should be changed and why.
    Chris D likes this.
  8. Alfa1

    Alfa1 Well-Known Member

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

Share This Page