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

feat: Strict locale negotiate #9360

Open
wants to merge 8 commits into
base: 4.6
Choose a base branch
from

Conversation

neznaika0
Copy link
Contributor

@neznaika0 neznaika0 commented Dec 31, 2024

Description
See #9256
I added a setting for the desired behavior.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • Conforms to style guide
  • User guide updated

Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

While this may be a solution for some simpler cases, I think more complex ones won't be so simple.

If we take $supportedLocales as an array of en_US, fr_FR, and the browser is set to fr_BE, the loaded locale will be en_US. Is this the desired result? We can argue about that.

I think in this case changes would be needed to select the fallback locale.

I'm curious what others think about it.

app/Config/Feature.php Outdated Show resolved Hide resolved
app/Config/Feature.php Outdated Show resolved Hide resolved
@neznaika0
Copy link
Contributor Author

If we take $supportedLocales as an array of en_US, fr_FR, and the browser is set to fr_BE, the loaded locale will be en_US. Is this the desired result? We can argue about that.

It current behavior - select first element array

@michalsn
Copy link
Member

michalsn commented Jan 1, 2025

Yes, I know. While it makes sense when we compare locales loosely. I don't believe it should work that way with strict comparison. Fallback should search for the best match.

@neznaika0
Copy link
Contributor Author

neznaika0 commented Jan 1, 2025

What would be better than the first (main) element of the array?
EDIT: I note that this is the first change. it's in the Feature settings. It can be transferred to the App when there are user reviews.

@michalsn
Copy link
Member

michalsn commented Jan 1, 2025

What would be better than the first (main) element of the array?

In the example I presented it would be fr_FR.

We would have to search for the best matching option. It would be fr as a first choice (if available is $supportedLocales) or fr_FR.

@michalsn michalsn added the wrong branch PRs sent to wrong branch label Jan 1, 2025
@neznaika0
Copy link
Contributor Author

I tried to add logic to find the nearest locale. But it looks bad.

I added a repeat search for fr if the locale from the request (result best-match) does not start with fr and returned the first fr-BE instead of en-US.

Can I save the first variant?

@michalsn
Copy link
Member

michalsn commented Jan 2, 2025

How about something like this? Although I haven't tested it yet...

protected function getBestLocaleMatch(array $supported, ?string $header): string
{
    if ($supported === []) {
        throw HTTPException::forEmptySupportedNegotiations();
    }

    if ($header === null || $header === '') {
        return $supported[0];
    }

    $acceptable      = $this->parseHeader($header);
    $fallbackLocales = [];

    foreach ($acceptable as $accept) {
        // if acceptable quality is zero, skip it.
        if ($accept['q'] === 0.0) {
            continue;
        }

        // if acceptable value is "anything", return the first available
        if ($accept['value'] === '*') {
            return $supported[0];
        }

        // look for exact match
        if (in_array($accept['value'], $supported, true)) {
            return $accept['value'];
        }

        // set a fallback locale
        $fallbackLocales[] = strtok($accept['value'], '-');
    }

    foreach ($fallbackLocales as $fallbackLocale) {
        // look for exact match
        if (in_array($fallbackLocale, $supported, true)) {
            return $fallbackLocale;
        }

        // look for locale variants match
        foreach ($supported as $locale) {
            if (str_starts_with($locale, $fallbackLocale . '-')) {
                return $locale;
            }
        }
    }

    return $supported[0];
}

For $supportedLocales set to ['en-US', 'fr-FR'], it will first check for all the options from the Accept-Language header. If no exact match is found, it will check the "general locale": en and then the "locale variant": en-*. The en is just an example, the values will come from the parsed header.

@neznaika0
Copy link
Contributor Author

I did something similar, but right in the language() method. It seems to me that duplication is not good.

@michalsn
Copy link
Member

michalsn commented Jan 2, 2025

Oh... I would definitely choose a separate method. Even if some small parts are the same, the main goal is to make it as readable as possible.

@neznaika0 neznaika0 force-pushed the feat/strict-locale-negotiate branch from 7432d2d to 29472c6 Compare January 3, 2025 12:07
@neznaika0 neznaika0 changed the base branch from develop to 4.6 January 3, 2025 12:07
@paulbalandan paulbalandan removed the wrong branch PRs sent to wrong branch label Jan 3, 2025
@neznaika0
Copy link
Contributor Author

Ready.

I would pay attention to the description. It is not completely correct. Depends on the array of locales and the query. Sometimes it works, sometimes it returns en instead of en-US. Is the reason for PR. Is there any way to describe it?

The system is smart enough to fall back to more generic language codes if an exact match
cannot be found. If the locale code was set to ``en-US`` and we only have language files set up for ``en``
then those will be used since nothing exists for the more specific ``en-US``. If, however, a language
directory existed at the **app/Language/en-US** directory then that would be used first.

@neznaika0 neznaika0 requested a review from michalsn January 3, 2025 12:19
@@ -26,4 +26,10 @@ class Feature extends BaseConfig
* If false, `limit(0)` returns no records. (the behavior of 3.1.9 or later in version 3.x.)
*/
public bool $limitZeroAsAll = true;

/**
* Set `false` to use strict localization comparison (with territory en-*) instead of an abbreviated value.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Set `false` to use strict localization comparison (with territory en-*) instead of an abbreviated value.
* Set `false` to use strict localization comparison (e.g., against locale code `en-US` instead of the ISO 639-1 language code `en`)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will accept michalsn's correction.


/**
* Set `false` to use strict localization comparison (with territory en-*) instead of an abbreviated value.
* Set `true`, so territory was cut off (en-* as en) before localization comparing.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Set `true`, so territory was cut off (en-* as en) before localization comparing.
* If set to `true`, the ISO 3166-1 region code will be cut off (e.g., `en-US` as `en`) before localization comparison.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will accept michalsn's correction.

system/HTTP/Negotiate.php Show resolved Hide resolved
@@ -52,4 +52,4 @@ All Changes
This is a list of all files in the **project space** that received changes;
many will be simple comments or formatting that have no effect on the runtime:

- @TODO
- @TODO
Copy link
Member

Choose a reason for hiding this comment

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

what changed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I restore code, maybe space?

app/Config/Feature.php Show resolved Hide resolved
system/HTTP/Negotiate.php Show resolved Hide resolved
system/HTTP/Negotiate.php Show resolved Hide resolved
system/HTTP/Negotiate.php Show resolved Hide resolved
system/HTTP/Negotiate.php Show resolved Hide resolved
user_guide_src/source/changelogs/v4.6.0.rst Show resolved Hide resolved
user_guide_src/source/changelogs/v4.6.0.rst Show resolved Hide resolved
@neznaika0
Copy link
Contributor Author

neznaika0 commented Jan 3, 2025

Pff.. Again, my small changes overlap with your additions. 😄

@michalsn
Copy link
Member

michalsn commented Jan 3, 2025

Pff.. Again, my small changes overlap with your additions. 😄

Yeah, I know - sorry, but I believe this will fit better. I hope I explained my reasons.

Copy link
Contributor Author

@neznaika0 neznaika0 left a comment

Choose a reason for hiding this comment

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

Ready.

@paulbalandan paulbalandan added enhancement PRs that improve existing functionalities 4.6 labels Jan 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.6 enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants