[jira] Commented: (STR-2897) Rework Locale Resolution

["Niall Pemberton (JIRA)" <ji...@apache.org>]

    [ http://issues.apache.org/struts/browse/STR-2897?page=comments#action_38878 ] 
            
Niall Pemberton commented on STR-2897:
--------------------------------------

Current Struts 1 Scenario
=========================

I think its worth laying out currently what Struts1 provides (since I had to look it up to remind myself the details)

1) Setting the ControllerConfig's "locale" property to "true" causes the SelectLocale command(1.3) or RequestProcessor(1.2) to store the Locale from the HttpServletRequest under key Globals.LOCALE_KEY in the session (if a value isn't already there).

2) Users can specifically set the Locale (store it under Globals.LOCALE_KEY in the session) using the Action's setLocale(HttpServletRequest) convenience method.

3) The Struts Taglibs, ValidatorForm and Action access (either directly or indirectly) the stored Locale via the RequestUtils.getUserLocale(HttpServletRequest, localeKey) method (not sure what tiles does, somthing slightly different I think).

TODO: Look at what current tiles and Standalone Tiles do?


Features of the Enhancement
===========================
The attached patch is pretty big and it would be easier if the aims of the enhancement are clearly laid out - looking at the patch there seems to be two aims:

1) Provide a plugable mechanism for storing and resolving the Locale - on an application wide basis:
   - Request Header (read-only)
   - A fixed specified default locale (read-only)
   - Stored in the Session (as per current functionality)
   - Stored in a Cookie

2) Provide access to the Locale throughout the Web Application via a ThreadLocale variable

It would be nice if the "motivation" for this feature was explained. From memory one motivation was to remove the need to have a "Session" to store the Locale (hence the Cookie functionality).


What Paul's Patch Does
======================

(summary of the relevant parts IMO)

1) New "Resolver" type
 - ActionLocaleResolver - interface with methods to set and retrieve the Locale
 - 4 ActionLocaleResolver implementations (request header, fixed, Session and Cookie)

2) Resolver Configuration
 - new "localeResolverClass" attribute on the ControllerConfig to specify the ActionLocaleResolver implementation to use (on an application wide basis).
 - The resolver is instantiated and initalized in each RequestProcessor

3) ActionContext - getLocaleResolver() method added

4) ComposableRequestProcessor(1.3) - store the "ActionLocaleResolver" in the ActionContext

5) SelectLocale(1.3) Command
 - Retireve the "ActionLocaleResolver" from the HttpServletRequest
 - Store the "ActionLocaleResolver" in a ThreadLocale variable
 - Store the "Locale" returned by the "ActionLocaleResolver" in the ActionContext

6) RequestProcessor(1.2)
 - init() method: cache the "ActionLocaleResolver"
 - processLocale() method:
    - store the "ActionLocaleResolver" in the HttpServletRequest
    - store the "ActionLocaleResolver" in a ThreadLocale variable

7) CookieGenerator - new class for managing http Cookies

8) LocaleUtils - utility class for Locale, includes providing getLocale(HttpServletRequest) method (to replace RequestUtils((HttpServletRequest, localeKey) method?)


Comments on the Feature
=======================
Having different mechanisms for resolving the Locale is a good idea and the four implementations provided offer a good standard feature set.

One area that might be worth consideration is the limitation of having one strategy (i.e. resolver) for the whole application. Especially since the Cookie implementation is dependant on the user configuring to allow Cookies. I guess that this could be resolved by someone creating a  "Cookie if allowed, otherwise Session" implementation.

I'm not convinced that we need to store the locale resolver in three places (Request, ActionContext and ThreadLocale) seems like making this overly complex if Struts internally is only going to ever retrieve it from one place (I assume Taglibs etc are going to get it from the Request, but since their changes are not included in the patch, this is a guess). I would say lets keep it simple for now and store it in the Request only (or request and ActionContext) - any user wanting it in a ThreadLocale can easily add a Command to do that.

Comments on the Patch
=====================

Some general observations:

- patches which include your local path are a pain to review
  (I had to remove the "C:/Dev/workspace/struts-" prefix to apply the patch and look at it)
- patch should stick to the coding convention of using four spaces to indent
- quite a few lines are flagged as changed that are not really 
   (for example, just because the indentation was altered).
  This makes reviewing the patch a pain - since it adds additional noise to the diffs.


1) ActionServlet - maybe these changes are all unecessary (or need to be different) - see following comments.

2) RequestProcessor - adding an additional init() method is completely unnecessary. If the ActionLocaleResolver is instantiated in the RequestProcessors's existing init() method, rather than the ActionServlet. As a general principle we should not change the API, deprecating existing methods, when it can be done without inflicting any pain on the user.

3) I think theres a bug in the ComposableRequestProcessor/SelectLocale implementation - ComposableRequestProcessor is storing the "ActionLocaleResolver" in the ActionContext but SelectLocale is then try to retrieve it from the HttpServletRequest (where it hasn't been stored) and also store it in the ActionContext.

All this is unnecessarily complex anyway - why not just instantiate the locale resolver in the ActionServlet and store it in application scope? The RequestProcessor(1.2)/Command(1.3) could just then retrieve it from application scope and store it in the request and ActionContext. I also don't think we should change the existing AbatractSelectLocale/SelectLocale commands - lets just create new Command(s) for this if they're needed. That way anyone depending on the existing Command's behaviour can carry on using them - thats one of the beauty of the Command feature - you can configure different flavours. I also think that RequestUtils (or its LocaleUtils replacement) should support both the old and new feature - i.e. look for a locale resolver and use that if its there - otherwise if theres a session and it has the Locale, use that. Maybe thats a way of providing Cookie locale storage with Session as a backup?

4) ActionLocaleContext - this class I just don't get at all. Why have this as an extra layer rather than just putting the actual "ActionLocaleResolver" directly in the ThreadLocale variable? Having said that, as I said before I'd rather leave the ThreadLocale variable out if were not actually going to reference it in Struts internally.


Generally looks like a good thing to do, but I think the actual implementation needs some more work before its ready to implement.


> Rework Locale Resolution
> ------------------------
>
>                 Key: STR-2897
>                 URL: http://issues.apache.org/struts/browse/STR-2897
>             Project: Struts 1
>          Issue Type: Improvement
>          Components: Core, Taglibs
>    Affects Versions: 1.3.5
>            Reporter: Ted Husted
>         Assigned To: Paul Benedict
>             Fix For: 1.3.7
>
>         Attachments: str-2897-patch.txt
>
>
> Struts 1 locale resolution could be improvied to work more like Cantoo [http://messages.cintoo.org/] and Struts 2. 
> Ideally, we should be able to share locale resolution between s1, s2, and Spring, in the same application. 
> For background see, 
>  WW/SAF2 i18n & Cintoo Messages 
> * http://www.mail-archive.com/dev%40struts.apache.org/msg23270.html 
> Locale resolver 
> * http://www.mail-archive.com/dev%40struts.apache.org/msg23273.html

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: http://issues.apache.org/struts/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira