It's not a matter of impact, it's a matter of principle. You don't leave code in a worse state for no valid reason. People are working very hard to refactor the code to decrease the entropy, not increase it.
On Sep 3, 2017 3:45 PM, "Jacques Le Roux" <[email protected]> wrote: Reverted at revision: 1807142 I did not thought that changing from private to "no modifier" there would have a such impact on dev ML It's right that Wai did not give any justifications. Let's see if he will... Jacques Le 03/09/2017 à 11:20, Michael Brohl a écrit : > +1 > > Additionally, I think such modifications should not be made in a hurry. > They should wait for committer/contributor reactions or be (briefly) > discussed before being committed. In this case, the the time between issue > creation and the commit was just a few hours, no chance for objections. > > We had similar discussions in the past - PLEASE be more careful about such > changes. > > Thanks and regards, > > Michael > > > Am 03.09.17 um 10:50 schrieb Taher Alkhateeb: > >> I see two things which happened here: >> 1- a modification on the code base to accommodate one person working on >> their own project >> 2- The modification exposes internals which makes the system more fragile >> for no added value. >> >> I recommend a revert, and I believe the one who committed is the one who >> should revert, not others. Inviting people to "feel free" to revert your >> work does not make much sense and takes away from their time which they >> already spent reviewing your code. >> >> On Sep 3, 2017 10:18 AM, "Jacques Le Roux" <[email protected]> >> wrote: >> >> Hi Paul, >>> >>> That's a good idea, please feel free to enhance or revert >>> >>> Thanks >>> >>> Jacques >>> >>> >>> Le 03/09/2017 à 06:58, Paul Foxworthy a écrit : >>> >>> Hi Jacques, >>>> >>>> These fields and methods will have package scope, but your description >>>> suggests you intended protected. I'm really uncomfortable about fields >>>> with >>>> package scope. Is it really necessary? Could they be protected? >>>> >>>> In the case of ignoreAttrs, the array should remain private. The loop >>>> in jsonResponseFromRequestAttributes that uses the array should be >>>> made a >>>> method in its own right, with a name such as removeSensitiveAttributes. >>>> That method could have higher visibility such as protected. The general >>>> principle is "don't ask for data, ask for help" ( >>>> https://martinfowler.com/bliki/TellDontAsk.html). >>>> >>>> Cheers >>>> >>>> Paul Foxworthy >>>> >>>> >>>> On 3 September 2017 at 04:45, Jacques Le Roux < >>>> [email protected]> >>>> wrote: >>>> >>>> I think Wai asked for that because he needs it for a custom project. >>>> It's >>>> >>>>> sill reasonably encapsulated. >>>>> >>>>> You can revert if you want >>>>> >>>>> Jacques >>>>> >>>>> >>>>> >>>>> Le 02/09/2017 à 20:06, Taher Alkhateeb a écrit : >>>>> >>>>> Do we have any derived classes that require exposing these interfaces? >>>>> >>>>>> I think it's not a good design to break encapsulation just because of >>>>>> things we "might" need. >>>>>> >>>>>> On Sat, Sep 2, 2017 at 3:25 PM, <[email protected]> wrote: >>>>>> >>>>>> Author: jleroux >>>>>> >>>>>>> Date: Sat Sep 2 12:25:13 2017 >>>>>>> New Revision: 1807045 >>>>>>> >>>>>>> URL: http://svn.apache.org/viewvc?rev=1807045&view=rev >>>>>>> Log: >>>>>>> Improved: CommonEvents improvements >>>>>>> (OFBIZ-9673) >>>>>>> >>>>>>> Replaces private to protected so that CommonEvents.java is more >>>>>>> usable >>>>>>> to >>>>>>> derived classes. >>>>>>> >>>>>>> Thanks: Wai >>>>>>> >>>>>>> Modified: >>>>>>> ofbiz/ofbiz-framework/trunk/framework/common/src/main/java/ >>>>>>> org/apache/ofbiz/common/CommonEvents.java >>>>>>> >>>>>>> Modified: ofbiz/ofbiz-framework/trunk/fr >>>>>>> amework/common/src/main/java/ >>>>>>> org/apache/ofbiz/common/CommonEvents.java >>>>>>> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra >>>>>>> mework/common/src/main/java/org/apache/ofbiz/common/Common >>>>>>> Events.java?rev=1807045&r1=1807044&r2=1807045&view=diff >>>>>>> ============================================================ >>>>>>> ================== >>>>>>> --- ofbiz/ofbiz-framework/trunk/framework/common/src/main/java/ >>>>>>> org/apache/ofbiz/common/CommonEvents.java (original) >>>>>>> +++ ofbiz/ofbiz-framework/trunk/framework/common/src/main/java/ >>>>>>> org/apache/ofbiz/common/CommonEvents.java Sat Sep 2 12:25:13 2017 >>>>>>> @@ -68,7 +68,7 @@ public class CommonEvents { >>>>>>> >>>>>>> public static final String module = >>>>>>> CommonEvents.class.getName(); >>>>>>> >>>>>>> - private static final String[] ignoreAttrs = new String[] { // >>>>>>> Attributes removed for security reason; _ERROR_MESSAGE_ is kept >>>>>>> + static final String[] ignoreAttrs = new String[] { // Attributes >>>>>>> removed for security reason; _ERROR_MESSAGE_ is kept >>>>>>> "javax.servlet.request.key_size", >>>>>>> "_CONTEXT_ROOT_", >>>>>>> "_FORWARDED_FROM_SERVLET_", >>>>>>> @@ -82,7 +82,7 @@ public class CommonEvents { >>>>>>> "thisRequestUri" >>>>>>> }; >>>>>>> >>>>>>> - private static final UtilCache<String, Map<String, String>> >>>>>>> appletSessions = UtilCache.createUtilCache("AppletSessions", 0, >>>>>>> 600000, >>>>>>> true); >>>>>>> + static final UtilCache<String, Map<String, String>> >>>>>>> appletSessions >>>>>>> = UtilCache.createUtilCache("AppletSessions", 0, 600000, true); >>>>>>> >>>>>>> public static String checkAppletRequest(HttpServletRequest >>>>>>> request, HttpServletResponse response) { >>>>>>> Delegator delegator = (Delegator) >>>>>>> request.getAttribute("delegator"); >>>>>>> @@ -309,7 +309,7 @@ public class CommonEvents { >>>>>>> return "success"; >>>>>>> } >>>>>>> >>>>>>> - private static void writeJSONtoResponse(JSON json, >>>>>>> HttpServletRequest request, HttpServletResponse response) throws >>>>>>> UnsupportedEncodingException { >>>>>>> + static void writeJSONtoResponse(JSON json, HttpServletRequest >>>>>>> request, HttpServletResponse response) throws >>>>>>> UnsupportedEncodingException { >>>>>>> String jsonStr = json.toString(); >>>>>>> if (jsonStr == null) { >>>>>>> Debug.logError("JSON Object was empty; fatal error!", >>>>>>> module); >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> > >
