Skip to content

Refactor AcceptHeaderLocaleResolver to implement LocaleContextResolver #29239

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 3 commits into from

Conversation

vpavic
Copy link
Contributor

@vpavic vpavic commented Sep 30, 2022

This PR wraps up efforts to improve org.springframework.web.servlet.LocaleResolver hierarchy started in:

The core change proposed here (first commit) is to refactor AcceptHeaderLocaleResolver to implement LocaleContextResolver, which makes the all locale resolvers provided by Spring MVC LocaleContextResolver based. It also aligns Spring MVC with Spring WebFlux, where all resolvers work with LocaleContext rather than plain Locale.

The two subsequent commits are optional, but could be helpful as they prepare ground to simplifying LocaleResolver hierarchy in the future:

  • second commit deprecates AbstractLocaleResolver in favor of AbstractLocaleContextResolver, as nothing is using AbstractLocaleResolver after the first commit
  • third commit is a bit more substantial as it deprecates org.springframework.web.servlet.LocaleResolver and reworks all the references to operate against LocaleContextResolver - while technically a breaking change, this only impacts those with custom LocaleResolver that don't implement LocaleContextResolver

At present, AcceptHeaderLocaleResolver is the only LocaleResolver
implementation that doesn't also implement LocaleContextResolver. This
is also not aligned with WebFlux, where the entire resolver hierarchy
works with LocaleContext.

This commit updates AcceptHeaderLocaleResolver to implement
LocaleContextResolver.
This commit deprecates AbstractLocaleResolver in favor of
AbstractLocaleContextResolver.
This commit deprecates LocaleResolver and updates all references to use
LocaleContextResolver instead.
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 30, 2022
@rstoyanchev rstoyanchev added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jan 24, 2023
@jhoeller
Copy link
Contributor

While I see the point of the PR, I am not inclined to proceed with phasing out LocaleResolver which has been around for 20 years. AcceptHeaderLocaleResolver implementing LocaleContextResolver does not provide any user-level benefits on its own (since the accept-header strategy is only capable of providing a plain locale to begin with). From my perspective, this would really have go along with a deprecation of LocaleResolver as per your third commit, and that's what I try not to inflict on the two-decade-long usage this has in configuration points, customizations/decorations, and also in testing scenarios.

Point taken that WebFlux has full-scale LocaleContext resolution. However, since that is not the only difference that shows MVC's age over WebFlux, that level of SPI style alignment between MVC and WebFlux is not a goal per se... as long as the overall capabilities are equivalent.

@jhoeller jhoeller closed this Nov 23, 2023
@jhoeller jhoeller added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 23, 2023
@vpavic vpavic deleted the improve-locale-resolvers branch April 1, 2024 11:46
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: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants