tapestry-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andy Blower (JIRA)" <j...@apache.org>
Subject [jira] Issue Comment Edited: (TAP5-577) TAP5-422 changes break persistent locale backwards compatibility.
Date Thu, 12 Mar 2009 21:32:50 GMT

    [ https://issues.apache.org/jira/browse/TAP5-577?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12681499#action_12681499
] 

Andy Blower edited comment on TAP5-577 at 3/12/09 2:31 PM:
-----------------------------------------------------------

Howard, 

I agree that using cookies to track user's persistent locale is not industry standard, but
it happens to be the method used by the final public release of T5.0 which is the important
point here. The fact that the default method for persisting locales is different is really
only a backwards compatibility issue for anyone who's relying on the generated URLs for some
reason or has issues with locale being in the URLs. (like our site which needs durable and
shareable URLs regardless of a users' locale preference)

To my mind this is a judgement call for you to take as it's quite a subjective aspect of backwards
compatibility, assuming of course that it's easy to override the default behaviour like it
was in T5.0 where you provided an easy way with a public interface. This is where I have the
main (less subjective, more objective) issue...

You are right that the functionality of persisting locale once set is still retained and is
just done using a different method in T5.1, but only for people who haven't implemented their
own PersistentLocale service. If they have then it will break on upgrading to T5.1 because
the service interface (which is public and not internal) is now used in a different way by
the internals - in other words the contract you made for this public interface is broken.
If it were as simple as changing the symbol to false to re-enable the old contract with the
public PersistentLocale service then it wouldn't be so bad, but currently it's non-trivial
for someone to get their custom PersistentLocale services working in T5.1 which is why I consider
backwards compatibility to be broken in this instance.

Regarding your statement "vast majority of users (who were not mucking about in the internals
of Tapestry to customize persistent locales)" - I really don't think that creating and contributing
a custom implementation of a public service interface can be characterized in this way. Especially
by you. I thought that this was what T5 was all about; functional out of the box, but allowing
powerful customization where it just "gets out of the way" to let you do what you need. Have
I got this wrong?

I definitely have mucked about with Tapestry internals and I will have some upgrade pains
due to these modification, but I knew that when I did it and you will not hear a thing from
me about backwards compatibility when I've messed with internals (even if there was no other
way in T5.0 to achieve what I needed to do) but that is not the case here.

I think I can safely assume that you do not consider reverting the default behaviour of T5.1
to cookie locale persistence an option which is fair enough. I don't think much of cookie
persistence myself so I'm certainly not bothered by it, I just hope it doesn't trip up too
many others. That's my only concern, although you seem to have confidence that it wont so
my fears are most likely unfounded. 

However, I do think the other issue of overriding the default method of locale persistence
becoming so much harder in T5.1 is a major issue and does break backwards compatibility. The
aim should be that if the ENCODE_LOCALE_INTO_PATH symbol is set to false, then a custom PersistentLocale
service that worked with T5.0 should work the same with T5.1 so I guess the only sticking
point is the lack of a LocalizationFilter in T5.1 which is enabled when the symbol is false
and allows a custom PersistentLocale service written for T5.0 to work with T5.1 without a
lot of hassle and migration work. 

This should fix the main (objective) part of this issue. As I've said before it will only
really be a minor inconvenience to me personally if you don't resolve this issue - I think
I know what I'm doing and have the ability to cope with this for my own work with Tapestry.

      was (Author: andyb):
    Howard, 

I agree that using cookies to track user's persistent locale is not industry standard, but
it happens to be the method used by the final public release of T5.0 which is the important
point here. The fact that the default method for persisting locales is different is really
only a backwards compatibility issue for anyone who's relying on the generated URLs for some
reason or has issues with locale being in the URLs. (like our site which needs durable and
shareable URLs regardless of a users' locale preference)

To my mind this is a judgement call for you to take as it's quite a subjective aspect of backwards
compatibility, assuming of course that it's easy to override the default behaviour like it
was in T5.0 where you provided an easy way with a public interface. This is where I have the
main (less subjective, more objective) issue...

You are right that the functionality of persisting locale once set is still retained and is
just done using a different method in T5.1, but only for people who haven't implemented their
own PersistentLocale service. If they have then it will break on upgrading to T5.1 because
the service interface (which is public and not internal) is now used in a different way by
the internals - in other words the contract you made for this public interface is broken.
If it were as simple as changing the symbol to false to re-enable the old contract with the
public PersistentLocale service then it wouldn't be so bad, but currently it's non-trivial
for someone to get their custom PersistentLocale services working in T5.1 which is why I consider
backwards compatibility to be broken in this instance.

Regarding your statement "vast majority of users (who were not mucking about in the internals
of Tapestry to customize persistent locales)" - I really don't think that creating and contributing
a custom implementation of a public service interface can be characterized in this way. Especially
by you. I thought that this was what T5 was all about; functional out of the box, but allowing
powerful customization where it just "gets out of the way" to let you do what you need. Have
I got this wrong?

