Bug: can't change and override valid locales

[davislasis]

Problem
System does not change incoming request locale with overriding it from outer source (not App config, like database) using filter approach

Describe the bug

• all my routes has {locale} in front as first url segment 'en/dashboard', 'lv/dashboard' etc
• supported locales are stored in local mysql database and managed through admin area
• my project will populate / manage languages only through database and admin area, no need of App config $supportedLocales as client will not change and re-commit files
• to avoid caching locale change in frontend, i have setup mainFilter which caches potential locale change at any point (route) within the system
• IncomingRequest class is initialised before filters, so if my filter override supportedLocales - it does not change in further system loding

CodeIgniter 4 version
4.1.1

Solution 1:

• IncomingRequest class - variable $validLocales change from protected to public
• this way it is possible to override values if needed

Affected module(s)
/system/HTTP/IncomingRequest.php

https://github.com/codeigniter4/CodeIgniter4/blob/develop/system/HTTP/IncomingRequest.php#L99

Solution 2:

• as App config does not have $validLocales variable
• as IncomingRequest class - variable $validLocales is not heavily used elsewhere
• as IncomingRequest class controller mandatory requests App config as first method param
• remove variable $validLocales from IncomingRequest class
• as IncomingRequest class method setLocale() uses $validLocales, replace it with already loaded $this->config->supportedLocales

Affected module(s)
/system/HTTP/IncomingRequest.php

remove: https://github.com/codeigniter4/CodeIgniter4/blob/develop/system/HTTP/IncomingRequest.php#L99

remove: https://github.com/codeigniter4/CodeIgniter4/blob/develop/system/HTTP/IncomingRequest.php#L162

update: https://github.com/codeigniter4/CodeIgniter4/blob/develop/system/HTTP/IncomingRequest.php#L225

	/**
	 * Sets the locale string for this request.
	 *
	 * @param string $locale
	 *
	 * @return IncomingRequest
	 */
	public function setLocale(string $locale)
	{
		// If it's not a valid locale, set it
		// to the default locale for the site.
		if (! in_array($locale, $this->config->supportedLocales))
		{
			$locale = $this->defaultLocale;
		}

		$this->locale = $locale;

		// If the intl extension is loaded, make sure
		// that we set the locale for it... if not, though,
		// don't worry about it.
		// this should not block code coverage thru unit testing
		// @codeCoverageIgnoreStart
		try
		{
			if (class_exists('\Locale', false))
			{
				\Locale::setDefault($locale);
			}
		}
		catch (\Exception $e)
		{
		}
		// @codeCoverageIgnoreEnd

		return $this;
	}

For reference my MainFilter class

<?php namespace Modules\Core\Base\Filters;

use CodeIgniter\HTTP\RequestInterface;
use CodeIgniter\HTTP\ResponseInterface;
use CodeIgniter\Filters\FilterInterface;
use Config\App;
use Config\Database;

class MainFilter implements FilterInterface
{
    /**
     * Do whatever processing this filter needs to do.
     * By default it should not return anything during
     * normal execution. However, when an abnormal state
     * is found, it should return an instance of
     * CodeIgniter\HTTP\Response. If it does, script
     * execution will end and that Response will be
     * sent back to the client, allowing for error pages,
     * redirects, etc.
     *
     * @param \CodeIgniter\HTTP\RequestInterface $request
     *
     * @param null $arguments
     * @return mixed
     */
	public function before(RequestInterface $request, $arguments = null)
	{
        $locales = [];

	    // get all languages from database
        $db = Database::connect();

        $results = $db->table('core_languages')
            ->select('language')
            ->where('published', 1)
            ->get()
            ->getResultArray();

        if (!empty($results)) {
            foreach ($results as $result) {
                $locales[] = $result['language'];
            }
        }

        if (!empty($locales)) {
            /** @var App $config */
            $config = Config('App');
            $config->supportedLocales = $locales;

            $request->config = $config;

            $locale = $request->uri->getSegment(1);

            if (in_array($locale, $locales)) {
                $request->setLocale($locale);
            } else {
                $request->setLocale($config->defaultLocale);
            }
        }
	}

	//--------------------------------------------------------------------

    /**
     * Allows After filters to inspect and modify the response
     * object as needed. This method does not allow any way
     * to stop execution of other after filters, short of
     * throwing an Exception or Error.
     *
     * @param \CodeIgniter\HTTP\RequestInterface $request
     * @param \CodeIgniter\HTTP\ResponseInterface $response
     *
     * @param null $arguments
     * @return mixed
     */
	public function after(RequestInterface $request, ResponseInterface $response, $arguments = null)
	{

	}

	//--------------------------------------------------------------------
}

Context

• OS: Ubuntu 20.04 LTS
• Web server: Apache 2.4.41
• PHP version 7.4 and 8.0

My suggestion
I vote for solution 2 my-self and i can create a PR if needed after discussion and decision

[paulbalandan]

As $validLocales is protected, user extensions of IncomingRequest might be using this property in their own implementations which will break if we remove that. I'll vote partially for 2 with changing only the setLocale method by replacing the locales array in in_array and still set to strict.

		// If it's not a valid locale, set it
		// to the default locale for the site.
		if (! in_array($locale, $this->config->supportedLocales, true))
		{
			$locale = $this->defaultLocale;
		}

[davislasis]

Great call on potential external user extensions to be envolved. I didn't think about that in first place.

I agree with using strict

