Thanks for comment David, I must admit I only tested the use case and dit not thought about all other implications (setSessionLocale is often called as an event for instance) I think it's better to revert it and to find another solution : there are too much implications. We could create a new method setSessionNewLocale to fix the problem Adrian intended to fix and use it in ofbiz/trunk/framework/common/webcommon/includes/listLocales.ftl Unless someone has a better idea ?
I will revert now and reopen the associated Jira issue with a patch with my suggested idea... Jacques De : "David E Jones" <[EMAIL PROTECTED]> > > This patch breaks stuff. It shouldn't necessarily be reverted, but the change > is incomplete so if the rest of the effort is not finished soon then yes, this patch should be reverted. > > The pattern to find this is fairly simple: change something lots of stuff > might use and only change one place that uses it, and chances are you have introduced a bug. > > In this case checking it was easy. Just search the whole project for > "setSessionLocale" and make sure every one that represents a use of it now uses "newLocale" instead of "locale". > > Doing this search I found this has NOT been done and there ARE resources > referring to the old name. > > When reviewing patches like this there are various acceptable things to do: > > 1. reject the patch describing the problem (more or less as above), and as > the contributor to fix it > > 2. do the review and fix problems and commit it > > The alternative of committing without testing and doing some sanity checking > is not very helpful for the project in general. Yeah, there is such a thing as CTR (Commit-Then-Review), but that is for group review of concepts and to catch things missed, it is NOT meant to be a replacement for testing and impact review to see if it breaks other stuff. > > Thanks for everyone's effort on general contribution review and commit, and > thanks to contributors for contributing. Let's all just keep in mind that our lives in general will be a lot easier if we "First Do No Harm" as described on the committers page: > > http://docs.ofbiz.org/display/OFBADMIN/OFBiz+Committers+Roles+and+Responsibilities > > That page may seem to have a lot of useless crap on it, but please keep in > mind that everything I wrote there was in reaction to a real world problem that has come up! > > Thanks, > -David > > > [EMAIL PROTECTED] wrote: > > Author: jleroux > > Date: Sun Sep 2 14:46:01 2007 > > New Revision: 572172 > > > > URL: http://svn.apache.org/viewvc?rev=572172&view=rev > > Log: > > A patch from Adrian Crum for an issue from Bilgin Ibryam"Changing the > > language in party manager is broken" (https://issues.apache.org/jira/browse/OFBIZ-1223) > > > > Modified: > > ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonEvents.java > > ofbiz/trunk/framework/common/webcommon/includes/listLocales.ftl > > > > Modified: > > ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonEvents.java > > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonEvents.java?rev=572172&r1=572171&r2=572172&view=diff > > ============================================================================== > > --- ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonEvents.java > > (original) > > +++ ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonEvents.java Sun > > Sep 2 14:46:01 2007 > > @@ -166,7 +166,7 @@ > > > > /** Simple event to set the users per-session locale setting */ > > public static String setSessionLocale(HttpServletRequest request, > > HttpServletResponse response) { > > - String localeString = request.getParameter("locale"); > > + String localeString = request.getParameter("newLocale"); > > if (UtilValidate.isNotEmpty(localeString)) { > > UtilHttp.setLocale(request, localeString); > > > > @@ -243,6 +243,7 @@ > > return "success"; > > } > > } > > + > > > > > > > > > > Modified: ofbiz/trunk/framework/common/webcommon/includes/listLocales.ftl > > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/webcommon/includes/listLocales.ftl?rev=572172&r1=572171&r2=572172&view=diff > > ============================================================================== > > --- ofbiz/trunk/framework/common/webcommon/includes/listLocales.ftl > > (original) > > +++ ofbiz/trunk/framework/common/webcommon/includes/listLocales.ftl Sun Sep > > 2 14:46:01 2007 > > @@ -50,7 +50,7 @@ > > </#if> > > <tr <#if altRow>class="alternate-row"</#if>> > > <td lang="${langAttr}" dir="${langDir}"> > > - <a href="<@ofbizUrl>setSessionLocale</@ofbizUrl>?locale=${availableLocale.toString()}">${availableLocale.getDisplayName(availableLocale )}</a> > > + <a href="<@ofbizUrl>setSessionLocale</@ofbizUrl>?newLocale=${availableLocale.toString()}">${availableLocale.getDisplayName(availableLoc ale)}</a> > > </td> > > </tr> > > </#list> > > > >
