Skip to content

Turn off useSuffixPatternMatching by default #24576

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

Closed
wants to merge 6 commits into from

Conversation

mikesmithson
Copy link

@mikesmithson mikesmithson commented Feb 23, 2020

Changes for issue #23915.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 23, 2020
@mikesmithson mikesmithson requested review from rstoyanchev and removed request for rstoyanchev February 25, 2020 11:36
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

is this issue something a first-time contributor can submit a PR for? I saw that it was self-assigned back in December and wasn't sure if it was being actively worked on. I have a PR that I can submit if this issue is still available for outside contribution.

The necessary changes are quite small . The tricky part is reviewing the Javadoc in all the relevant classes and making updates, especially with regards to defaults. If you see the classes touched by commits associated with #24179, that will give you an idea.

The reference documentation will also need to be checked and updated as needed.

I see that RequestMappingInfo has a similar property that needs changing too.

There might be more but this should give enough for another draft.

@rstoyanchev rstoyanchev self-assigned this Feb 25, 2020
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) status: waiting-for-feedback We need additional information before we can continue labels Feb 25, 2020
@rstoyanchev rstoyanchev changed the title #23915 - Turn off useSuffixPatternMatching by default Turn off useSuffixPatternMatching by default Feb 25, 2020
@mikesmithson
Copy link
Author

is this issue something a first-time contributor can submit a PR for? I saw that it was self-assigned back in December and wasn't sure if it was being actively worked on. I have a PR that I can submit if this issue is still available for outside contribution.

The necessary changes are quite small . The tricky part is reviewing the Javadoc in all the relevant classes and making updates, especially with regards to defaults. If you see the classes touched by commits associated with #24179, that will give you an idea.

The reference documentation will also need to be checked and updated as needed.

I see that RequestMappingInfo has a similar property that needs changing too.

There might be more but this should give enough for another draft.

@rstoyanchev - OK, am I at least on the right track or do I need to regroup and start from the beginning?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Feb 25, 2020
@rstoyanchev
Copy link
Contributor

Yes that flag does need to change. My intention was to list what else would be needed.

@rstoyanchev rstoyanchev added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Feb 26, 2020
@mikesmithson
Copy link
Author

Yes that flag does need to change. My intention was to list what else would be needed.

@rstoyanchev - OK, I pushed another change. Crossing my fingers...

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Feb 27, 2020
@mikesmithson
Copy link
Author

mikesmithson commented Mar 29, 2020

@rstoyanchev - Is there any interest in this PR still?

@kuberr
Copy link

kuberr commented Apr 14, 2020

@rstoyanchev - Is there any interest in this PR still?

Yes.
We are facing a deprecation warning which I'm not sure if it will be solved by this PR. We currently use this:

pathMatchConfigurer.setUseSuffixPatternMatch(false);
and:
contentNegotiationConfigurer.favorPathExtension(false);

Bu it gives a deprecation warning. I was hoping this PR would allow us to remove this line of code because false would become the new default.

However, when I look at the actual changes in this PR I see that the useSuffixMatch variable is still set to true (alongside the useRegisteredSuffixMatch one). Shouldn't it be set to false by default?

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Apr 15, 2020

Is there any interest in this PR still?

@mikesmithson yes, the issue is scheduled for 5.3 M1 but master is not tracking 5.3.x yet. The PR is incomplete as it stands. See comment by @kuberr above and also the Javadoc and documentation will need a more thorough review and updates.

I'm sorry that I can't give more detailed guidance and there is a reason this is not marked as ideal for contribution. It is tricky with lots of small changes that require a lot of attention. You can see how much work it took to apply deprecations in #24179 and you can also use that for clues if you'd like to take this further.

@mikesmithson
Copy link
Author

@kuberr @rstoyanchev - I would gladly like to take this issue to the finish line if I can get incremental feedback along the way as I am mostly flying blind on a fair amount of this PR. I would probably have to make several small commits along the way and then make adjustments if necessary based on your feedback. Is that a reasonable proposal to you?

@rstoyanchev
Copy link
Contributor

@mikesmithson by all means, any further improvements would be welcome.

@rstoyanchev
Copy link
Contributor

@mikesmithson, I've resolved this with 147b8fb. As I mentioned this really wasn't ideal for contribution. Thanks for the pull request in any case!

@rstoyanchev rstoyanchev closed this May 5, 2020
@rstoyanchev rstoyanchev added status: superseded An issue that has been superseded by another and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged or decided on labels May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: superseded An issue that has been superseded by another
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants