NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[HttpFoundation] Similar locale selection#52986

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:7.1fromSpomky:bugs/incorrect-locale-selection

Apr 5, 2024

Merged

[HttpFoundation] Similar locale selection#52986

fabpot merged 1 commit intosymfony:7.1fromSpomky:bugs/incorrect-locale-selection

Apr 5, 2024

Conversation

Copy link

Contributor

Spomky commentedDec 10, 2023•
edited by fabpot
Loading

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	none
License	MIT

Allow the nearest locale to be selected instead the default one.
Ping@welcoMattic

I noted a non-optimized locale selection. Let say I have the following controller and root route:

#[Route('/', name:'app_root', methods: [Request::METHOD_GET])]publicfunctionindexNoLocale(Request$request):Response{$locale =$request->getPreferredLanguage($this->supportedLocales) ??$this->defaultLocale;return$this->redirectToRoute('app_homepage', ['_locale' =>$locale,    ], Response::HTTP_SEE_OTHER);}

And the following parameters:

$this->supportedLocales =['fr_FR', 'en_US']
$this->defaultLocale ='en_US'

When a user arrives on this page with the headeraccept-language: ja-JP,fr_CA;q=0.7,fr;q=0.5, the default localeen_US is returned.
In this situation, I would expect to usefr_FR as the browser indicates French is a possible choice (fr_CA andfr).

With this change, and if no exact match is found, a similar language is selected. In this example,fr_FR is acceptable and then returned.

carsonbot added Status: Needs Review Bug HttpFoundation labels

Dec 10, 2023

carsonbot added this to the6.4 milestone

Dec 10, 2023

Nyholm reviewed

Dec 11, 2023

View reviewed changes

Copy link

Member

Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you. It looks cool.

Can you also test these:

ja-JP,fr;q=0.5ja-JP,fr_CA;q=0.7

Both of them should go tofr_FR, right?

How about these?

ja-JP,fr_CA;q=0.7,fr;q=0.5,en_US;q=0.3ja-JP,fr;q=0.5,en_US;q=0.3ja-JP,fr_CA;q=0.7,en_US;q=0.3ja-JP,fr_CA;q=0.7,en;q=0.5

Copy link

Member

welcoMattic commentedDec 11, 2023

Maybe for another PR, but shouldn't we add support forq= parameter?https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Language#directives

Copy link

ContributorAuthor

Spomky commentedDec 11, 2023

Many thanks for your comments. Indeed there is something wrong with some casessuch as this one whereen is selected whenen-us is preferred just because of the order of the array values.
I will add use cases and make sureq is understood.

Spomky force-pushed thebugs/incorrect-locale-selection branch from48c8c98 tob1525a3Compare

December 11, 2023 19:19

Spomky requested review fromOskarStark,chalasr,dunglas,jderusse,kbond,lyrixx,welcoMattic,xabbuh andyceruto ascode owners

December 11, 2023 19:19

Spomky force-pushed thebugs/incorrect-locale-selection branch 4 times, most recently fromf8b2126 to4ed69e0Compare

December 11, 2023 19:57

Copy link

ContributorAuthor

Spomky commentedDec 11, 2023

Hi,

I updated the PR and changed the comparison logic:

If an exact match exists, it is returned
Else if a similar locale exists, the corresponding language is returned
If nothing is acceptable, the first supported locale is returned

Please note that theq parameter is already used. Thetest with a reversed order priority returns the expected language.

In addition, the returned locale value is alwaysxx_XX to make it consistant with the value used internally.
Also, it accepts locales such ashi_Latn_IN to be compared.

chalasr removed request forOskarStark,dunglas,jderusse,kbond,lyrixx,welcoMattic,xabbuh andyceruto

December 11, 2023 19:59

Spomky force-pushed thebugs/incorrect-locale-selection branch from10b0a45 to85e3a7fCompare

February 1, 2024 19:45

Copy link

ContributorAuthor

Spomky commentedFeb 1, 2024

Many thanks for the last suggestions.
The PR is now rebased, squashed and ready for other reviewers comments

nicolas-grekas added the Ready label

Mar 29, 2024

nicolas-grekas reviewed

Apr 3, 2024

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

That's an improvement, for 7.1 ;)

src/Symfony/Component/HttpFoundation/Request.php OutdatedShow resolvedHide resolved

src/Symfony/Component/HttpFoundation/Request.phpShow resolvedHide resolved

src/Symfony/Component/HttpFoundation/Request.php OutdatedShow resolvedHide resolved

smnandre reviewed

Apr 3, 2024

View reviewed changes

src/Symfony/Component/HttpFoundation/Request.php OutdatedShow resolvedHide resolved

Spomky force-pushed thebugs/incorrect-locale-selection branch 2 times, most recently from16a03e4 to1d5e821Compare

April 3, 2024 18:34

Spomky changed the base branch from6.4 to7.1

April 3, 2024 18:34

Spomky force-pushed thebugs/incorrect-locale-selection branch from1d5e821 tof67a5d7Compare

April 3, 2024 18:39

dunglas approved these changes

Apr 3, 2024

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Apr 3, 2024

Copy link

Member

stof commentedApr 3, 2024

PHP already has a locale matching algorithm in ext-intl:https://www.php.net/manual/fr/locale.lookup.php (or maybe some of the other methods there)

Would this algorithm help for this PR ? If yes, the proper way to solve it might be to implement them in the polyfill inhttps://github.com/symfony/polyfill/blob/1.x/src/Intl/Icu/Locale.php (where it currently throw an exception saying it is not implemented) and to use it.