Dont set the locale implicitly

[MatthiasKuehneEllerhold]

Q	A
Documentation	no
Bugfix	yes*
BC Break	no*
New Feature	no
RFC	no
QA	no

• = my opinion, yours may be different

Description

On a multi-locale project there may be some use cases where the locale is not fixed but has to be changed for certain actions. e. g. rendering a PDF in multiple languages, pre-rendering a view-script in multiple languages, rendering an email in a background job where no locale is inherently set.

Ideadlly I want to have a single source of truth: \Locale::getDefault(). This is used in php-internal functions like the DateFormatter and so on, so I can switch the output locale on the fly.

Sadly the view helpers of this module "cache" the locale in their state so I have to go through each and every one that may do this and set the locale explicitly.

This is hard to debug because the first usage of a specific view helper always uses the correct locale (because $this->locale is still null), but the second usage of the view helper takes the old $locale and not the new.

[froschdesign]

@MatthiasKuehneEllerhold

Q	A
BC Break	possibly

We need to clarify that. Do you see a scenario in which this change could lead to problems?
I will check my applications for this.

[MatthiasKuehneEllerhold]

Well, its complicated?

• set locale1 via \Locale::setDefault() and not to the view helper
• use said view helper
• "locale1" is used
• set locale2 via \Locale:.setDefault() and not the view helper
• use said view helper
• what happened before: locale1 is used
• what happens now: locale2 is used.

So yes this is a change of behavior to before.

I would argue that the previous behavior is a bug in need of a workaround (as I did) not a feature that someone purposefully used to somehow get the old locale after he/she set a new one via \Locale::setDefault().

So no I cant imagine using this bug purposefully and therefore dont think its BC break, but rather a bug fix that fixes faulty behavior. Your usage may vary though - I can only speak for my projects and my imagination ;-)

So Im open to your opinions on this.

[froschdesign]

The fact that the value of the locale is stored is not documented:

Default Value

By default, if no locale is provided, helper will use the system locale provided by PHP's Locale::getDefault().

This wording is used by almost all view helpers and validators.

[froschdesign]

The translator uses the same logic:

laminas-i18n/src/Translator/Translator.php

Lines 252 to 264 in 80d9f5d

	/**
	* Get the default locale.
	*
	* @return string
	*/
	public function getLocale()
	{
	if ($this->locale === null) {
	$this->locale = Locale::getDefault();
	}
	return $this->locale;
	}

Also the IsFloat validator:

laminas-i18n/src/Validator/IsFloat.php

Lines 75 to 86 in 80d9f5d

	/**
	* Returns the set locale
	*
	* @return string
	*/
	public function getLocale()
	{
	if (null === $this->locale) {
	$this->locale = Locale::getDefault();
	}
	return $this->locale;
	}

The IsInt validator:

laminas-i18n/src/Validator/IsInt.php

Lines 75 to 86 in 80d9f5d

	/**
	* Returns the set locale
	*
	* @return string|null
	*/
	public function getLocale()
	{
	if (null === $this->locale) {
	$this->locale = Locale::getDefault();
	}
	return $this->locale;
	}

In the DateTime and Postcode validators the locale is set in the constructor:

laminas-i18n/src/Validator/DateTime.php

Lines 72 to 87 in 80d9f5d

	public function __construct($options = [])
	{
	// Delaying initialization until we know ext/intl is available
	$this->dateType = IntlDateFormatter::NONE;
	$this->timeType = IntlDateFormatter::NONE;
	$this->calendar = IntlDateFormatter::GREGORIAN;
	parent::__construct($options);
	if (null === $this->locale) {
	$this->locale = Locale::getDefault();
	}
	if (null === $this->timezone) {
	$this->timezone = date_default_timezone_get();
	}
	}

laminas-i18n/src/Validator/DateTime.php

Lines 232 to 240 in 80d9f5d

	/**
	* Returns the locale used by the IntlDateFormatter or the system default if none given
	*
	* @return string|null
	*/
	public function getLocale()
	{
	return $this->locale;
	}

