Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Translation] Allow usage of Provider domains if possible#45171
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
[Translation] Allow usage of Provider domains if possible#45171
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedJan 26, 2022
Hey! I think@rvanlaak has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
src/Symfony/Component/Translation/Command/TranslationPushCommand.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
fabpot commentedMar 26, 2022
@welcoMattic Do you have time to answer Nicolas's question? Is it ready to be merged then? |
ossinkine commentedApr 2, 2022
Thank you for MR! I suggest to merge it to 5.4 but not to 6.1 because actually this can be considered as a fix for more obvious behavior. I've also been surprised when discovered the domains are got not from config. |
6e02668 to2533821ComparewelcoMattic commentedApr 6, 2022
@fabpot I fixed@nicolas-grekas comment, it's ready to merge now, sorry for the delay @ossinkine we must consider it as a feature, because nothing is properly broken here |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks like a bugfix to me as there is no way to opt-out of the proposed behavior. If it's a feature, it's a behavioral BC break then, which would be no-go ;)
2533821 to274625fComparewelcoMattic commentedApr 12, 2022
According to discussion with@nicolas-grekas, this PR is a bugfix. It's rebased on 5.4 |
274625f tob752653Comparenicolas-grekas commentedApr 12, 2022
Thank you@welcoMattic. |
plfort commentedMay 1, 2022
It seems the problem still persist. $domains is set from the $provider->getDomains() after reading local translations for all domains (if we don't pass 'domains' command option). Maybe we can handle this issue with something like this ? $domains =$domainsInput =$input->getOption('domains');if ($providerinstanceof FilteringProvider) {$providerDomains =$provider->getDomains();if ($providerDomains) {$domains =$domains ?array_intersect($domains,$providerDomains) :$providerDomains;if (!$domains) {$io->error(sprintf('The provider is configured for this domains: "%s", "%s" provided',implode(',',$providerDomains),implode(',',$domainsInput) ));return Command::FAILURE; } }}$localTranslations =$this->readLocalTranslations($locales,$domains,$this->transPaths);if (!$domains) {$domains =$this->getDomainsFromTranslatorBag($localTranslations);} |
plfort commentedMay 1, 2022
Or maybe we should handle the case in FilteringProvider::write (and FilteringProvider::delete ?), it seems more logical to filter domains here |
… the provider has domains (Florian-B)This PR was merged into the 5.4 branch.Discussion----------[Translation] Refresh local translations on PushCommand if the provider has domains| Q | A| ------------- | ---| Branch? | 5.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Tickets | no| License | MIT| Doc PR | noSince#45171 it's possible to define targeted translation domains in configuration.On `translation:push` without `--domains` option the command should read our configuration in order build local translations list.If the domain is only define in the configuration we need to refresh the local translations list to prevent full update of Provider (push all domains without filtering).I think we should add a test but I failed to do it properly.Commits-------8b0f7eb [Translation] Refresh local translations if the provider has domains
Uh oh!
There was an error while loading.Please reload this page.
This PR adds possibility for
translation:pushcommand to use the configured domains value for a provider, if there is no--domainsoption provided.Before that, the command tries to get the domains from the
MessageCatalogof all local translations (includingvalidatorsandsecurityeven if the user doesn't override these files).Now, we can configure
domainsintranslation.yaml:And
bin/console translation:push lokalisewill take['messages']as domains value, instead of['messages', 'validators', 'security'].