[
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
