launchpad-dev team mailing list archive

["Francis J. Lacoste" <francis.lacoste@xxxxxxxxxxxxx>]

Hi,

I agree with Salgado, I don't think removing that checks is the right 
solution.

And I don't really think there is a problem either. create_view and 
create_initialized_view have a layer parameter. Why don't you use that 
parametere?

Cheers

-- 
Francis J. Lacoste
francis.lacoste@xxxxxxxxxxxxx

On August 18, 2010, Guilherme Salgado wrote:
> 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

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