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/framework/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); >>>>> >>>>> >>>>> >>>>> >> >
