Remove usage of legacy locale parameter

[loic425]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets	partially symfony/recipes-contrib#1349)
License	MIT

[Roshyo]

What about keeping locale and introducing available_locales ? So that there is no BC break. 🤔

[vvasiloi]

Symfony defines kernel.default_locale and kernel.enabled_locales parameters, which are used for i18n across all it's components.
Why don't we use the same parameters, at least as defaults?

[loic425]

@vvasiloi Yes, you're right. This morning, I had an idea and I will mix it with your proposal.

[vvasiloi]

After this is merged and released, we should also change the sylius.translation_locale_provider.admin service definition in Sylius Core to use the new parameter.

[Zales0123]

👍

[Zales0123]

I understand all of these parameters were missing, but it would be more ordered if we add only enabled_locales and default_locale here and the rest of them in a separate PR 😄 #beingPettyIsMyPassion

[Zales0123]

As far as I see, previously it was only a %locale% parameter. Dit it fallback previously to 'en' too? If it didn't, maybe we should throw some exception to not assume anything that was not assumed before? 🤔 I just would like to keep it as close to the previous implementation as possible 🖖

[vvasiloi]

This is a BC layer, so throwing an exception here defies the purpose of this code.
It could trigger a deprecation error though, warning that this default will be removed and that it should be configured explicitly.

[vvasiloi]

The en is coming from the default value of locale parameter configured in Sylius, Sylius Standard and Sylius Plugin Skeleton, which all should be removed after this PR is merged.

[lchrusciel]

I have mixed feelings about it as well. What is more, it is bumped by the fact that we do not have tests for more than one config present here and a description of winning strategy (for now it is: 1. new resource config, 2. previous parameter, 3. Symfony parameter, 4. "en").

Another concern is that we are replacing the old parameter with the new one entirely, but both configurations may (and probably should) work together. This parameter is used in several places. This is the screenshot from the current version of Sylius-Standard:

So now - the best config would be to have $config['translation']['default_locale'] defined together with the %locale% parameter defined anyway - the application will work, and no deprecation warning will be triggered (as we have early return). Also, the application will work with the %locale% parameter defined without our new config, but the deprecation will be triggered. If nothing is defined or only our new config - the whole application will collapse. Am I missing something?

What I would recommend is as follows - the %locale% parameter should be declared together with the %sylius.resource.translation.default_locale% if not present based on the new config (that could have "en" defined as default in config tree). If the%locale% parameter is defined together with our new config, then the error/deprecation warning should be triggered(I'm not sure about which one). The last question remains - should the "%locale%" be overridden by our config, or should it override our default locale? I would say the latter, but I'm open to discussing it.

Anyway, I'm also lacking the cases I've described exposed as configuration tests + test drive with the current Sylius and/or Sylius-Standard to check the correctness of the new solution.

[vvasiloi]

I have revisited this and I agree that it needs some changes.
The goal is to get rid of the locale parameter and use the new configuration.
This means that internally, all Sylius packages, should remove the locale parameter and use the new configuration.
The only way a locale parameter will still exist, is if it's still defined in the end user projects.
That's where the BC layer comes in action and uses the value of the locale parameter to feed the new configuration, but only if it wasn't already explicitly set by the user.
In this case, a deprecation error should be triggered encouraging the user to migrate from using the locale parameter to the new configuration.
Here are the possible scenarios and the outcomes I think they should have:

	config defined	config undefined
locale defined	1. Use the config and suggest the user to remove the redundant parameter.	2. Use the parameter value as default config values and trigger a deprecation error to encourage the user to explicitly set the config values and remove the parameter.
locale undefined	3. Use the config. This is the desired outcome.	4. Use the default configuration, which is set to kernel.default_locale and kernel.enabled_locales. This will be the default behavior in new installs.

There's a 5th scenario, which is a combination of the 4th scenario with Symfony 4.4, which does not have the kernel.enabled_locales parameter.
There is another important detail to consider in this case: when kernel.enabled_locales is empty, all locales should be enabled.
In the Resource Bundle context this is not a viable solution, so we should stick to the current behavior and only use the default locale as the only available locale by default.
This means that, when kernel.enabled_locales is empty (normal behavior) or is not defined (BC layer), the default value of sylius.resource.translation.enabled_locales should be and array with only sylius.resource.translation.default_locale.

To achieve the above, the following modifications must be made to the currently proposed changes:

1. Add note about the what happens when kernel.enabled_locales is empty;
2. Define the default locale parameter first, as it should always be
3. sylius.resource.translation.enabled_locales should fallback to sylius.resource.translation.default_locale (instead of the current ['en'] if no other options were found;
4. $container->setParameter('sylius.resource.translation.default_locale', 'en'); should be removed and an error should be thrown if the default locale was not set any other way

WDYT?

[loic425]

@vvasiloi Thank you, this is an awesome explanation. I think your suggested change is perfect.