bezhanSalleh / filament-language-switch

A versatile and user-friendly plugin designed for Filament Panels.
MIT License
226 stars 44 forks source link

Only first browser locale is matched, and others ignored (most cases) #68

Closed wq9578 closed 10 months ago

wq9578 commented 10 months ago

Due to a bug only the first browser locale is matched.

In most cases the second browser locale is relevant (en in en-US,en;...). The buggy foreach loop then returns null, which means the browser locale is ignored.

Fixed source file below, see function getBrowserLocale. Also some minor optimization added (LanguageSwitch::make()->getLocales() only called once).

<?php

namespace BezhanSalleh\FilamentLanguageSwitch\Http\Middleware;

use BezhanSalleh\FilamentLanguageSwitch\LanguageSwitch;
use Closure;
use Illuminate\Http\Request;

class SwitchLanguageLocale
{
    public function handle(Request $request, Closure $next): \Illuminate\Http\Response | \Illuminate\Http\RedirectResponse
    {
        $locale = session()->get('locale')
            ?? $request->get('locale')
            ?? $request->cookie('filament_language_switch_locale')
            ?? $this->getBrowserLocale($request)
            ?? config('app.locale', 'en');

        if (in_array($locale, LanguageSwitch::make()->getLocales())) {
            app()->setLocale($locale);
        }

        return $next($request);
    }

    private function getBrowserLocale(Request $request): ?string
    {
        $userLangs = preg_split('/[,;]/', $request->server('HTTP_ACCEPT_LANGUAGE'));

        /* Previous code (bug: foreach returns already in first loop)
        foreach ($userLangs as $locale) {
            return in_array($locale, LanguageSwitch::make()->getLocales())
                ? $locale
                : null;
        }
        */

        // New code begin
        $language_switch_locales = LanguageSwitch::make()->getLocales();

        foreach ($userLangs as $locale) {
            if (in_array($locale, $language_switch_locales)) {
                return $locale;
            }
        }

        return null;
        // New code end
    }
}
UtkuDalmaz commented 10 months ago

I was having an issue. It reverted to the browser locale even though I set it to another. Is that related to this bug?

wq9578 commented 10 months ago

I don't think so. It looks like that your session data / cookie was lost. Once set, it clearly has precedence over the browser locale:

        $locale = session()->get('locale')
            ?? $request->get('locale')
            ?? $request->cookie('filament_language_switch_locale')
            ?? $this->getBrowserLocale($request)
            ?? config('app.locale', 'en');
UtkuDalmaz commented 10 months ago

Sometimes when I open the page, it shows different languages on the page and the plugin. For example page loads in Turkish but it shows EN (English) in the up-right switcher.

bezhanSalleh commented 10 months ago

Sometimes when I open the page, it shows different languages on the page and the plugin. For example page loads in Turkish but it shows EN (English) in the up-right switcher.

As @wq9578 said if session and cookies are lost then it will default to the browser locale then the app locale.

bezhanSalleh commented 10 months ago

Due to a bug only the first browser locale is matched.

In most cases the second browser locale is relevant (en in en-US,en;...).

The buggy foreach loop then returns null, which means the browser locale is ignored.

Fixed source file below, see function getBrowserLocale.

Also some minor optimization added (LanguageSwitch::make()->getLocales() only called once).


<?php

namespace BezhanSalleh\FilamentLanguageSwitch\Http\Middleware;

use BezhanSalleh\FilamentLanguageSwitch\LanguageSwitch;

use Closure;

use Illuminate\Http\Request;

class SwitchLanguageLocale

{

    public function handle(Request $request, Closure $next): \Illuminate\Http\Response | \Illuminate\Http\RedirectResponse

    {

        $locale = session()->get('locale')

            ?? $request->get('locale')

            ?? $request->cookie('filament_language_switch_locale')

            ?? $this->getBrowserLocale($request)

            ?? config('app.locale', 'en');

        if (in_array($locale, LanguageSwitch::make()->getLocales())) {

            app()->setLocale($locale);

        }

        return $next($request);

    }

    private function getBrowserLocale(Request $request): ?string

    {

        $userLangs = preg_split('/[,;]/', $request->server('HTTP_ACCEPT_LANGUAGE'));

        /* Previous code (bug: foreach returns already in first loop)

        foreach ($userLangs as $locale) {

            return in_array($locale, LanguageSwitch::make()->getLocales())

                ? $locale

                : null;

        }

        */

        // New code begin

        $language_switch_locales = LanguageSwitch::make()->getLocales();

        foreach ($userLangs as $locale) {

            if (in_array($locale, $language_switch_locales)) {

                return $locale;

            }

        }

        return null;

        // New code end

    }

}

It was a PR didn't notice the issue with the loops. Nice catch 👍 Looks good to me could you pr it. Thanks.