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

Le 07/11/13 12:42, Maurits van Rees a écrit :
> 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

That change was reverted by Timo because it broke tests. (Thanks to Timo)

I moved it to the branch publication-through-explicit-acquisition.

https://github.com/plone/Products.CMFPlone/tree/publication-through-explicit-acquisition

This is the place where I'll work on that issue for now.

>> >
>> >
>> >is a fix forhttps://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?

Yup, I added a test to prove it.

https://github.com/plone/Products.CMFPlone/commit/4ae17f9733e94397acc6ad305caf90d59ac873d3

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

I agree that publication through acquisition is a valid use case that we 
want to keep in Plone.

Nevertheless, keeping it implicit leads to nasty side effects like 
described in https://dev.plone.org/ticket/13793

I think that it is reasonable to ask sites that want to use publication 
through acquisition to do it explicitely.

In the branch mentioned above, the NotFound is not raised anymore if the 
content provides a IPublishableThroughAcquisition marker interface.

https://github.com/plone/Products.CMFPlone/commit/f09d78548b7acbc0e1c37fffe781005e4863d9c9

Feedback ?
-- 
Godefroid Chapelle (aka __gotcha) http://bubblenet.be


------------------------------------------------------------------------------
Shape the Mobile Experience: Free Subscription
Software experts and developers: Be at the forefront of tech innovation.
Intel(R) Software Adrenaline delivers strategic insight and game-changing 
conversations that shape the rapidly evolving mobile landscape. Sign up now. 
http://pubads.g.doubleclick.net/gampad/clk?id=63431311&iu=/4140/ostg.clktrk