[Translation] Refresh local translations on PushCommand if the provider has domains #46239
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
| @@ -136,6 +136,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int | |||
| if (!$domains) { | |||
| if ($provider instanceof FilteringProvider) { | |||
| $domains = $provider->getDomains(); | |||
| $localTranslations = $this->readLocalTranslations($locales, $domains, $this->transPaths); | |||
There was a problem hiding this comment.
I guess it's better to not refresh translations, but read them after the main if (!$domains) { block
There was a problem hiding this comment.
This was my first idea, but the variable $localTranslations is used in the if (!$domains) { block at line 143
We can also rewrite the if (!$domains) { block a bit like this :
if (!$domains && $provider instanceof FilteringProvider) {
$domains = $provider->getDomains();
}
$localTranslations = $this->readLocalTranslations($locales, $domains, $this->transPaths);
if (!$domains) {
$domains = $this->getDomainsFromTranslatorBag($localTranslations);
}c9d787d to
d4d0bb2
Compare
|
If there is no relevant way to add a test for this, can we at least put a phpdoc comment to ease catching regressions? |
d4d0bb2 to
8b0f7eb
Compare
Done 👍 |
|
Good catch, thanks @Florian-B. |
Since #45171 it's possible to define targeted translation domains in configuration.
On
translation:pushwithout--domainsoption 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.