[Translation] Extract locale fallback computation into a dedicated class #45553

mpdude · 2022-02-24T23:14:05Z

Q	A
Branch?	6.2
Bug fix?	no
New feature?	yes
Deprecations?	yes
Tickets
License	MIT
Doc PR

I am working on an application that in addition to Symfony-based translations needs to work with its own translation system. It would be 💯 if both parts could easily use the same locale hierarchy and fallback rules.

Since fallback resolution is in fact very involved, this PR aims to expose the logic in a dedicated new class, the FallbackLocaleProvider.

In the future, when the fallback locales are to be changed, a new instance of FallbackLocaleProviderInterface needs to be set on the Translator.

TODO

Update CHANGELOG
Add to docs if required
Provide interface for the new class
Add service definition to FrameworkBundle
Check if tests can be refactored/split up with this
Move \Symfony\Component\Translation\Translator::assertValidLocale() to a place where it can also be used by the FallbackLocaleProvider
Make sure deprecations/transition path is sane and add necessary documentation

src/Symfony/Component/Translation/Translator.php

carsonbot · 2022-02-25T19:20:02Z

Hey!

I think @rvanlaak has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

rvanlaak

I see value in the usecase where you want to be able to take more control over the fallback locales. Besides that it also abstracts out a responsibility from the big Translator class, so 👍 for continuing with the remaining TODOs from the PR description.

…es in FallbackLocaleProvider as well

mpdude · 2022-02-28T08:02:44Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/translation.php

@@ -59,6 +61,7 @@
                    'debug' => param('kernel.debug'),
                ],
                abstract_arg('enabled locales'),
+                service('translation.fallback_locale_provider')


Is this correct, or should I use FallbackLocaleProviderInterface::class here?

mpdude · 2022-02-28T08:42:20Z

There is a possible mistake people might run into when they start using the FallbackLocaleProvider:

The Translator needs to know when the fallback locales are changed in order to reset built-in caching. Re-configuring the FallbackLocaleProvider directly without going through Translator::setFallbackLocales() may lead to wrong results.

One solution I see would be:

Deprecate FallbackLocaleProvider::setFallbackLocales(), create a new FallbackLocaleProvider instance instead. In 7.0, this will leave FallbackLocaleProvider immutable.
Deprecate Translator::setFallbackLocales(), use a new setFallbackLocaleProvider() method instead that sets the new instance.

Would that be acceptable?

mpdude · 2022-02-28T09:51:53Z

Additionally, we might get rid of getFallbackLocales() in DataCollectorTranslator and LoggingTranslator if the FallbackLocaleProvider were used directly.

mpdude · 2022-03-03T09:26:33Z

I think I need some opinions and/or feedback on this before I can proceed. 👀 🙏🏻

welcoMattic · 2022-07-29T17:38:40Z

There is a possible mistake people might run into when they start using the FallbackLocaleProvider:

The Translator needs to know when the fallback locales are changed in order to reset built-in caching. Re-configuring the FallbackLocaleProvider directly without going through Translator::setFallbackLocales() may lead to wrong results.

One solution I see would be:
* Deprecate `FallbackLocaleProvider::setFallbackLocales()`, create a new `FallbackLocaleProvider` instance instead. In 7.0, this will leave `FallbackLocaleProvider` immutable.

As FallbackLocaleProvider is a new feature, we do not have to deprecate anything, just change it. IMHO, it's better to design well this feature, to avoid known issues later which makes us support a BC layer.

* Deprecate `Translator::setFallbackLocales()`, use a new `setFallbackLocaleProvider()` method instead that sets the new instance.

Would that be acceptable?

I guess it's acceptable, but we need to provide a BC layer, to let Translator::setFallbackLocales() works until 7.0.

src/Symfony/Component/Translation/CHANGELOG.md

src/Symfony/Component/Translation/LocaleValidator.php

Co-authored-by: Mathieu Santostefano <msantostefano@protonmail.com>

…the fallback locale configuration changes

mpdude · 2022-08-01T08:57:30Z

@welcoMattic and @rvanlaak Thanks for your initial reviews!

I haven't dealt with deprecations and replacement code in Symfony for a long time. Do you think you could help me to get this straight?

The getFallbackLocales() methods on the DataCollectorTranslator and LoggingTranslator are also somewhat strange, since they need to proxy methods that are not part of the TranslatorInterface... maybe this will get easier as well?

mpdude · 2022-08-01T09:00:09Z

I also renamed fallbackLocales to ultimateFallbackLocales to make it more clear that in fact, these are not the only fallbacks – they are the last fallbacks after the ones derived based on ICU data.

mpdude · 2022-08-01T09:02:34Z

src/Symfony/Component/Translation/FallbackLocaleProvider.php