I think $validLocales is not much used from outside and we could remove it and call it as "breaking change", although safer is not to do that.
From other hand, if CI has plans on this variable - then leave it as is for now, if not - then what's the point to not remove it?

Are we waiting on other opinions and/or comments?
or
Are we set and i can go with pull request?

[paulbalandan]

Please send in a PR. Maybe we'll get comments in the PR. For $validLocales we can deprecate it later if found it to be of no use.

[davislasis]

Great, thanks,
I will try to get some free time on weekend to get PR in.

[MGatner]

As I mentioned on the PR, I think your best bet will be to define a Registrar for App that supplies the locales from your database. This may require some testing since App is loaded pretty early and is a bit of a "special case", but it currently holds the definitive source of application locales so solutions that make IncomingRequest "exempt" from that seem unwise. Now I'm not opposed to thinking bigger about how we handle all these (maybe locales should be their own config??) but for now I think we're bound by backwards-compatibility.

[MGatner]

A third "quick solution" would be to define your own extension of IncomingRequest in app/Config/Services.php. That should be fairly straightforward:

	public static function request(App $config = null, bool $getShared = true)
	{
		if ($getShared)
		{
			return static::getSharedInstance('request', $config);
		}

		$config = $config ?? config('App');

		return new MyIncomingRequest(
			$config,
			static::uri(),
			'php://input',
			new UserAgent()
		);
	}

... then your own request class would be something like:

class MyIncomingRequest extends \CodeIgniter\HTTP\IncomingRequest
{
	/**
	 * Constructor
	 *
	 * @param object      $config
	 * @param URI         $uri
	 * @param string|null $body
	 * @param UserAgent   $userAgent
	 */
	public function __construct($config, URI $uri = null, $body = 'php://input', UserAgent $userAgent = null)
	{
		parent::__construct($config, $uri, $body, $userAgent);

		// Insert your locales database code here for $this->validLocales
	}
}

[davislasis]

@MGatner i can't agree with you on this one + this issue was approved to move on to PR

1. Your request service through MyIncomingRequest - does not work for me (maybe i missed something though)
2. this solution is terribly overcomplicated and it overrides whole IncomingRequest - which actually can be an issue or lead to unexpected behaiviour
3. i guess 90% of novadays websites uses admin panel (database) driven dynamic languages
4. lot's of devs creates platforms, systems and sell them to clients and they can't guess what actuall languages client will gona use in there

based on all those upper points - it's only reasonable that i (as a dev) can easily control list of available system languages (locales) in-between controller call - IF i seriously need that

Thanks,
Dāvis

[MGatner]

@davislasis I have no problems whatsoever with the premise! I want it done right. If you want to submit a PR with Paul's suggestion so we can see what that actually looks like please do.

This problem is not unique: we have requests to load configuration from the database all the time. Registrars are really the tool for this, and they are much simpler than override core services (though I still think that is a viable and logical solution, but I haven't actually tried it).

EDIT: I should add that I agree with eliminating $validLocales eventually, as-is it is a duplicate of App's version and subject to confusion.

[davislasis]

OK, example of PR change is upper in the thread and i already made a PR, only that your closed (deleted) it.
I can re-submit it again if it is not possible to revert it

Solution is

CodeIgniter4/system/HTTP/IncomingRequest.php

Line 224 in b3ebd3d

if (! in_array($locale, $this->validLocales, true))

change it to
if (! in_array($locale, $this->config->supportedLocales, true))

Thanks,
Dāvis

[MGatner]

Oh right, #4478. We can always reopen. I guess I misunderstood what @paulbalandan was saying, I still don't think that is the right solution because it would invalidate anything done in $this->validLocales. Maybe a compromise for now would be to check both?

$locales = array_merge($this->validLocales, $this->config->supportedLocales);
 if (! in_array($locale, $locales, true))
...

Let's see what @paulbalandan says because I didn't know he had addressed this one before I saw your PR.

[davislasis]

OK, let's wait on @paulbalandan 's decision

Meanwhile, your array_merge stuff works, but maybe fully avoid possible conflicts with original core flow, we could implement 2nd param as toggle? Something along those lines:

public function setLocale(string $locale, bool $override = false)
{
    $locales = $this->validLocales;

    if ($override)
    {
        $locales = array_merge($this->validLocales, $this->config->supportedLocales);
    }

    if (! in_array($locale, $locales, true))
    {
        $locale = $this->defaultLocale;
    }

    $this->locale = $locale;
    Locale::setDefault($locale);

    return $this;
}

[paulbalandan]

I haven't thought that while $validLocales is unused anywhere except for the constructor and setLocale, it may be used in userland. So, yeah, removing that fully in the picture might invalidate the stuff made to it. Using registrars seems to me to be complicated (or maybe I just don't know where to hook the registrar call and haven't done it yet).

It seems to me in order to resolve both sides we'd better go with the array_merge solution for now until we get to a deeper fix for this, and a verdict on $validLocales array. My concern is we cannot use $this->config as the overriding config. This must be freshly supplied. The fix only works because you have edited directly the shared instance of App in your filter. If someone will be using a new instance, then $this->config will still be pointing to the old config.

My suggestion is something like this:

public function setLocale(string $locale, ?App $override = null)
{
    if (! is_null($override))
    {
        $this->validLocales = array_merge($this->validLocales, $override->supportedLocales);
    }

    if (! in_array($locale, $this->validLocales, true))
    {
        $locale = $this->defaultLocale;
    }

    ...
}