Skip to content
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

[HttpFoundation] Similar locale selection #52986

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

Spomky
Copy link
Contributor

@Spomky Spomky commented Dec 10, 2023

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])]
public function indexNoLocale(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 header accept-language: ja-JP,fr_CA;q=0.7,fr;q=0.5, the default locale en_US is returned.
In this situation, I would expect to use fr_FR as the browser indicates French is a possible choice (fr_CA and fr).

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.

Copy link
Member

@Nyholm 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.5
ja-JP,fr_CA;q=0.7

Both of them should go to fr_FR, right?


How about these?

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

@welcoMattic
Copy link
Member

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

@Spomky
Copy link
Contributor Author

Spomky commented Dec 11, 2023

Many thanks for your comments. Indeed there is something wrong with some cases such as this one where en is selected when en-us is preferred just because of the order of the array values.
I will add use cases and make sure q is understood.

@Spomky Spomky force-pushed the bugs/incorrect-locale-selection branch from 48c8c98 to b1525a3 Compare December 11, 2023 19:19
@Spomky Spomky force-pushed the bugs/incorrect-locale-selection branch 4 times, most recently from f8b2126 to 4ed69e0 Compare December 11, 2023 19:57
@Spomky
Copy link
Contributor Author

Spomky commented Dec 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 the q parameter is already used. The test with a reversed order priority returns the expected language.

In addition, the returned locale value is always xx_XX to make it consistant with the value used internally.
Also, it accepts locales such as hi_Latn_IN to be compared.

Copy link
Member

@nicolas-grekas 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.

Here's some nitpicking. What's the state of this PR? Any votes from previous reviewers?

src/Symfony/Component/HttpFoundation/Request.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpFoundation/Request.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpFoundation/Request.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpFoundation/Request.php Outdated Show resolved Hide resolved
@Spomky Spomky force-pushed the bugs/incorrect-locale-selection branch from 10b0a45 to 85e3a7f Compare February 1, 2024 19:45
@Spomky
Copy link
Contributor Author

Spomky commented Feb 1, 2024

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

Copy link
Member

@nicolas-grekas 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 Outdated Show resolved Hide resolved
src/Symfony/Component/HttpFoundation/Request.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpFoundation/Request.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpFoundation/Request.php Outdated Show resolved Hide resolved
src/Symfony/Component/HttpFoundation/Request.php Outdated Show resolved Hide resolved
@Spomky Spomky force-pushed the bugs/incorrect-locale-selection branch 2 times, most recently from 16a03e4 to 1d5e821 Compare April 3, 2024 18:34
@Spomky Spomky changed the base branch from 6.4 to 7.1 April 3, 2024 18:34
@Spomky Spomky force-pushed the bugs/incorrect-locale-selection branch from 1d5e821 to f67a5d7 Compare April 3, 2024 18:39
@stof
Copy link
Member

stof commented Apr 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 in https://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.

Copy link
Member

@nicolas-grekas 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.

LGTM thanks

PHP already has a locale matching algorithm in ext-intl [...] the proper way to solve it might be to implement them in the polyfill

Good idea! I suggest merging this PR, and switch to the polyfill once we have it. Nothing is public here so the switch should be easy.

@Spomky Spomky force-pushed the bugs/incorrect-locale-selection branch from 8a259ab to fbe44f0 Compare April 4, 2024 06:10
src/Symfony/Component/HttpFoundation/Request.php Outdated Show resolved Hide resolved
@fabpot fabpot modified the milestones: 6.4, 7.1 Apr 5, 2024
@fabpot fabpot force-pushed the bugs/incorrect-locale-selection branch from fbe44f0 to ef73505 Compare April 5, 2024 06:35
@fabpot
Copy link
Member

fabpot commented Apr 5, 2024

Thank you @Spomky.

@fabpot fabpot merged commit d1f3ee8 into symfony:7.1 Apr 5, 2024
7 of 10 checks passed
@Spomky Spomky deleted the bugs/incorrect-locale-selection branch April 7, 2024 16:37
@fabpot fabpot mentioned this pull request May 2, 2024
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.

None yet