+     */
+    private array $ultimateFallbackLocales;
+
+    private ?array $parentLocales = null;


Should this be static? It is used to cache parsed JSON data, so we might want some extra efficiency.

It could be, I've no strong opinion on it. @nicolas-grekas WDYT?

mpdude · 2022-08-01T09:03:47Z

src/Symfony/Component/Translation/FallbackLocaleProviderInterface.php

+     *
+     * @return string[]
+     */
+    public function getUltimateFallbackLocales(): array;


Given that it is @internal, should it even be part of the interface?

Do you have callees from external? If not, feel free to indeed remove from the interface. If we have calls from outside the class, do we really need those calls because calling internal classes might be considered a violation.

mpdude · 2022-08-01T09:12:12Z

src/Symfony/Component/Translation/Translator.php

+
+        // needed as the fallback locales are linked to the already loaded catalogues
+        $this->catalogues = [];
+        $this->cacheVary['fallback_locales'] = $fallbackLocaleProvider->getUltimateFallbackLocales();


I don't like this for the following reasons:

When, in general, a FallbackLocaleProvider is responsible for giving the sequence of locales to try for a given starting locale, we should not assume it has this list of "last resort" locales, and we should also not assume that this configurable setting is the only thing affecting the computation results.

It might not be relevant in practice, but what if the JSON file with the locale hierarchy changes?

If the FallbackLocaleProvider just provides something like a cache key/suffix or similar (not exposing the ultimateFallbackLocales list), what would that mean for the "fallback locale" information gathered in the DataCollectorTranslator?

I'm not sure to understand your concerns here. What you did seems really similar to what was done before FallbackLocaleProvider introduction.

welcoMattic · 2022-08-01T15:50:37Z

src/Symfony/Component/Translation/DataCollectorTranslator.php

