Tapestry
  1. Tapestry
  2. TAPESTRY-1997

PersistentLocale is lower-casing locales

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 5.0.6
    • Fix Version/s: 5.0.14
    • Component/s: tapestry-core
    • Labels:
      None

      Description

      An issue affecting localization: PersistentLocale is converting locales from mixed case to all lower case, which is useless for formatting. For example, if page 1 sets the locale like this:

      @Inject
      private PersistentLocale _persistentLocaleService;

      Locale locale = Locale.UK;
      _persistentLocaleService.set(locale);
      System.out.println("locale is " + locale + " - " + locale.getDisplayName());

      then this is what prints:

      locale is en_GB - English (United Kingdom)

      But when I'm in page 2 I get the locale and find it has mutated...

      Locale locale = _persistentLocaleService.get();
      System.out.println("locale is " + locale + " - " + locale.getDisplayName());

      ...this is what prints:

      locale is en_gb - en_gb

      This mutated locale in page 2 is useless for formatting. Code like the following produces default-styling instead of the styling for en_GB:

      _myDateFormat = DateFormat.getDateInstance(DateFormat.LONG, locale);
      System.out.println(_myDateFormat.format(new Date()));

      It seems this has also adversely affected how supported-locales are declared (maybe in previous releases only). See http://thread.gmane.org/gmane.comp.java.tapestry.user/56526/focus=56527

        Activity

        Hide
        Howard M. Lewis Ship added a comment -

        I can't see where this might be happening. It may be an issue with how the HttpServletRequest object reports the Locale.

        The code doesn't seem to do anything special with the case of the locale:

        public class PersistentLocaleImpl implements PersistentLocale
        {
        /**

        • Name of the cookie written to the client web browser to identify the locale.
          */
          private static final String LOCALE_COOKIE_NAME = "org.apache.tapestry.locale";

        private final Cookies _cookieSource;

        public PersistentLocaleImpl(Cookies cookieSource)

        { _cookieSource = cookieSource; }

        public void set(Locale locale)

        { _cookieSource.writeCookieValue(LOCALE_COOKIE_NAME, locale.toString()); }

        public Locale get()

        { String localeCookieValue = getCookieValue(); return localeCookieValue != null ? new Locale(localeCookieValue) : null; }

        private String getCookieValue()

        { return _cookieSource.readCookieValue(LOCALE_COOKIE_NAME); }

        public boolean isSet()

        { return getCookieValue() != null; }

        }

        Show
        Howard M. Lewis Ship added a comment - I can't see where this might be happening. It may be an issue with how the HttpServletRequest object reports the Locale. The code doesn't seem to do anything special with the case of the locale: public class PersistentLocaleImpl implements PersistentLocale { /** Name of the cookie written to the client web browser to identify the locale. */ private static final String LOCALE_COOKIE_NAME = "org.apache.tapestry.locale"; private final Cookies _cookieSource; public PersistentLocaleImpl(Cookies cookieSource) { _cookieSource = cookieSource; } public void set(Locale locale) { _cookieSource.writeCookieValue(LOCALE_COOKIE_NAME, locale.toString()); } public Locale get() { String localeCookieValue = getCookieValue(); return localeCookieValue != null ? new Locale(localeCookieValue) : null; } private String getCookieValue() { return _cookieSource.readCookieValue(LOCALE_COOKIE_NAME); } public boolean isSet() { return getCookieValue() != null; } }
        Hide
        Geoff Callender added a comment -

        It appears there is no problem as long as you use the following guidelines:

        1. Use lower-case locale names for supported-locales, eg. in AppModule:

        public static void contributeApplicationDefaults(MappedConfiguration<String, String> configuration)

        { configuration.add("tapestry.supported-locales", "en_gb,en_us,fr"); }

        2. In the page class, get the current locale like this:

        @Inject
        private Locale _locale;

        instead of like this:

        @Inject
        private PersistentLocale _persistentLocaleService;

        Locale locale = _persistentLocaleService.get();

        When guideline 1 is ignored, then these 4 statements all seem to select the first English supported locale:

        _persistentLocaleService.set(new Locale("en_GB");
        _persistentLocaleService.set(new Locale("en_gb");
        _persistentLocaleService.set(new Locale("en_US");
        _persistentLocaleService.set(new Locale("en_us");

        When it IS followed, then all is well, although this makes me uneasy because I'm not sure that "en_gb" and "en_us" are actually valid locales.

        Guideline 2 returns a _locale that is correctly mixed case, eg. "en_GB" or "en_US", whereas _persistentLocaleService.get() does not - it returns a lower-cased locale, eg. "en_gb" or "en_us" which is useless for formatting things correctly (see original comments in this issue).

        These guidelines are OK as a work-around, but my vote is that the handling of supported-locales should be changed. Requiring lower-case is confusing and, I suspect, not even correct.

        Show
        Geoff Callender added a comment - It appears there is no problem as long as you use the following guidelines: 1. Use lower-case locale names for supported-locales, eg. in AppModule: public static void contributeApplicationDefaults(MappedConfiguration<String, String> configuration) { configuration.add("tapestry.supported-locales", "en_gb,en_us,fr"); } 2. In the page class, get the current locale like this: @Inject private Locale _locale; instead of like this: @Inject private PersistentLocale _persistentLocaleService; Locale locale = _persistentLocaleService.get(); When guideline 1 is ignored, then these 4 statements all seem to select the first English supported locale: _persistentLocaleService.set(new Locale("en_GB"); _persistentLocaleService.set(new Locale("en_gb"); _persistentLocaleService.set(new Locale("en_US"); _persistentLocaleService.set(new Locale("en_us"); When it IS followed, then all is well, although this makes me uneasy because I'm not sure that "en_gb" and "en_us" are actually valid locales. Guideline 2 returns a _locale that is correctly mixed case, eg. "en_GB" or "en_US", whereas _persistentLocaleService.get() does not - it returns a lower-cased locale, eg. "en_gb" or "en_us" which is useless for formatting things correctly (see original comments in this issue). These guidelines are OK as a work-around, but my vote is that the handling of supported-locales should be changed. Requiring lower-case is confusing and, I suspect, not even correct.
        Hide
        Geoff Callender added a comment -

        I've just confirmed that country codes should not be lower case. From the javadoc for Locale (http://java.sun.com/j2se/1.5.0/docs/api/index.html) regarding the constructors for Locale:

        "The country argument is a valid ISO Country Code. These codes are the upper-case, two-letter codes as defined by ISO-3166. You can find a full list of these codes at a number of sites, such as: http://www.iso.ch/iso/en/prods-services/iso3166ma/02iso-3166-code-lists/list-en1.html"

        Show
        Geoff Callender added a comment - I've just confirmed that country codes should not be lower case. From the javadoc for Locale ( http://java.sun.com/j2se/1.5.0/docs/api/index.html ) regarding the constructors for Locale: "The country argument is a valid ISO Country Code. These codes are the upper-case, two-letter codes as defined by ISO-3166. You can find a full list of these codes at a number of sites, such as: http://www.iso.ch/iso/en/prods-services/iso3166ma/02iso-3166-code-lists/list-en1.html "
        Hide
        Howard M. Lewis Ship added a comment -

        I still can't find anything that lower-cases locale names.

        I've changed my app to use support a locale of "en_US" and it seems to work fine.

        Show
        Howard M. Lewis Ship added a comment - I still can't find anything that lower-cases locale names. I've changed my app to use support a locale of "en_US" and it seems to work fine.
        Hide
        Andy Blower added a comment -

        I decided to take a look into this (just trying to do my bit.. and I think I've found what the problem is. The problem is not with the list of supported codes or storing the current locale in the cookie, it's incorrect use of a Locale constructor in PersistentLocaleImpl line 45 when retrieving the current locale:

        return localeCookieValue != null ? new Locale(localeCookieValue) : null;

        This is fine if localeCookieValue is just a language code, but if it's a language and country (and/or variant) like "en_GB" then the Locale object created contains a language code of "en_gb" (lowercased in the Locale class as language codes should be lc) and no country code. To construct a Locale with language and country codes, you need to separate them and use the two (or three) string constructor. Personally I don't bother with this and simply use the LocaleUtils class from the commons-lang.jar like this:

        LocaleUtils.toLocale(str)

        So line 45 of PersistantLocaleImpl could simply become:

        return localeCookieValue != null ? LocaleUtils.toLocale(localeCookieValue) : null;

        There may be other changes to T5 code required as a consequence of this change, I don't have enough familiarity with the codebase yet to know. Adding the commons lang dependency is your choice, but it's so generally useful I'd be surprised if it wasn't used in most webapps anyway - I know I've used it in every webapp I've developed for several years now.

        Show
        Andy Blower added a comment - I decided to take a look into this (just trying to do my bit.. and I think I've found what the problem is. The problem is not with the list of supported codes or storing the current locale in the cookie, it's incorrect use of a Locale constructor in PersistentLocaleImpl line 45 when retrieving the current locale: return localeCookieValue != null ? new Locale(localeCookieValue) : null; This is fine if localeCookieValue is just a language code, but if it's a language and country (and/or variant) like "en_GB" then the Locale object created contains a language code of "en_gb" (lowercased in the Locale class as language codes should be lc) and no country code. To construct a Locale with language and country codes, you need to separate them and use the two (or three) string constructor. Personally I don't bother with this and simply use the LocaleUtils class from the commons-lang.jar like this: LocaleUtils.toLocale(str) So line 45 of PersistantLocaleImpl could simply become: return localeCookieValue != null ? LocaleUtils.toLocale(localeCookieValue) : null; There may be other changes to T5 code required as a consequence of this change, I don't have enough familiarity with the codebase yet to know. Adding the commons lang dependency is your choice, but it's so generally useful I'd be surprised if it wasn't used in most webapps anyway - I know I've used it in every webapp I've developed for several years now.
        Hide
        Howard M. Lewis Ship added a comment -

        Great detective work!

        About commons-lang: I prefer to avoid dependencies where possible, to help avoid versioning hell.

        Show
        Howard M. Lewis Ship added a comment - Great detective work! About commons-lang: I prefer to avoid dependencies where possible, to help avoid versioning hell.
        Hide
        Andy Blower added a comment -

        Thanks.

        Regarding dependencies, T5 already has dependencies on commons-codec, commons-io and commons-upload otherwise I wouldn't have suggested it. Still, you can always follow their code and do the same in T5 somewhere. I've tested the fix out using LocaleUtils and it appears to work well. I'm just trying out the localisation features for the first time though so I may well have missed something.

        Show
        Andy Blower added a comment - Thanks. Regarding dependencies, T5 already has dependencies on commons-codec, commons-io and commons-upload otherwise I wouldn't have suggested it. Still, you can always follow their code and do the same in T5 somewhere. I've tested the fix out using LocaleUtils and it appears to work well. I'm just trying out the localisation features for the first time though so I may well have missed something.
        Hide
        Howard M. Lewis Ship added a comment -

        Actually, the other commons- code is only brought in if you have tapestry-upload.

        Show
        Howard M. Lewis Ship added a comment - Actually, the other commons- code is only brought in if you have tapestry-upload.
        Hide
        Howard M. Lewis Ship added a comment -

        Thanks again for the great detective work.

        Show
        Howard M. Lewis Ship added a comment - Thanks again for the great detective work.

          People

          • Assignee:
            Howard M. Lewis Ship
            Reporter:
            Geoff Callender
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development