#234: Standardizing our use of INavigationRoot

Contents
  1. Motivation
  2. Proposal
  3. Implementation
  4. Risks
by Calvin Hendryx-Parker — last modified Jan 21, 2010 07:28 AM

Plone has the INavigationRoot marker interface which supports rooting your tabs, navigation, breadcrumbs, sitemaps and searches to a specific container inside of a portal which itself is a INavigationRoot. Some areas of this need to be cleaned up and standardized to follow a consistent convention.

Proposed by
Calvin Hendryx-Parker
Proposal type
Architecture
Assigned to release
State
completed

Motivation

Currently the INavigationRoot is used in quite a few areas. The main use is in NavtreeQueryBuilder and the SitemapNavtreeStrategy which are used by CatalogNavigationTree and CatalogNavigationTabs. You can also find it in the PhysicalNavigationBreadcrumbs which is the class for the breadcrumbs_view.

It appears that the intention of the live search and the global search box is to use getNavigationRoot() to constrain the search to the INavigationRoot, but the action of the form isn't obeying the INavigationRoot and always posts to the portal absolute_url instead. If you change the URL it posts to, the search boxes will work as expected because of the following:

  • In CMFPlone/skins/plone_scripts/livesearch_reply.py:70 getNavigationRoot() does get called, but since the URL that the livesearch posts to is the absolute_url of the portal the root isn't correct even if the search is done from a folder that does have the INavigationRoot marker interface.
  • CMFPlone/skins/plone_scripts/queryCatalog.py:72 does the same thing, but doesn't work from the global search box or search viewlet due to the action url of the form. If you call /Plone/mynavroot/search it will do the right thing since it will post back to mynavroot and getNavigationRoot() will find the mynavroot if it has the INavigationRoot marker interface on it.

The last place I have found this is in plone.app.redirector/plone/app/redirector/browser.py:72 where it currently works and constrains the search_for_similar method to just the INavigationRoot it finds.

The sitemap does do the right thing if it is called in the right context. Calling the sitemap_view anywhere in a container that has the INavigationRoot marker interface will give you a sitemap of just that root and not the whole portal. Again all that needs to be fixed here is the URL that is put in for the plone.site_actions so that it will write them rooted at the nearest INavigationRoot. Actually, all of the plone.site_actions should probably be written with the INavigationRoot in mind so they work correctly.

Proposal

We would need to fix at a minimum the search portlet and plone.searchbox viewlet which are both posting to the incorrect URLs. As it stands getNavigationRoot() never returns what would be expected if you are calling these from inside of a folder with the INavigationRoot marker on it.

At this point I don't think we need the ISearchRoot marker interface since it looks like this is really just a bug and not new functionality. The searches are supposed to be rooted at the INavigationRoot, but due to the form actions not being written correctly the code that does this never picks up the right root.

Implementation

  • Fix the site_url property of plone.app.layout.viewlets.common.ViewletBase to return the properly rooted url using the PortalState.navigation_root_url() instead of the portal absolute_url
  • Fix the plone.site_actions to generate their URLs based on navigation_root_url also

Risks

The only potential change in behavior would be when I fix plone.app.layout/plone/app/layout/viewlets/common.py to set the site_url property on the view to the INavigationRoot. If I do this then the search boxes will generate the URL for the form actions correctly, but the PersonalBarViewlet and DocumentBylineViewlet are using this property to write the url for the author page and dashboard links. I haven't seen any other code that is touching this so I think it is low risk at this point and would be good to change early in case people start depending on this.

Suggestion

Posted by Christian Schneider at Oct 27, 2008 09:35 AM
+1 on consistent behaviour for the INavigationRoot, but I would suggest splitting things up even further. Right now the INavigationRoot does a whole lot in one big thing, but I have encountered use cases where I want to have somewhat contained sections of a site that restrict searching and display portal tabs for one given section for example, but still have the breadcrumbs display every item up to the real site root. If every element had its own marker interface, i.e. IBreadcrumbRoot/ISearchRoot/IPortalTabsRoot/ISitemapRoot... things would be much more flexible. Creating the current INavigationRoot behaviour for sections where breadcrumbs, sitemap, search, portal tabs and navigation should be affected would just be a simple UI matter.

Framework team vote

Posted by Danny Bloemendaal at Oct 27, 2008 03:17 PM
I refrain from voting on this plip. I don't have the knowledge to cast a proper vote.

3.3 Framework team vote

Posted by Martijn Pieters at Oct 27, 2008 03:49 PM
I am -1 on this for the same reasons as Wichert; instead of altering the view to change site_url, add a navigation_root_url instead and alter views to use that attribute intead. If this proposal were changed in this way I'd vote +1.

framework team vote

Posted by Raphael Ritz at Oct 28, 2008 09:55 AM
+1 on the updated version

3.3 Framework team vote

Posted by Martijn Pieters at Oct 28, 2008 09:59 AM
+1 now that our concerns have been addressed.

Framework Team vote

Posted by Tom Lazar at Oct 28, 2008 10:18 AM
+1 on the updated version, i.e. where we "don't change the return value of site_url, but instead change things over to use a navigation_root_url attribute instead"

Framework team vote (for merging into Plone 3.3)

Posted by Andreas Zeidler at Jan 27, 2009 08:28 PM
-1 (see http://lists.plone.org/[…]/002652.html)

Framework team Plone 3.3 review #2

Posted by Tom Lazar at Jan 30, 2009 06:53 PM
PLIP 234 framework review #2 by Tom Lazar (tomster), 2009-01-30
==============================================================

I have repeated the test runs and manual test steps that andi detailed
above and came to the identical findings (not surprising, really, as there
haven't been any code changes since and Andi and I run the same platform...)

I have also reviewed the code changes for `Plone`__, `plone.app.layout`__ and
`plone.app.portlets`__ and found nothing amiss (except the `__init__` issue for
portlets that andi mentions above).

.. __: http://dev.plone.org/plone/[…]ne%2Fbranches%2F3.2%4024100
.. __: http://dev.plone.org/plone/[…]ut%2Fbranches%2F1.1%4024100
.. __: http://dev.plone.org/plone/[…]pp.portlets%2Ftrunk%4024100

I confirm resp. agree with andi's notes and observations, as well as with his
conclusion, particularly with the assessment that all of this is still fixable
during the second revision period.

Framework team Plone 3.3 review #2 (round two)

Posted by Tom Lazar at Feb 12, 2009 10:27 PM

I have reviewed the changes in the documentation and in the three affected packages
since december and have conducted another local TTW test.

The test showed that the application of INavigationRoot worked as advertised. While I
personally still feel that the switch of the root in the navigation tree once you reach
a new root is kind of 'creepy' I realize that a) this is intended and b) that in actual
deployment the sub-roots are usually served under different domains from the front end
webserver, so the effect won't be observable to users.

While I agree (with andi), that in a perfect world, this PLIP would come with more tests, I
also agree (with calvin, optilude etc.) that this PLIP is by nature more a bug fix than a
new feature. I don't think we can reasonably expect that all fixes to Plone must come with
100% test coverage, or else they will not be accepted.

Seeing that most changes are actually in template markup and not so much in code and given
the test of the base portlet and that no existing tests break I vote +1 on this.

Can I guarantee that it absolutely won't break anything? No. Am I convinced that Plone is better
with this PLIP than without it? Yes :-)