@@ -102,6 +102,8 @@ public function warmUp(string $cacheDir): array
     */
    public function getFallbackLocales(): array
    {
+        trigger_deprecation('symfony/translation', '6.2', 'DataCollectorTranslator::getFallbackLocales() is deprecated. Get the FallbackLocaleProvider instead.');


Suggested change

trigger_deprecation('symfony/translation', '6.2', 'DataCollectorTranslator::getFallbackLocales() is deprecated. Get the FallbackLocaleProvider instead.');

trigger_deprecation('symfony/translation', '6.2', '"%s::getFallbackLocales()" is deprecated, use "%s::getUltimateFallbackLocales()" instead.', DataCollectorTranslator::class, FallbackLocaleProvider::class);

6.2 must be replaced with 6.4 here and in all other trigger_deprecation occurrences.

nicolas-grekas · 2023-03-09T17:42:17Z

What's the status here?

mpdude · 2023-03-10T11:33:21Z

I am still interested in working on it and the use case still exists. Will have to think this through again, forgot about most of the parts. I hope I have all the feedback and information regarding the deprecation steps that I need.

welcoMattic · 2023-09-20T07:25:41Z

src/Symfony/Component/Translation/CHANGELOG.md

@@ -6,6 +6,7 @@ CHANGELOG

 * Parameters implementing `TranslatableInterface` are processed
 * Add the file extension to the `XliffFileDumper` constructor
+ * Add `FallbackLocaleProvider` to configure the fallback locale chain in `Translator`


This branch must be rebased on 6.4 and this line moves to 6.4 section

welcoMattic · 2023-09-20T07:26:20Z

src/Symfony/Component/Translation/DataCollectorTranslator.php

@@ -102,6 +102,8 @@ public function warmUp(string $cacheDir): array
     */
    public function getFallbackLocales(): array
    {
+        trigger_deprecation('symfony/translation', '6.2', 'DataCollectorTranslator::getFallbackLocales() is deprecated. Get the FallbackLocaleProvider instead.');


6.2 must be replaced with 6.4 here and in all other trigger_deprecation occurrences.

welcoMattic · 2023-09-20T07:27:12Z

src/Symfony/Component/Translation/FallbackLocaleProvider.php

+     */
+    private array $ultimateFallbackLocales;
+
+    private ?array $parentLocales = null;


It could be, I've no strong opinion on it. @nicolas-grekas WDYT?

welcoMattic · 2023-09-20T07:31:33Z

src/Symfony/Component/Translation/Translator.php

+
+        // needed as the fallback locales are linked to the already loaded catalogues
+        $this->catalogues = [];
+        $this->cacheVary['fallback_locales'] = $fallbackLocaleProvider->getUltimateFallbackLocales();


I'm not sure to understand your concerns here. What you did seems really similar to what was done before FallbackLocaleProvider introduction.

welcoMattic · 2023-09-20T07:33:46Z

This branch needs to be rebased on 6.4.

Otherwise, I guess we are close to an acceptable feature. I think here we must stick to the existing feature but moved to its own class FallbackLocaleProvider. Then later another PR could be done to improve the fallback locales mechanism if needed.

carsonbot added Status: Needs Review Feature labels Feb 24, 2022

carsonbot added this to the 6.1 milestone Feb 24, 2022

mpdude force-pushed the fallback-locale-provider branch from bb06f51 to 3fabce5 Compare February 25, 2022 09:15

mpdude commented Feb 25, 2022

View reviewed changes

src/Symfony/Component/Translation/Translator.php Outdated Show resolved Hide resolved

rvanlaak approved these changes Feb 26, 2022

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 26, 2022

mpdude force-pushed the fallback-locale-provider branch 3 times, most recently from 7ff72be to 1913770 Compare February 28, 2022 07:49

mpdude added 6 commits February 28, 2022 08:58

Extract locale fallback computation into a dedicated class

aa6a433

Add FallbackLocaleProviderInterface

2afae5e

Move locale validation out of Translator and use it to validate local…

54e6a4a

…es in FallbackLocaleProvider as well

Add a constructor for FallbackLocaleProvider

042757a

Add a CHANGELOG entry

2455bac

Make FrameworkBundle configure FallbackLocaleProvider in the container

532a72c

mpdude force-pushed the fallback-locale-provider branch from 826daa0 to 532a72c Compare February 28, 2022 07:58

mpdude commented Feb 28, 2022

View reviewed changes

mpdude force-pushed the fallback-locale-provider branch from 48bd240 to 9c39534 Compare February 28, 2022 08:07

Try to get away without requirement bumps in FrameworkBundle

47fe72a

mpdude force-pushed the fallback-locale-provider branch from 9c39534 to 47fe72a Compare February 28, 2022 08:34

fabpot modified the milestones: 6.1, 6.2 May 20, 2022

nicolas-grekas added the Translation label Jul 29, 2022

carsonbot changed the title ~~Extract locale fallback computation into a dedicated class~~ [Translation] Extract locale fallback computation into a dedicated class Jul 29, 2022

nicolas-grekas requested a review from welcoMattic July 29, 2022 16:35

carsonbot added Status: Needs Review and removed Status: Reviewed labels Jul 29, 2022

welcoMattic reviewed Jul 29, 2022

View reviewed changes

src/Symfony/Component/Translation/CHANGELOG.md Outdated Show resolved Hide resolved

src/Symfony/Component/Translation/LocaleValidator.php Outdated Show resolved Hide resolved

mpdude and others added 3 commits August 1, 2022 09:28

Update CHANGELOG as suggested in GH Review

0c19e4f

Co-authored-by: Mathieu Santostefano <msantostefano@protonmail.com>

Rename method as suggested in GH Review

c2ac4a2

Make the FallbackLocaleProvider immutable; create new instances when …

ddb9462

…the fallback locale configuration changes

mpdude commented Aug 1, 2022

View reviewed changes

welcoMattic reviewed Aug 1, 2022

View reviewed changes

nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022

nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023

welcoMattic reviewed Sep 20, 2023

View reviewed changes

nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023

xabbuh modified the milestones: 7.1, 7.2 May 15, 2024

fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024

fabpot modified the milestones: 7.3, 7.4 May 26, 2025

	trigger_deprecation('symfony/translation', '6.2', 'DataCollectorTranslator::getFallbackLocales() is deprecated. Get the FallbackLocaleProvider instead.');
	trigger_deprecation('symfony/translation', '6.2', '"%s::getFallbackLocales()" is deprecated, use "%s::getUltimateFallbackLocales()" instead.', DataCollectorTranslator::class, FallbackLocaleProvider::class);

Uh oh!

[Translation] Extract locale fallback computation into a dedicated class #45553

Are you sure you want to change the base?

[Translation] Extract locale fallback computation into a dedicated class #45553

Uh oh!

Conversation

mpdude commented Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

Uh oh!

carsonbot commented Feb 25, 2022

Uh oh!

rvanlaak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpdude commented Feb 28, 2022

Uh oh!

mpdude commented Feb 28, 2022

Uh oh!

mpdude commented Mar 3, 2022

Uh oh!

welcoMattic commented Jul 29, 2022

Uh oh!

Uh oh!

Uh oh!

mpdude commented Aug 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpdude commented Aug 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Mar 9, 2023

Uh oh!

mpdude commented Mar 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

welcoMattic commented Sep 20, 2023

Uh oh!

Uh oh!

mpdude commented Feb 24, 2022 •

edited

Loading

mpdude commented Aug 1, 2022 •

edited

Loading