NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Translation] Fix default value for locales in translation push|pull commands#41504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:5.3fromwelcoMattic:fix/translation-providers-locales-config

Jul 25, 2021

Merged

[Translation] Fix default value for locales in translation push|pull commands#41504

fabpot merged 1 commit intosymfony:5.3fromwelcoMattic:fix/translation-providers-locales-config

Jul 25, 2021

Conversation

Copy link

Member

welcoMattic commentedJun 1, 2021

Q	A
Branch?	5.3
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix#41501
License	MIT
Doc PR

This PR ensures that$locales are always defined eitherframework.translator.enabled_locales orframework.translator.providers.(loco|lokalise|crowdin).locales is defined. Until now, the code expectedframework.translator.enabled_locales to be defined, now if it's not, we merge allframework.translator.providers.(loco|lokalise|crowdin).locales values and pass them toconsole.command.translation_pull andconsole.command.translation_push definitions.

carsonbot added Status: Needs Review Bug Translation labels

Jun 1, 2021

DevScyu reviewed

Jun 2, 2021

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Outdated

		thrownewLogicException(sprintf('You must specify one of "framework.translator.enabled_locales" or "framework.translator.providers.%s.locales" in order to use translation providers.',$name));
		}

		$locales =array_merge($locales,$provider['locales']);

Copy link

DevScyuJun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Is array_merge really the intended way here? Shouldn't the locale options for the provider be what controls what locales get sent to the provider, even if we have more locales enabled?

Copy link

MemberAuthor

welcoMatticJun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Actually we want to pass to the commands all locales supported by all configured providers, in order to read local translations. That's why thelocales property of theTranslationPushCommand hasenabledLocales as default value (https://github.com/symfony/symfony/pull/41504/files#diff-a00f86baf85c337ee61148e5fff8d189edf224ef40382518307359f579f40e6aR66).

Then, theFilteringProvider class is a decorator for each actual provider, and in itsread method, we intersect domains and locales : the ones come from the provider configurations, the others come from theread call from the command class.

But I'm ok, we need to review this part, to make it more explicit and more logical. One of the solution might be to use only locales of providers configuration, make this parameter mandatory? WDYT@nicolas-grekas, in terms of design?

Copy link

Member

nicolas-grekasJun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I've no strong opinion. I'm fine with the current design.
But if someone disagrees, I'd be happy to know why.

Copy link

MemberAuthor

welcoMatticJun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Are@OskarStark or@Nyholm has an opinion on it?

Copy link

MemberAuthor

welcoMatticJul 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

ping@OskarStark @Nyholm

nicolas-grekas changed the base branch from5.4 to5.3

June 2, 2021 09:38

nicolas-grekas added this to the5.3 milestone

Jun 2, 2021

nicolas-grekas reviewed

Jun 2, 2021

View reviewed changes

src/Symfony/Component/Translation/Command/TranslationPushCommand.phpShow resolvedHide resolved

Copy link

MemberAuthor

welcoMattic commentedJun 5, 2021

I just figured out there no relevant reason thatenabled_locales was related to the locales passed to the push and pull commands. In fact, someone could need to pull/push translation for a locales which is not (yet) enabled.

WDTY about not set locales for Providers fromenabled_locales?

Copy link

Member

nicolas-grekas commentedJun 8, 2021

It's true that this line might be strange:
https://github.com/welcoMattic/symfony/blob/94a451dcd1dd44a20a1b8038d1a12b80255460f6/src/Symfony/Component/Translation/Command/TranslationPushCommand.php#L100

I don't see how the 2nd DI parameter that ismentionned in the message relates to the command.
Maybe the command should default to the list of locales of each provider? Is that doable?

Copy link

MemberAuthor

welcoMattic commentedJun 14, 2021•
edited
Loading

It's pretty much what I thought to do, and make the following behaviour:

A provider will treat all locales (framework.translator.enabled_locales) by default, unless we pass explicit locales via CLI options.

Are we agree on it?

Fixed default value for locales in translation push|pull commands

375b058

welcoMattic force-pushed thefix/translation-providers-locales-config branch from94a451d to375b058Compare

July 25, 2021 09:40

Copy link

MemberAuthor

welcoMattic commentedJul 25, 2021

I've just pushed an update to useenabled_locales by default, and append allframework.translator.providers.(loco|lokalise|crowdin).locales values, thenarray_unique the result. It will allow users to defined a common list of locales for push/pull commands, and specify additional locales (for instance, a not yet enabled locale) for one or more provider.