[Translation] Fix default value for locales in translation push|pull commands #41504
Conversation
| throw new LogicException(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']); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Actually we want to pass to the commands all locales supported by all configured providers, in order to read local translations. That's why the locales property of the TranslationPushCommand has enabledLocales as default value (https://github.com/symfony/symfony/pull/41504/files#diff-a00f86baf85c337ee61148e5fff8d189edf224ef40382518307359f579f40e6aR66).
Then, the FilteringProvider class is a decorator for each actual provider, and in its read method, we intersect domains and locales : the ones come from the provider configurations, the others come from the read 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?
There was a problem hiding this comment.
I've no strong opinion. I'm fine with the current design.
But if someone disagrees, I'd be happy to know why.
There was a problem hiding this comment.
Are @OskarStark or @Nyholm has an opinion on it?
There was a problem hiding this comment.
ping @OskarStark @Nyholm
|
I just figured out there no relevant reason that WDTY about not set locales for Providers from |
|
It's true that this line might be strange: I don't see how the 2nd DI parameter that ismentionned in the message relates to the command. |
|
It's pretty much what I thought to do, and make the following behaviour: A provider will treat all locales ( Are we agree on it? |
94a451d to
375b058
Compare
|
I've just pushed an update to use |
|
Thank you @welcoMattic. |
This PR ensures that
$localesare always defined eitherframework.translator.enabled_localesorframework.translator.providers.(loco|lokalise|crowdin).localesis defined. Until now, the code expectedframework.translator.enabled_localesto be defined, now if it's not, we merge allframework.translator.providers.(loco|lokalise|crowdin).localesvalues and pass them toconsole.command.translation_pullandconsole.command.translation_pushdefinitions.