Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor follow-up changes to PR #371 #378

Merged

Conversation

JCgH4164838Gh792C124B5
Copy link
Contributor

Minor follow-up changes to PR #371

  • added some additional exclusions in struts-default.xml.
  • added log warning that specifies the value of maxLength involved if applyExpressionMaxLength(maxLength) fails.
  • added null guards to two handleOgnlException() methods that could result in an NPE with WW-5041 Upgrade to OGNL 3.1.26 and adapt to its new features #371 changes (a null OgnlException parameter was permissible previously, correct or not).

- added some additional exclusions in struts-default.xml.
- added log warning that specifies the value of maxLength involved if
  applyExpressionMaxLength(maxLength) fails.
- added null guards to two handleOgnlException() methods that could
  result in an NPE with apache#371 changes (a null OgnlException parameter
  was permissible previously, correct or not).
@JCgH4164838Gh792C124B5
Copy link
Contributor Author

Hello Apache Struts Team.

This is a follow-up PR based on earlier comments in a PR#371 review. The changes are straightforward and should be very low risk. The build succeeds locally and a quick run through the showcase application didn't reveal any failures either.

@lukaszlenart , according to the developer list you are considering a Struts 2.5.21 release sometime soon. Could you please review this PR and consider it for inclusion in 2.5.21 ?

@coveralls
Copy link

coveralls commented Nov 2, 2019

Coverage Status

Coverage increased (+0.005%) to 47.069% when pulling dd6d206 on JCgH4164838Gh792C124B5:local_25x_CfgChg1 into 13cfba8 on apache:struts-2-5-x.

@JCgH4164838Gh792C124B5
Copy link
Contributor Author

Coveralls claims that coverage diminished in applyExpressionMaxLength(maxLength).
That isn't really true though. To make it happy I will try to add a simple unit test.

- added unit test (hoping to make coveralls happy).
@yasserzamani yasserzamani self-requested a review November 3, 2019 09:09
Copy link
Member

@yasserzamani yasserzamani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@lukaszlenart
Copy link
Member

LGTM 👍

@lukaszlenart lukaszlenart merged commit 1348971 into apache:struts-2-5-x Nov 4, 2019
@JCgH4164838Gh792C124B5 JCgH4164838Gh792C124B5 deleted the local_25x_CfgChg1 branch November 30, 2019 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants