Re: review for fixes of bugs due to acquisition black magic

Hi Godefroid,

Godefroid Chapelle schreef op 06-11-13 18:09:
> Hi all,
>
> I committed fixes in 4.3.x branch for two nasty acquisition bugs.
>
> I'd like to get some feedback :
>
> -
> https://github.com/plone/Products.CMFPlone/commit/e20ea907d2854ee7ad41e9953b64889ee7431b49
>
>
> is a fix for https://dev.plone.org/ticket/13793
>
> but it might break some sites that have wrongly built URLS, iow URLS
> where content is currently made accessible through acquisition.

This has the potential to break sites that rely on 'wrongly built urls'. 
  One example is when the main_template has '<img src="header.jpg" />' 
to show the header.jpg in the context.  This is a simple way to have 
different header images for different parts of your site.  There are 
better ways, especially if you consider the effect this has on caching, 
but I think this does happen.

You give an example in the ticket: Joe deletes root/department/news and 
Jane has this in a bookmark, visits it and gets served the content of 
root/news under the url root/department/news.  What happens if Jane 
visits root/department/news/some_template?  Does your event handler 
raise a 404 then?

Oh, sudden alternative idea: do not raise a 404 but simply redirect to 
the correct url.  That may still have unwanted side effects, but it 
would be less, I think.

For example it may redirect to a lower level url that is not available. 
  The root/department folder may be a navigation root and may be 
available at department.example.org.  If Jane visits 
department.example.org/news I cannot think of a correct url to redirect to.

It may be totally intended that she sees the content that is in reality 
located at root/news.  This is an argument against redirecting, but also 
against raising a 404.

Possibly, the code could check whether the new url is within the same 
navigation root and only redirect then.  If it is outside the navigation 
root, it may be best to keep the current behavior and just serve the 
acquired content.


> https://github.com/plone/Products.CMFPlone/commit/6d6710728b35b97e2f328c454c8688346612b1e1
>
> is a fix for https://dev.plone.org/ticket/13603

Nasty bug.  I haven't tried it, but it looks like a good fix too me, 
including the test.

> but it breaks backward compatibility by forbidding use of relative paths.
>
> In both cases, I think that not fixing the bug is worse than the
> side-effect of the fix. But maybe some of you disagree... or have ideas
> for better fixes.

-1 for merging the first.

+1 for merging the second.

Cheers,

-- 
Maurits van Rees: http://maurits.vanrees.org/
Zest Software: http://zestsoftware.nl


------------------------------------------------------------------------------
November Webinars for C, C++, Fortran Developers
Accelerate application performance with scalable programming models. Explore
techniques for threading, error checking, porting, and tuning. Get the most 
from the latest Intel processors and coprocessors. See abstracts and register
http://pubads.g.doubleclick.net/gampad/clk?id=60136231&iu=/4140/ostg.clktrk