laminas-i18n/src/Validator/PostCode.php

Lines 235 to 245 in 80d9f5d

	public function __construct($options = [])
	{
	if ($options instanceof Traversable) {
	$options = ArrayUtils::iteratorToArray($options);
	}
	if (array_key_exists('locale', $options)) {
	$this->setLocale($options['locale']);
	} else {
	$this->setLocale(Locale::getDefault());
	}

laminas-i18n/src/Validator/PostCode.php

Lines 256 to 264 in 80d9f5d

	/**
	* Returns the set locale
	*
	* @return string|null The set locale
	*/
	public function getLocale()
	{
	return $this->locale;
	}

A standardised behaviour would therefore be appropriate.

[froschdesign]

So no I cant imagine using this bug purposefully and therefore dont think its BC break, but rather a bug fix that fixes faulty behavior. Your usage may vary though - I can only speak for my projects and my imagination ;-)

I immediately assumed a BC break at the beginning and even you had labelled it as a possibility. A more in-depth analysis is therefore essential.
I would expect that the translator would keep the original value but for the rest… 🤔

[MatthiasKuehneEllerhold]

Oh Ive missed those. Ill be changing them too for completeness' sake.

[froschdesign]

@gsteel
You added the CountryCodeDataList helper last, maybe you can say something about the default value for the locale?
Thanks in advance! 👍🏻

[MatthiasKuehneEllerhold]

Fixed the unit tests and removed the assert() in CountryCodeDataListFactoryTest because the type hint takes care of that.
EDIT: still on it apparently...

The CountryCodeDataList suffered from the same problem. The factory did a fallback to \Locale::getDefault() on creating the Helper. Ive changed that in the same manner as the other view helpers: it uses the fallback during runtime (and not set it to a variable).

[MatthiasKuehneEllerhold]

Psalm is ... something else.

[froschdesign]

@MatthiasKuehneEllerhold

The CountryCodeDataList suffered from the same problem. The factory did a fallback to \Locale::getDefault() on creating the Helper. Ive changed that in the same manner as the other view helpers: it uses the fallback during runtime (and not set it to a variable).

This view helper does it right, a locale should always be set. The factory provides the initial value, and when the helper is called, a customised locale can be defined. Manipulation from outside of the helper, validator or translator falls into the category of magic.
Beside that the concrete classes are independent of PHP's Locale class and the locale can be set via application configuration. And a custom factory can also change the origin of the locale string.

[MatthiasKuehneEllerhold]

@MatthiasKuehneEllerhold

The CountryCodeDataList suffered from the same problem. The factory did a fallback to \Locale::getDefault() on creating the Helper. Ive changed that in the same manner as the other view helpers: it uses the fallback during runtime (and not set it to a variable).

This view helper does it right, a locale should always be set.

What differentiates the CountryCodeDataList from the other view helpers in this regard? Why does the CountryCodeDataList always need a locale set but the Translate view helper doesnt and is allowed to fallback to \Locale::getDefault()?

The factory provides the initial value, and when the helper is called, a customised locale can be defined. Manipulation from outside of the helper, validator or translator falls into the category of magic. Beside that the concrete classes are independent of PHP's Locale class and the locale can be set via application configuration. And a custom factory can also change the origin of the locale string.

Then every call to Locale should be removed and everything has to go through the factory.

Which would make using this lib a lot harder. Even if only one locale / language is used...

What Laminas could do is to define a central class where every other class gets the currently active locale from. A one-stop-shop to switch the active locale in every subsystem that needs it. Every class only has a reference and uses it on the fly.

Tada you're reimplementing Locale::getDefault().

[froschdesign]

Why does the CountryCodeDataList always need a locale set but the Translate view helper doesnt and is allowed to fallback to \Locale::getDefault()?

Please keep in mind that there are years between these two view helpers. But changing the behaviour in this direction for all helpers means creating a new major version.

Which would make using this lib a lot harder to use. Even if only one locale / language is used...

Why harder? You can set it via configuration and it is done. If you require a different locale, define it when calling the helper.

[froschdesign]

Tada you're reimplementing Locale::getDefault().

I have never said that PHP's Locale is obsolete but it is no longer needed in the concrete class only in factories or configuration.

[MatthiasKuehneEllerhold]

Its harder when I want to render the same view script in 5 different locales. I have to go through each and every class and look if it has the old locale set and set it to the new one.
With the proposed changes every class uses the locale from Locale::getDefault() and all I have to do is call Locale::setDefault($newLocale) to apply it to all translation-related classes.

Alas I dont want to die on this hill, Ive never used the CountryCodeDataList in the past and may never do so in the future.
If you want me to revert the changes, I'll do it.

[froschdesign]

I have to go through each and every class and look if it has the old locale set and set it to the new one.

So that we don't misunderstand each other, please ignore the old helpers.

Here is an example:

final class CustomFormatHelper
{
    public function __construct(private string $defaultLocale)
    {}

    public function __invoke(string $text, int $format, ?string $locale = null): string
    {
        // …
    }
}

Usage:

$helper = new CustomFormatHelper('de_DE');

echo $helper('example', 1);
echo $helper('example', 2);

echo $helper('example', 1, 'en_US');
echo $helper('example', 2, 'en_US');
echo $helper('example', 1, 'de_CH');

No setters or getters for the locale and no magic to manipulate the helper's state from the outside.

[gsteel]

Am I right in assuming that this patch is specifically to prevent the state inside the view helpers and validators being populated with Locale::getDefault(), so that you can change the default locale via Locale::setDefault() multiple times whilst the view helpers and validators remain in memory?

In this case, the whole premise is flawed - The locale should be passed to the view helpers via __invoke and the validators should be made aware of the locale via $options['locale'].

By explicitly providing the desired locale, where it's needed at runtime, the default locale becomes irrelevant.

Edit: I added this comment before seeing @froschdesign 's more recent comments which I believe echoes what I've said there.

[froschdesign]

@gsteel
Your comment is correct and welcome. I myself had to read the code again first, which is why my concrete statements are so late.
Thanks! 👍🏻

[MatthiasKuehneEllerhold]

Am I right in assuming that this patch is specifically to prevent the state inside the view helpers and validators being populated with Locale::getDefault(), so that you can change the default locale via Locale::setDefault() multiple times whilst the view helpers and validators remain in memory?

Yes exactly.

In this case, the whole premise is flawed - The locale should be passed to the view helpers via __invoke and the validators should be made aware of the locale via $options['locale'].

So each and every invocation of a view helper should have the $locale parameter set OR the default locale should be set via setLocale() (or something similar)?

This will increase the needed boiler plate in our view script by A LOT. Each translate() now needs the $textDomain and $locale explicitly set instead of using the fallback (like now).
OR
I have to go through each and every class registered in the service manager and registered plugin managers (like the view helper), figure it if and how the old locale is set and overwrite it.
I already got something like this in place which has the downside that it creates instances of ALL view helpers - even the one I never used and sets some value to it. Which in turn increases RAM consumption and slows everything down.

I'd argue that this change would in need of a new major version too. But oh well.

By explicitly providing the desired locale, where it's needed at runtime, the default locale becomes irrelevant.

Edit: I added this comment before seeing @froschdesign 's more recent comments which I believe echoes what I've said there.

Im not sure on how to go forward now - if there is any way now at all.

What I'm getting from your two comments is, that my PR should be dismissed and all view helper reworked to be analogeous to the behavior in CountryCodeDataList ?

Stating this upfront would have saved me a lot of wasted time.

[froschdesign]

@gsteel
Would it be okay for you if we adopted the changes for the helpers for the current major version?
But I would revert the changes for the translator class and the new CountryCodeDataList helper. Because that makes no sense to me:

Locale::setDefault('de_DE');

$translator = new Laminas\I18n\Translator\Translator();
var_dump($translator->getLocale()); // string(5) "de_DE"

