You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by GitBox <gi...@apache.org> on 2020/04/22 06:01:00 UTC
[GitHub] [struts] lukaszlenart opened a new pull request #405: [WW-5065] Defines a new flag to control appending params
lukaszlenart opened a new pull request #405:
URL: https://github.com/apache/struts/pull/405
Follow up on https://github.com/apache/struts/pull/400
Refs [WW-5065](https://issues.apache.org/jira/browse/WW-5065)
----------------------------------------------------------------
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
[GitHub] [struts] JCgH4164838Gh792C124B5 commented on a change in pull request #405: [WW-5065] Defines a new flag to control appending params
Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on a change in pull request #405:
URL: https://github.com/apache/struts/pull/405#discussion_r414185407
##########
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 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
[GitHub] [struts] lukaszlenart commented on a change in pull request #405: [WW-5065] Defines a new flag to control appending params
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on a change in pull request #405:
URL: https://github.com/apache/struts/pull/405#discussion_r414359429
##########
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:
Right, done!
----------------------------------------------------------------
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
[GitHub] [struts] coveralls edited a comment on pull request #405: [WW-5065] Defines a new flag to control appending params
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #405:
URL: https://github.com/apache/struts/pull/405#issuecomment-617625161
[](https://coveralls.io/builds/30308885)
Coverage increased (+0.01%) to 47.084% when pulling **6e1d2add0729a629412f0e4a407f0b02461b04a0 on WW-5065-append-or-not** into **5c82f0246ef33584823e357e8b067979958e3a51 on struts-2-5-x**.
----------------------------------------------------------------
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
[GitHub] [struts] lukaszlenart commented on a change in pull request #405: [WW-5065] Defines a new flag to control appending params
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on a change in pull request #405:
URL: https://github.com/apache/struts/pull/405#discussion_r414359578
##########
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:
Good catch, fixed!
##########
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:
Right, fixed :)
----------------------------------------------------------------
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
[GitHub] [struts] JCgH4164838Gh792C124B5 commented on a change in pull request #405: [WW-5065] Defines a new flag to control appending params
Posted by 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
[GitHub] [struts] atkaiser commented on issue #405: [WW-5065] Defines a new flag to control appending params
Posted by GitBox <gi...@apache.org>.
atkaiser commented on issue #405:
URL: https://github.com/apache/struts/pull/405#issuecomment-618022537
LGTM 👍
----------------------------------------------------------------
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
[GitHub] [struts] coveralls edited a comment on issue #405: [WW-5065] Defines a new flag to control appending params
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on issue #405:
URL: https://github.com/apache/struts/pull/405#issuecomment-617625161
[](https://coveralls.io/builds/30277905)
Coverage increased (+0.02%) to 47.086% when pulling **30b43044a31e2dac6e1364cbb7388d860c4a5259 on WW-5065-append-or-not** into **5c82f0246ef33584823e357e8b067979958e3a51 on struts-2-5-x**.
----------------------------------------------------------------
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
[GitHub] [struts] coveralls commented on issue #405: [WW-5065] Defines a new flag to control appending params
Posted by GitBox <gi...@apache.org>.
coveralls commented on issue #405:
URL: https://github.com/apache/struts/pull/405#issuecomment-617625161
[](https://coveralls.io/builds/30251043)
Coverage increased (+0.02%) to 47.086% when pulling **c2765e5236dd99765851e90677f3bf4e53c7b5a7 on WW-5065-append-or-not** into **5c82f0246ef33584823e357e8b067979958e3a51 on struts-2-5-x**.
----------------------------------------------------------------
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