launchpad-dev team mailing list archive

[Danilo Šegan <danilo@xxxxxxxxxxxxx>]

Hi Tim,

У сре, 18. 08 2010. у 23:24 +1200, Tim Penhey пише:

> While trawling through the errors with my latest branch I'm pretty sure that I 
> have found a bug with canonical_url.
> 
> Following a recent discussion about facets and layers, I decided to go and be 
> more strict on the views for the code application by adding 
> layer="lp.code.publisher.CodeLayer" to the ZCML directives for the pages.
> 
> This caused (quite) a number of tests to fail.
> 
> create_initialized_view needed to have the layer passed in.  That was fine.
> 
> However, I hit the following:
> 
>   canonical_url(branch) would be fine
>   canonical_url(branch, view_name="+edit") would fail with one of two errors:
>     - view not registered
>     - or no request passed in
> 
> The view check uses the current browser request (if one isn't passed in).  
> This is often the on the OverviewLayer or some other layer.  The views we want 
> aren't registered on those, but on the "code" rootsite (or CodeLayer).  I went 
> looking to see how it was done elsewhere, but it looks like *no-one* else 
> specifies layers on the page registrations, only on default views.  So this 
> kinda explains why we haven't hit this before.

We've hit it before (we do specify layers on pages).  We directly passed
rootsite="translations" where we needed it instead.  I've also hit
similar problems with create_view calls which don't really create views
using the appropriate layer, so you have to pass it in manually (even
when ZCML specifies it).

Also, it seems we very rarely used view_name in canonical_url in the
first place.  So, I guess we instinctively avoided the problem by
hand-crafting URLs instead (i.e. "canonical_url(something) + '/+export'"
patterns that I've grepped out).

> I spent quite a bit of time talking this through with Michael Hudson this 
> afternoon, and we came up with two main solutions:
> 
>  1) Drop the check for the registration of the view as it isn't checking the 
> right thing anyway

I'd go with this right away (provided I understood it correctly, see
below).

>  2) Change the implementation to use the Layer defined for the rootsite.  Since 
> the getMultiAdapter check is just using the request to check on the layer that 
> is being provided, we can fake this if we have a link from the rootsite to the 
> layer for that rootsite. 
>   - However right now we don't.  The suggestion was to change the meta-zcml 
> directive for "publisher" to define the layer for the rootsite.  This could 
> then be used to create the request and publisher factory using a closure and 
> avoid a lot of the code duplication in the different app's publisher code.  We 
> would still need the layers defined in each app, but the request class and 
> factory method could be defined through the code that handles the "publisher" 
> ZCML directive.  This approach is somewhat more complex than the "just remove 
> the check" approach.

Well, I admit to not understanding this fully (and not spending too much
time on this for some time already), but we've also got definitions
like:

        <browser:defaultView
            for="canonical.launchpad.interfaces.ILanguage"
            name="+index"
            layer="lp.translations.publisher.TranslationsLayer"/>
        <browser:url
            for="canonical.launchpad.interfaces.ILanguage"
            path_expression="code"
            parent_utility="canonical.launchpad.interfaces.ILanguageSet"
            rootsite="translations"/>
        <browser:page
            for="canonical.launchpad.interfaces.ILanguage"
            class="lp.translations.browser.language.LanguageView"
            permission="zope.Public"
            template="../templates/language-index.pt"
            name="+index"
            layer="lp.translations.publisher.TranslationsLayer"/>
        <browser:page
            for="canonical.launchpad.interfaces.ILanguage"
            class="lp.translations.browser.language.LanguageAdminView"
            permission="launchpad.Admin"
            template="../../app/templates/generic-edit.pt"
            name="+admin"
            layer="lp.translations.publisher.TranslationsLayer"/>

We explicitely set rootsite="translations" on browser:url for ILanguage,
and that's why canonical_url works for the default view.  However, for
+admin view, it should also extract the rootsite from ILanguage
browser:url, and I guess that's what you achieve by simply removing the
check (your option 1).

However, redundancy is quite obvious in a definition like this.
Ideally, I'd just specify a default layer for ILanguage and that would
imply the rootsite as well (basically what you suggest in 2: creating a
link between a layer and rootsite).  But, I'd also like to avoid having
to specify the layer on each of the remaining views for this interface,
unless it's a different layer.

I am explicitely not talking about the specifics you are asking about,
because I would probably say a bunch of nonsense without looking at the
code first. :)

But, I think we can best resolve this by deciding exactly what for and
how we want to use ZCML.  In essence, how do we get the notion of layers
into queryMultiAdapter calls when fetching views?


> I wanted to get a second opinion and a sanity check.

I am probably providing neither though :)

Cheers,
Danilo