Locale::setDefault('en_US');

var_dump($translator->getLocale()); // string(5) "en_US"

And the new CountryCodeDataList helper is the way for the future.

---

Unfortunately, the differences in handling have always been there, so we are not making things worse at this point.
What do you think?

[gsteel]

I don't see the initial patch as problematic - to begin with, you were just returning Locale::getDefault() when a locale was not explicitly passed, in favour of returning $this->locale from Whatever::getLocale() specifically within view helpers - I don't see that as a problem - it just swaps state from one place to another and I don't foresee this affecting users even though it is technically a BC break.

The later changes to the validators and CountryDataList are too much IMO. Validators should not be kept in memory anyhow as each instance should only be used for the lifecycle of a form, and the CountryDataList helper is designed to be explicitly configured - changing the factory here is pointless because once constructed, the view helper will remain in memory.

I still haven't actually reviewed the changes yet, so I'll try and do that ASAP so that we can maybe get a middleground that solves your problems without affecting future design goals.

As @froschdesign mentions, in future, all helpers would benefit from being declared something like:

final readonly class NumberFormat
{
  public function __construct(public string $defaultLocale) {}
  
  public function __invoke(int|string $number, string|null $locale): string
  {
     $locale = $locale ?? $this->defaultLocale;
     
     return $this->formatStuff($number, $locale);
  }
}

And yes, this would necessitate passing in the locale explicitly at the point of use in all view scripts in cases where you wish to render the same view script in multiple different locales within the same process.

I'd say it would be a big improvement for all helpers to not have access to any global state.

[MatthiasKuehneEllerhold]

Which changes do you want me to revert further?

[froschdesign]

@MatthiasKuehneEllerhold

Which changes do you want me to revert further?

Please revert everything except the changes in the view helper.
Thanks in advance! 👍🏻

Sorry for the delay.

[MatthiasKuehneEllerhold]

Done.

[froschdesign]

@MatthiasKuehneEllerhold
Can I ask to squash the commits?
Thanks in advance! 👍🏻

[MatthiasKuehneEllerhold]

Squashed the commits to hide the history.

[gsteel]

Thanks @MatthiasKuehneEllerhold

It's worth noting that this slight behaviour change should not be relied upon in the next major, even though it more closely aligns with the current documentation.

I am expecting that the setLocale and getLocale methods will be dropped from all the helpers, along with internal calls to Locale::getDefault, and instead, to deviate from the configured default locale, it would be incumbent on the caller to pass the desired locale via __invoke.

Allowing external state to dictate behaviour is not something I personally want to see perpetuated.

[MatthiasKuehneEllerhold]

It seems I struck a nerve with this PR. I wanted to make the classes in this lib more to my liking, more usable for us. I guess I thought others felt the same.
But it seems like you (als techincal leads) dont agree with that and want to take a different path. A path I think is more pure (cleaner), but a whole lot less practical. Thats fine. Its your lib, you can do with it what you want.

So you're saying that I can not trust this implementation to last more than until the next major version. This means 3.0 right? This might be weeks or months away, so its hard to judge how long this change will last then.

@gstell If you want I can drop this PR and be done with it. Going back to the old implementation proved to be much less hassle than arguing with you for a month straigth.

[gsteel]

No nerves have been struck @MatthiasKuehneEllerhold - Purity is definitely a goal for me - this kind of global state can be especially problematic in environments like Swoole and RoadRunner. I can't speak for all the members of the TSC mind, perhaps @Ocramius or @weierophinney might offer another opinion.

Your patch is appreciated - it has highlighted potential issues with the design of the lib that we might want to address in the next major, which is probably a way off, and thanks to your contributions, we have a clearer idea of what others want from this library.

[Ocramius]

I did not read up to everything above, but the diff is indeed a behaviour BC break.

I'm for it, but it can only happen in a new major release.

That said, I'm similarly a friend of referential transparency, and detecting the locale inside a getter method is something I would not have added to the codebase within the last decade: it's just inherited behaviour that we have to live with, for now.