[jira] Issue Comment Edited: (TAP5-577) TAP5-422 changes break persistent locale backwards compatibility.

[Ilya Obshadko (JIRA)]

[ 
https://issues.apache.org/jira/browse/TAP5-577?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12747829#action_12747829
 ] 

Ilya Obshadko edited comment on TAP5-577 at 8/26/09 1:20 AM:
-

 The weird thing is what I've done at least half works... when the filter 
 sets the locale, the messages displayed on
 my page are correctly localised even though the thread locale is 
 subsequently set back to the browser default 
 rather than our persistent locale. I have no idea why this is and I honestly 
 haven't the time or inclination any 
 more to look into this problem more so I'm not going to be finding out.

This is because of the following code in LocalizationSetterImpl

public boolean setLocaleFromLocaleName(String localeName)
{
boolean supported = acceptedLocaleNames.contains(localeName);

if (supported)
{
Locale locale = findClosestSupportedLocale(toLocale(localeName));
persistentLocale.set(locale);
threadLocale.setLocale(locale);
}
else
{
Locale requestLocale = request.getLocale();
//  
Locale supportedLocale = findClosestSupportedLocale(requestLocale);
threadLocale.setLocale(supportedLocale);
}

return supported;
}

I found out that during normal request processing this method is being called 
even if there is no persistent locale in the URL. So, when URL is, for example, 
http://localhost:8080/app/index, the methods takes string index as an 
argument; then of course it sets supported to false, and eventually falls back 
to the request locale.

I believe the whole implementation is flawed; LocalizationSetter implementation 
should not guess if it's supplied with locale name or not, the argument must be 
valid locale code (already defined in the list of possible locales in 
configuration) from the very start. So locale code should be checked for 
validity before it's passed to LocalizationSetter.

My solution was just copy-pasting LocalizationSetterImpl; then I've modified 
the code so LocalizationSetter would fall back to 'default' locale (first in 
supported locales list) rather than request locale; then I've overridden 
default LocalizationSetter service. That solved my particular problem (I need 
Russian language version of the website by default).

  was (Author: xfyre):
iThe weird thing is what I've done at least half works... when the filter 
sets the locale, the messages displayed on my page are correctly localised even 
though the thread locale is subsequently set back to the browser default rather 
than our persistent locale. I have no idea why this is and I honestly haven't 
the time or inclination any more to look into this problem more so I'm not 
going to be finding out./i

This is because of the following code in LocalizationSetterImpl

public boolean setLocaleFromLocaleName(String localeName)
{
boolean supported = acceptedLocaleNames.contains(localeName);

if (supported)
{
Locale locale = findClosestSupportedLocale(toLocale(localeName));
persistentLocale.set(locale);
threadLocale.setLocale(locale);
}
else
{
Locale requestLocale = request.getLocale();
//  
Locale supportedLocale = findClosestSupportedLocale(requestLocale);
threadLocale.setLocale(supportedLocale);
}

return supported;
}

I found out that during normal request processing this method is being called 
even if there is no persistent locale in the URL. So, when URL is, for example, 
http://localhost:8080/app/index, the methods takes string index as an 
argument; then of course it sets supported to false, and eventually falls back 
to the request locale.

I believe the whole implementation is flawed; LocalizationSetter implementation 
should not guess if it's supplied with locale name or not, the argument must be 
valid locale code (already defined in the list of possible locales in 
configuration) from the very start. So locale code should be checked for 
validity before it's passed to LocalizationSetter.

My solution was just copy-pasting LocalizationSetterImpl; then I've modified 
the code so LocalizationSetter would fall back to 'default' locale (first in 
supported locales list) rather than request locale. That solved my particular 
problem (I need Russian language version of the website by default).

  
 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

[jira] Issue Comment Edited: (TAP5-577) TAP5-422 changes break persistent locale backwards compatibility.

[Andy Blower (JIRA)]

[ 
https://issues.apache.org/jira/browse/TAP5-577?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=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)