I definitely have mucked about with Tapestry internals and I will have some upgrade pains
due to these modification, but I knew that when I did it and you will not hear a thing from
me about backwards compatibility when I've messed with internals (even if there was no other
way in T5.0 to achieve what I needed to do) but that is not the case here.

I think I can safely assume that you do not consider reverting the default behaviour of T5.1
to cookie locale persistence an option which is fair enough. I don't think much of cookie
persistence myself so I'm certainly not bothered by it, I just hope it doesn't trip up too
many others. That's my only concern, although you seem to have confidence that it wont so
my fears are most likely unfounded. As I've said before it will only really be a minor inconvenience
to me personally if you don't resolve this issue - I think I know what I'm doing and have
the ability to cope with this for my own work with Tapestry.

However, I do think the other issue of overriding the default method of locale persistence
becoming so much harder in T5.1 is a major issue and does break backwards compatibility. The
aim should be that if the ENCODE_LOCALE_INTO_PATH symbol is set to false, then a custom PersistentLocale
service that worked with T5.0 should work the same with T5.1 so I guess the only sticking
point is the lack of a LocalizationFilter in T5.1 which is enabled when the symbol is false
and allows a custom PersistentLocale service written for T5.0 to work with T5.1 without a
lot of hassle and migration work. This should fix the main (objective) part of this issue.
  
> TAP5-422 changes break persistent locale backwards compatibility.
> -----------------------------------------------------------------
>
>                 Key: TAP5-577
>                 URL: https://issues.apache.org/jira/browse/TAP5-577
>             Project: Tapestry 5
>          Issue Type: Bug
>          Components: tapestry-core
>    Affects Versions: 5.1.0.0, 5.1.0.1
>            Reporter: Andy Blower
>            Priority: Critical
>
> I think that the changes made in T5.1 for TAP5-422 break backwards compatibility with
T5.0's locale persistence. In T5.0 it was a simple matter to override the default cookie persistence
by creating a custom implementation of the PersistentLocale service and contributing it to
be used instead of the standard internal T5 implementation.
> The TAP5-422 changes broke backwards compatibility because anyone who's created their
own implementation of PersistentLocale, or just wants the 5.0 cookie persistence behaviour,
would have found that it's a lot more work and involves some heavy changes to Tapestry internals.
Now with the recent changes for TAP5-418 (committed yesterday), the situation had been alleviated
somewhat by allowing the the hard-wired URl locale persistence to be switched off using a
new symbol.
> However, I still think that this breaks backwards compatibility in two ways:
> 1) By changing the default behaviour of locale persistence so that anyone relying on
the locale persistence behaviour of 5.0 will have to make non-trivial changes when they upgrade
to 5.1 to keep the same operation.
> 2) By requiring so much work for anyone wanting to keep the 5.0 cookie persistence behaviour
or define their own custom locale persistence. (In 5.0 it was easy to figure out and implement
a custom locale persistence method)
> From my analysis of the changes made by TAP5-422 & TAP5-418, I think anyone wanting
non-URL based locale persistence will need to do the following when upgrading from 5.0 to
5.1:
> 1) Set the ENCODE_LOCALE_INTO_PATH symbol to false.
> 2) Create an implementation of PersistentLocale and contribute it to the IOC. (copied
from the standard 5.0 code if the old default cookie persistence is desired)
> 3) Create a custom filter written and created to do the same job as the 5.0 LocalizationFilter
and contribute it to the IOC RequestHandler. This filter will need to call the LocalizationSetter
setLocaleFromLocaleName() method instead of the old setThreadLocale() method. 
> My suggested resolution would be to re-instate the 5.0 cookie persistence (LocalizationFilter
& PersistentLocaleImpl) and have the new ENCODE_LOCALE_INTO_PATH symbol default to false
allowing 5.1 to work the same way as 5.0 out of the box. If the symbol is set to true, then
the LocalizationFilter is disabled (not contributed to RequestHandler) and the PersistentLocale
service will need to just store the locale (not set it in a cookie) for later use by LinkSourceImpl.
LocalizationSetterImpl.setLocaleFromLocaleName(String localeName) would also need changing
back to overriding the passed localeName if a persistent one had been set into the PersistentLocale
service. There may by a much better solution than this as I've not spent much time on it,
but I though I should try to be helpful as possible.
> (It should be noted that this is purely a product of my analysis of the 5.1 code, I have
not found the time to actually run T5.1 and test this out - I should be able to do this in
about a week and a half, but I'm currently approaching a major milestone deadline. Hopefully
someone else will find the time to prove or disprove my hypothesis.)

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message