-
Notifications
You must be signed in to change notification settings - Fork 813
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
WW-5381 Introduce extension point for CompoundRootAccessor #824
Conversation
61d2619
to
11f4fa3
Compare
11f4fa3
to
aef5d5c
Compare
@Inject | ||
protected void setCompoundRootAccessor(RootAccessor compoundRootAccessor) { | ||
this.compoundRootAccessor = compoundRootAccessor; | ||
OgnlRuntime.setPropertyAccessor(CompoundRoot.class, compoundRootAccessor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unified the previously 3 separate instances of CompoundRootAccessor
(SonarCloud report is comparing against wrong base branch) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just one small comment
<bean type="ognl.ClassResolver" name="com.opensymphony.xwork2.util.CompoundRoot" | ||
<bean type="com.opensymphony.xwork2.ognl.accessor.RootAccessor" name="struts" | ||
class="com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be a breaking change? Maybe somehow mention that ClassResolver
shouldn't be used anymore and implement CompoundRoot
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep very true - looks like our bundled version of Guice is quite old and doesn't support binding beans to multiple interfaces. Although that might not work here either since the Bean name has also changed.
I can create a 2nd bean using the old interface and name to prevent injection failures but obviously this bean won't be the same one used by the OgnlValueStackFactory
- it might be fine depending on what these classes intend to do with the CompoundRootAccessor
bean, and if they really need it to share state with the bean injected into OgnlValueStackFactory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - reinstated the old beans for injection compatibility with any custom beans written for Struts 6.3 or earlier
4ae768e
to
f3c160d
Compare
# Conflicts: # core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
WW-5381
Fairly straightforward