-
Notifications
You must be signed in to change notification settings - Fork 41.4k
Drop a constant from Servlet package in OrderedHiddenHttpMethodFilter #14228
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
Drop a constant from Servlet package in OrderedHiddenHttpMethodFilter #14228
Conversation
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.
Thanks again for the PR. I am not sure about this one, I've added a comment.
@@ -32,8 +31,7 @@ | |||
/** | |||
* The default order is high to ensure the filter is applied before Spring Security. | |||
*/ | |||
public static final int DEFAULT_ORDER = FilterRegistrationBean.REQUEST_WRAPPER_FILTER_MAX_ORDER | |||
- 10000; | |||
public static final int DEFAULT_ORDER = -10000; |
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.
I can understand why you'd want to drop a reference to this class but I am not sure copy/pasting the default (I mean 0
) is something we want to do. Perhaps we need a shared context for this?
WDYT?
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.
@snicoll Thanks for the quick feedback! I thought the same thing but couldn't find a good place to move them. So I considered adding a constant container class like FilterOrders
but with only one member, I wasn't sure it's okay. Any suggestion?
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.
This should be applied to |
I think for 2.0.x we should just drop the reference and hard code 0 but it feels like we're missing something from the code. The constant was originally added in 1.3 (see #3613) but looking again I wonder if we should move it out of I wonder if adding an |
This looks resolved via 3ff20b2, so we can close this PR unless there needs to discuss more on this. |
It might be worth still considering the |
Closing in favor of #14793 |
The constant comes from Servlet package and has a value of
0
, so this PR simply drops the constant.