[GitHub] [struts] JCgH4164838Gh792C124B5 commented on a change in pull request #405: [WW-5065] Defines a new flag to control appending params

[GitBox <gi...@apache.org>]

JCgH4164838Gh792C124B5 commented on a change in pull request #405:
URL: https://github.com/apache/struts/pull/405#discussion_r414181934



##########
File path: core/src/main/java/com/opensymphony/xwork2/config/impl/AbstractMatcher.java
##########
@@ -36,6 +36,9 @@
  * @since 2.1
  */
 public abstract class AbstractMatcher<E> implements Serializable {
+
+    private static final Logger LOG = LogManager.getLogger(AbstractMatcher.class);

Review comment:
       This looks like it introduces a second private `Logger` reference ?  There is already:
   ```
    private static final Logger log = LogManager.getLogger(AbstractMatcher.class); 
   ```
   Maybe the intention was to rename `log` to `LOG` and update all the logger calls ?

##########
File path: core/src/main/java/org/apache/struts2/StrutsConstants.java
##########
@@ -341,4 +341,7 @@
     public static final String STRUTS_DISALLOW_PROXY_MEMBER_ACCESS = "struts.disallowProxyMemberAccess";
 
     public static final String STRUTS_OGNL_AUTO_GROWTH_COLLECTION_LIMIT = "struts.ognl.autoGrowthCollectionLimit";
+
+    /** See {@link com.opensymphony.xwork2.config.impl.AbstractMatcher#appendNamedParameters */
+    public static final String STRUTS_MATCHER_APPEND_NAMED_PARAMETERS = "";

Review comment:
       Looks like the constant is currently `""` (empty string).  Was that intentional, or should it be changed to:
   ```
   public static final String STRUTS_MATCHER_APPEND_NAMED_PARAMETERS = "struts.matcher.appendNamedParameters";
   ```
   which was the name Lukasz favoured in PR #400.

##########
File path: core/src/main/java/com/opensymphony/xwork2/config/impl/AbstractMatcher.java
##########
@@ -50,10 +53,23 @@
      * <p> The compiled patterns and their associated target objects </p>
      */
     List<Mapping<E>> compiledPatterns = new ArrayList<>();
-    ;
+
+    /**
+     * This flag controls if passed named params should be appended
+     * to the map in {@link #replaceParameters(Map, Map)}
+     * and will be accessible in {@link com.opensymphony.xwork2.config.entities.ResultConfig}.
+     * If set to false, the named parameters won't be appended.
+     *
+     * This behaviour is controlled by {@link org.apache.struts2.StrutsConstants#STRUTS_MATCHER_APPEND_NAMED_PARAMETERS}
+     *
+     * @since 2.5.23
+     * See WW-5065
+     */
+    private final boolean appendNamedParameters;
     
-    public AbstractMatcher(PatternMatcher<?> helper) {
+    public AbstractMatcher(PatternMatcher<?> helper, boolean appendNamedParameters) {

Review comment:
       Because the original method 
   ```
   public AbstractMatcher(PatternMatcher<?> helper)
   ```
   is public, people may have extended this class, which would then break their code unexpectedly with this change.  Maybe it would be a good idea to add keep the old method signature present with something like:
   ```
   public AbstractMatcher(PatternMatcher<?> helper) {
       this(helper, true);  // Set boolean to whatever default setting is decided on
   }
   ```
   That should preserve backward-compatbility.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org