I refactored many parts of the framework. I might have done more refactoring than anyone recently.
However, I never just pushed a commit. I always ask for feedback, I make a patch and I consistently ask for people's feedback, some of that feedback came directly from you Jacques. So for you to call it just refactoring as if this makes it a benign this is surprising to me. I would join Michael in recommending that you revert and start a proper discussion. On Jun 29, 2018 1:29 PM, "Jacques Le Roux" <[email protected]> wrote: Michael It's not a change just a refactorization. I thought it was simple enough to be committed. If we go this way for simple and small changes like here things will be quite slow I also answered in the Jira since you also asked the same there Jacques Le 29/06/2018 à 12:21, Michael Brohl a écrit : > Jacques, > > didn't we just agreed upon a slower process and review from more committers when changing these core aspects of the framework? > > Especially when you change the patch there is no chance for anyone to review before it gets committed {#emotions_dlg.sad} > > Michael > > Am 29.06.18 um 12:03 schrieb [email protected]: >> Author: jleroux >> Date: Fri Jun 29 10:03:22 2018 >> New Revision: 1834662 >> >> URL: http://svn.apache.org/viewvc?rev=1834662&view=rev >> Log: >> Improved: Factorize code logic from ‘ConfigXMLReader’ >> (OFBIZ-10453) >> >> There is a lot of repetitive code in ConfigXMLReader. >> Using a functional interface as a parameter of a generic algorithm avoids those >> repetitions. >> >> Thanks: Mathieu Lirzin >> >> Modified: >> ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java >> >> Modified: ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java >> URL: >> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java?rev=1834662&r1=1834661&r2=1834662&view=diff >> ============================================================================== >> --- ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java (original) >> +++ ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ConfigXMLReader.java Fri Jun 29 10:03:22 2018 >> @@ -32,6 +32,7 @@ import java.util.List; >> import java.util.Map; >> import java.util.Objects; >> import java.util.Set; >> +import java.util.function.Function; >> import java.util.stream.Collectors; >> import javax.servlet.ServletContext; >> @@ -218,120 +219,67 @@ public class ConfigXMLReader { >> } >> } >> - public Map<String, Event> getAfterLoginEventList() throws WebAppConfigurationException { >> - MapContext<String, Event> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getAfterLoginEventList()); >> + private <K, V> Map<K, V> pushIncludes(Function<ControllerConfig, Map<K, V>> f) throws WebAppConfigurationException { >> + MapContext<K, V> res = MapContext.getMapContext(); >> + for (URL include : includes) { >> + res.push(getControllerConfig(include).pushIncludes(f)); >> + } >> + res.push(f.apply(this)); >> + return res; >> + } >> + >> + private String getIncludes(Function<ControllerConfig, String> f) throws WebAppConfigurationException { >> + String val = f.apply(this); >> + if (val != null) { >> + return val; >> + } >> + for (URL include : includes) { >> + String inc = getControllerConfig(include).getIncludes(f); >> + if (inc != null) { >> + return inc; >> + } >> } >> - result.push(afterLoginEventList); >> - return result; >> + return null; >> + } >> + >> + public Map<String, Event> getAfterLoginEventList() throws WebAppConfigurationException { >> + return pushIncludes(ccfg -> ccfg.afterLoginEventList); >> } >> public Map<String, Event> getBeforeLogoutEventList() throws WebAppConfigurationException { >> - MapContext<String, Event> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getBeforeLogoutEventList()); >> - } >> - result.push(beforeLogoutEventList); >> - return result; >> + return pushIncludes(ccfg -> ccfg.beforeLogoutEventList); >> } >> public String getDefaultRequest() throws WebAppConfigurationException { >> - if (defaultRequest != null) { >> - return defaultRequest; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String defaultRequest = controllerConfig.getDefaultRequest(); >> - if (defaultRequest != null) { >> - return defaultRequest; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.defaultRequest); >> } >> public String getErrorpage() throws WebAppConfigurationException { >> - if (errorpage != null) { >> - return errorpage; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String errorpage = controllerConfig.getErrorpage(); >> - if (errorpage != null) { >> - return errorpage; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.errorpage); >> } >> public Map<String, String> getEventHandlerMap() throws WebAppConfigurationException { >> - MapContext<String, String> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getEventHandlerMap()); >> - } >> - result.push(eventHandlerMap); >> - return result; >> + return pushIncludes(ccfg -> ccfg.eventHandlerMap); >> } >> public Map<String, Event> getFirstVisitEventList() throws WebAppConfigurationException { >> - MapContext<String, Event> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getFirstVisitEventList()); >> - } >> - result.push(firstVisitEventList); >> - return result; >> + return pushIncludes(ccfg -> ccfg.firstVisitEventList); >> } >> public String getOwner() throws WebAppConfigurationException { >> - if (owner != null) { >> - return owner; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String owner = controllerConfig.getOwner(); >> - if (owner != null) { >> - return owner; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.owner); >> } >> public Map<String, Event> getPostprocessorEventList() throws WebAppConfigurationException { >> - MapContext<String, Event> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getPostprocessorEventList()); >> - } >> - result.push(postprocessorEventList); >> - return result; >> + return pushIncludes(ccfg -> ccfg.postprocessorEventList); >> } >> public Map<String, Event> getPreprocessorEventList() throws WebAppConfigurationException { >> - MapContext<String, Event> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getPreprocessorEventList()); >> - } >> - result.push(preprocessorEventList); >> - return result; >> + return pushIncludes(ccfg -> ccfg.preprocessorEventList); >> } >> public String getProtectView() throws WebAppConfigurationException { >> - if (protectView != null) { >> - return protectView; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String protectView = controllerConfig.getProtectView(); >> - if (protectView != null) { >> - return protectView; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.protectView); >> } >> // XXX: Temporary wrapper to handle conversion from Map to MultiMap. >> @@ -391,51 +339,19 @@ public class ConfigXMLReader { >> } >> public String getSecurityClass() throws WebAppConfigurationException { >> - if (securityClass != null) { >> - return securityClass; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String securityClass = controllerConfig.getSecurityClass(); >> - if (securityClass != null) { >> - return securityClass; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.securityClass); >> } >> public String getStatusCode() throws WebAppConfigurationException { >> - if (statusCode != null) { >> - return statusCode; >> - } >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - String statusCode = controllerConfig.getStatusCode(); >> - if (statusCode != null) { >> - return statusCode; >> - } >> - } >> - return null; >> + return getIncludes(ccfg -> ccfg.statusCode); >> } >> public Map<String, String> getViewHandlerMap() throws WebAppConfigurationException { >> - MapContext<String, String> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getViewHandlerMap()); >> - } >> - result.push(viewHandlerMap); >> - return result; >> + return pushIncludes(ccfg -> ccfg.viewHandlerMap); >> } >> public Map<String, ViewMap> getViewMapMap() throws WebAppConfigurationException { >> - MapContext<String, ViewMap> result = MapContext.getMapContext(); >> - for (URL includeLocation : includes) { >> - ControllerConfig controllerConfig = getControllerConfig(includeLocation); >> - result.push(controllerConfig.getViewMapMap()); >> - } >> - result.push(viewMapMap); >> - return result; >> + return pushIncludes(ccfg -> ccfg.viewMapMap); >> } >> private void loadGeneralConfig(Element rootElement) { >> >> > >
