launchpad-dev team mailing list archive

[Guilherme Salgado <salgado@xxxxxxxxxxxxx>]

On Wed, 2010-08-18 at 23:24 +1200, Tim Penhey wrote:
> Hi Guys,
> 
> 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

I think that for the view tests where you create manually (or has a way
of specifying the request), we should make it easy to specify a request
that provides the layer on which the view is registered.  That way the
above would work as the current browser request would provide the
correct layer.

This won't be of help for testbrowser tests, though, but for those we're
using the zope publication, which should make sure the requests provide
the appropriate layer according to the vhost you used.

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

I think that's the real problem -- we should either infer the layer from
the vhost used (in case of testbrowser tests) or make it easy for tests
to specify them.  Would that fix the test failures you saw?

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

As you saw, this is not widely used, but it's (IMO) a very nice
mechanism (that we should be using more!) to make sure we don't expose
404 links to users, so I'd be against removing it. 

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

That'd work, but we'd be changing production code just to please tests
as the errors you're seeing don't happen in production.  In production
the requests always implement the layer that is appropriate for the
vhost you're on.

>   - 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.
> 
> I wanted to get a second opinion and a sanity check.
> 
> Tim
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~launchpad-dev
> Post to     : launchpad-dev@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~launchpad-dev
> More help   : https://help.launchpad.net/ListHelp

-- 
Guilherme Salgado <https://launchpad.net/~salgado>

Attachment: signature.asc
Description: This is a digitally signed message part