tapestry-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ilya Obshadko (JIRA)" <j...@apache.org>
Subject [jira] Issue Comment Edited: (TAP5-577) TAP5-422 changes break persistent locale backwards compatibility.
Date Wed, 26 Aug 2009 08:21:59 GMT

    [ https://issues.apache.org/jira/browse/TAP5-577?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=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):
    <i>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.</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
>    Affects Versions: 5.1.0.0, 5.1.0.1
>            Reporter: Andy Blower
>            Priority: Critical
>         Attachments: T5.1persistentLocale.txt
>
>
> 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