Skip to content

[HttpKernel] Fix default locale is ignored when set_locale_from_accept_language is used #53195

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

Merged
merged 1 commit into from
Dec 29, 2023

Conversation

jkobus
Copy link
Contributor

@jkobus jkobus commented Dec 23, 2023

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #53086
License MIT

Fix as discussed in #53086

@carsonbot carsonbot added this to the 6.4 milestone Dec 23, 2023
@jkobus jkobus changed the title Fix default locale is ignored when set_locale_from_accept_language is used [HttpFoundation][FrameworkBundle] Fix default locale is ignored when set_locale_from_accept_language is used Dec 23, 2023
@smnandre
Copy link
Member

I'm having just a doubt about the second line of the "if"

  $request->attributes->set('_vary_by_language', true);

It is later used by the ResponseListener to send a Vary header.. so i guess we want to send this header regardless of whether the client send its preferences or not (or that coud generate big cache problems)

(sorry, did not think about that when I wrote the example)

@chalasr
Copy link
Member

chalasr commented Dec 23, 2023

Indeed. Nice catch @smnandre

@carsonbot carsonbot changed the title [HttpFoundation][FrameworkBundle] Fix default locale is ignored when set_locale_from_accept_language is used Fix default locale is ignored when set_locale_from_accept_language is used Dec 26, 2023
@carsonbot carsonbot changed the title Fix default locale is ignored when set_locale_from_accept_language is used [HttpKernel] Fix default locale is ignored when set_locale_from_accept_language is used Dec 26, 2023
@jkobus jkobus force-pushed the default-locale-is-ignored-fix branch from 6123971 to 671befa Compare December 27, 2023 09:17
@jkobus
Copy link
Contributor Author

jkobus commented Dec 27, 2023

Thanks, I added a test case for that @smnandre

@fabpot
Copy link
Member

fabpot commented Dec 28, 2023

Isn't something we need to merge in 5.4?

@chalasr
Copy link
Member

chalasr commented Dec 29, 2023

@fabpot yes, good catch.

@chalasr chalasr modified the milestones: 6.4, 5.4 Dec 29, 2023
@nicolas-grekas nicolas-grekas force-pushed the default-locale-is-ignored-fix branch from 671befa to c626b3a Compare December 29, 2023 13:50
@nicolas-grekas
Copy link
Member

Thank you @jkobus.

@nicolas-grekas nicolas-grekas merged commit 386e238 into symfony:5.4 Dec 29, 2023
@jkobus
Copy link
Contributor Author

jkobus commented Dec 29, 2023

Thanks, happy new year for you all 🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants