review for fixes of bugs due to acquisition black magic

classic Classic list List threaded Threaded
7 messages Options
Godefroid Chapelle Godefroid Chapelle
Reply | Threaded
Open this post in threaded view
|

review for fixes of bugs due to acquisition black magic

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.

-
https://github.com/plone/Products.CMFPlone/commit/6d6710728b35b97e2f328c454c8688346612b1e1 


is a fix for https://dev.plone.org/ticket/13603

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.

Looking forward to hearing from you all.
--
Godefroid Chapelle (aka __gotcha) http://bubblenet.be


------------------------------------------------------------------------------
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
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers
Maurits van Rees-3 Maurits van Rees-3
Reply | Threaded
Open this post in threaded view
|

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
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers
David Glick (Plone) David Glick (Plone)
Reply | Threaded
Open this post in threaded view
|

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

On 11/7/13, 3:42 AM, Maurits van Rees wrote:

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

I like trying to address this problem, but I am also worried about
breaking existing sites that inadvertently rely on Acquisition in their
links. Perhaps we should only make the change for Plone 5.


------------------------------------------------------------------------------
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
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers
Timo Stollenwerk-2 Timo Stollenwerk-2
Reply | Threaded
Open this post in threaded view
|

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

Am 07.11.2013 19:14, schrieb David Glick (Plone):

> On 11/7/13, 3:42 AM, Maurits van Rees wrote:
>> 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.
>
> I like trying to address this problem, but I am also worried about
> breaking existing sites that inadvertently rely on Acquisition in their
> links. Perhaps we should only make the change for Plone 5.

This commit also broke the 4.3 build:

http://jenkins.plone.org/job/plone-4.3-python-2.7/1585/

This has not been fixed yet, therefore the commit should be reverted
immediately in any case.

The testbot mailinglist seems to be broken right now, Ramon is working
on a fix.

Timo

------------------------------------------------------------------------------
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
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers
Jota Jota
Reply | Threaded
Open this post in threaded view
|

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

This post has NOT been accepted by the mailing list yet.
In reply to this post by David Glick (Plone)
David Glick (Plone) wrote
I like trying to address this problem, but I am also worried about
breaking existing sites that inadvertently rely on Acquisition in their
links. Perhaps we should only make the change for Plone 5.
   
   
In my opinion, this kind of fix, that could break something on existing instances, should be done in a point release prior to Plone 5.
If they only arrive with P5, people may get confused when facing breakage, because of the many new features that P5 will bring.
The other reason is that this are important fixes.
J.
Rok Garbas-2 Rok Garbas-2
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by David Glick (Plone)
Quoting David Glick (Plone) (2013-11-07 19:14:50)

> On 11/7/13, 3:42 AM, Maurits van Rees wrote:
> > 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.
>
> I like trying to address this problem, but I am also worried about
> breaking existing sites that inadvertently rely on Acquisition in their
> links. Perhaps we should only make the change for Plone 5.
>

i would say definetly for Plone 5. but we need to document it. we'll be also
breaking some type of links with <base/> removal plip[1]. +1 for Plone 5 as
long as the change is its documented.


[1] https://dev.plone.org/ticket/13705

--
Rok Garbas - http://www.garbas.si

------------------------------------------------------------------------------
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
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers
Godefroid Chapelle Godefroid Chapelle
Reply | Threaded
Open this post in threaded view
|

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

In reply to this post by Maurits van Rees-3
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
_______________________________________________
Plone-developers mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/plone-developers