[FrameworkBundle] Allow to specify a domain when updating translations #19325

antograssiot · 2016-07-09T19:05:22Z

Q	A
Branch?	"master"
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes/
Fixed tickets
License	MIT
Doc PR	no

The MR add the --domain option to the translation:update console command
When working with large number of domains, this helps to reduce the noise in the diff when updating only a translation file.

nicolas-grekas · 2016-07-17T09:42:10Z

src/Symfony/Bundle/FrameworkBundle/Command/TranslationUpdateCommand.php

@@ -203,8 +211,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
            if (!$bundleTransPath) {
                $bundleTransPath = end($transPaths).'translations';
            }
-
-            $writer->writeTranslations($operation->getResult(), $input->getOption('output-format'), array('path' => $bundleTransPath, 'default_locale' => $this->getContainer()->getParameter('kernel.default_locale')));
+            $writer->writeTranslations($operation->getResult($input->getOption('domain')), $input->getOption('output-format'), array('path' => $bundleTransPath, 'default_locale' => $this->getContainer()->getParameter('kernel.default_locale')));


but getResult takes no arguments, or did I miss something?

@nicolas-grekas I added an optional one in src/Symfony/Component/Translation/Catalogue/AbstractOperation.php

antograssiot · 2016-07-17T13:31:20Z

ping @nicolas-grekas I've updated as requested.

nicolas-grekas · 2016-07-17T13:52:50Z

src/Symfony/Component/Translation/Catalogue/AbstractOperation.php

@@ -150,9 +150,13 @@ public function getObsoleteMessages($domain)
    /**
     * {@inheritdoc}
     */
-    public function getResult()
+    public function getResult($domain = null)


I'm not comfortable with this change. The $operation should already have filtered domains.
I see two alternatives: add setDomains() on AbstractOperation, or add a $domains argument to MessageCatalogue to make it filter messages it can contain. A third alt. would be to filter at the loader/extractor level, but they look too specific for this. WDYT?

Or fourth way, maybe the less intrusive one: filter the result itself?

I went with adding setDomains(). but I can still try to update if you want.
Also do you want me to squash the 2 commits ?

antograssiot · 2016-07-17T15:41:25Z

@nicolas-grekas the tests are passing locally but fails with low deps on travis due to composer issue. This doesn't seems related to my change, am I right ?

nicolas-grekas · 2016-07-17T15:42:51Z

src/Symfony/Bundle/FrameworkBundle/Command/TranslationUpdateCommand.php

        // Exit if no messages found.
-        if (!count($operation->getDomains())) {
+        if (!count($domains)) {


Change not needed anymore, same below

that was still to avoid some function call but I can remove it

nicolas-grekas · 2016-07-17T15:46:45Z

FrameworkBundle now needs Translation 3.2, that's why lowest tests are failing

nicolas-grekas · 2016-07-17T15:49:02Z

src/Symfony/Bundle/FrameworkBundle/Command/TranslationUpdateCommand.php

+        $domain = $input->getOption('domain');
+
+        if (null !== $domain) {
+            $domains = array_intersect($domains, array($domain));


I don't think we need this line: when the option is set, let's call setDomains with it

I may be wrong though here :)

I'll leave the array_intersect() because if you use for example --domain nonexisting
Then you'll get an empty array when calling getDomains() and the command will exit (L155-158)

antograssiot · 2016-07-17T15:55:15Z

@nicolas-grekas thanks for your explanation on the Travis build and your review.
It should now be limited to the minimum changes.

nicolas-grekas · 2016-07-17T18:08:56Z

Looks like your target can be achieved with changing the translation component:
master...nicolas-grekas:console-translation-domain-choice
Or do you miss a test case?

antograssiot · 2016-07-17T18:25:51Z

Nicolas I'm sorry I don't get your comment. You tm branch is not up to date with mine but I tested it again in an app and it seems to work as expected.

nicolas-grekas · 2016-07-17T18:28:37Z

@antograssiot if you're fine with my patch, then make it yours. If we don't need to change anything in the translation component, it's better.

antograssiot · 2016-07-17T20:24:49Z

@nicolas-grekas I now understood your first comment. Yes I'm missing a test case.
Your patch will only work when using the --dump-messages flag but when using --force all files will still get generated, that's how I came to modify the AbstractOperation.

A test proving that only one file is generated is missing. I tried to wrote one but I failed and I couldn't find any existing test that could guide me. $this->translationDir.'/Resources/translations seems to always stay empty.

The only solution that I can see without updating the translation component would be to find a way to filter $operation->getResult() fro TranslationUpdateCommand.phpbut I don't know how.

nicolas-grekas · 2016-07-18T04:31:59Z

@antograssiot see updated patch at master...nicolas-grekas:console-translation-domain-choice

antograssiot · 2016-07-18T04:55:31Z

@nicolas-grekas It works fine in my case but I'm concerned about MessageCatalogue::addMetadata() being a private function.
Thanks for your work on patching this anyway, should I close my PR ?

nicolas-grekas · 2016-07-18T05:02:33Z

I didn't spot the method was private, fixed. No need to close this PR, just make my patch yours by doing e.g.:

git checkout console-translation-domain-choice
git fetch https://github.com/nicolas-grekas/symfony.git console-translation-domain-choice
git reset --hard FETCH_HEAD
git push -f origin console-translation-domain-choice

antograssiot · 2016-07-18T05:26:08Z

PR updated. Thank you very much @nicolas-grekas for your help and patience

nicolas-grekas · 2016-07-18T05:32:56Z

👍

fabpot · 2016-07-18T09:47:17Z

Thank you @antograssiot.

…ing translations (antograssiot) This PR was merged into the 3.2-dev branch. Discussion ---------- [FrameworkBundle] Allow to specify a domain when updating translations | Q | A | ------------- | --- | Branch? | "master" | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes/ | Fixed tickets | | License | MIT | Doc PR | no The MR add the `--domain` option to the `translation:update` console command When working with large number of domains, this helps to reduce the noise in the diff when updating only a translation file. Commits ------- a8f3a93 [FrameworkBundle] Allow to specify a domain when updating translations

… a merge (jakzal) This PR was merged into the 3.2-dev branch. Discussion ---------- [FrameworkBundle] Fix TranslationUpdateCommand tests after a merge | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19921 | License | MIT | Doc PR | - #19325 relied on a wrong count of messages, which was fixed by #19878. Additionally, the `getContainer()` method on the master branch expect messages to be indexed by domain. Commits ------- d093c40 [FrameworkBundle] Fix TranslationUpdateCommand tests after a merge

carsonbot added Status: Needs Review Console FrameworkBundle Feature labels Jul 9, 2016

nicolas-grekas reviewed Jul 17, 2016
View reviewed changes

nicolas-grekas changed the title ~~[Console] [FrameworkBundle] Allow to specify a domain when updating translation~~ [FrameworkBundle] Allow to specify a domain when updating translations Jul 17, 2016

[FrameworkBundle] Allow to specify a domain when updating translations

a8f3a93

fabpot merged commit a8f3a93 into symfony:master Jul 18, 2016

This was referenced Sep 13, 2016

TranslationUpdateCommand test failure on master #19921

Closed

[FrameworkBundle] Fix TranslationUpdateCommand tests after a merge #19926

Merged

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle] Allow to specify a domain when updating translations #19325

[FrameworkBundle] Allow to specify a domain when updating translations #19325

antograssiot commented Jul 9, 2016 •

edited

Loading

nicolas-grekas Jul 17, 2016

antograssiot Jul 17, 2016

antograssiot commented Jul 17, 2016

nicolas-grekas Jul 17, 2016 •

edited

Loading

nicolas-grekas Jul 17, 2016

antograssiot Jul 17, 2016

antograssiot commented Jul 17, 2016

nicolas-grekas Jul 17, 2016

antograssiot Jul 17, 2016 •

edited

Loading

nicolas-grekas commented Jul 17, 2016

nicolas-grekas Jul 17, 2016

nicolas-grekas Jul 17, 2016

antograssiot Jul 17, 2016 •

edited

Loading

antograssiot commented Jul 17, 2016

nicolas-grekas commented Jul 17, 2016

antograssiot commented Jul 17, 2016

nicolas-grekas commented Jul 17, 2016 •

edited

Loading

antograssiot commented Jul 17, 2016 •

edited

Loading

nicolas-grekas commented Jul 18, 2016

antograssiot commented Jul 18, 2016

nicolas-grekas commented Jul 18, 2016 •

edited

Loading

antograssiot commented Jul 18, 2016

nicolas-grekas commented Jul 18, 2016

fabpot commented Jul 18, 2016

[FrameworkBundle] Allow to specify a domain when updating translations #19325

[FrameworkBundle] Allow to specify a domain when updating translations #19325

Conversation

antograssiot commented Jul 9, 2016 • edited Loading

nicolas-grekas Jul 17, 2016

Choose a reason for hiding this comment

antograssiot Jul 17, 2016

Choose a reason for hiding this comment

antograssiot commented Jul 17, 2016

nicolas-grekas Jul 17, 2016 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Jul 17, 2016

Choose a reason for hiding this comment

antograssiot Jul 17, 2016

Choose a reason for hiding this comment

antograssiot commented Jul 17, 2016

nicolas-grekas Jul 17, 2016

Choose a reason for hiding this comment

antograssiot Jul 17, 2016 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Jul 17, 2016

nicolas-grekas Jul 17, 2016

Choose a reason for hiding this comment

nicolas-grekas Jul 17, 2016

Choose a reason for hiding this comment

antograssiot Jul 17, 2016 • edited Loading

Choose a reason for hiding this comment

antograssiot commented Jul 17, 2016

nicolas-grekas commented Jul 17, 2016

antograssiot commented Jul 17, 2016

nicolas-grekas commented Jul 17, 2016 • edited Loading

antograssiot commented Jul 17, 2016 • edited Loading

nicolas-grekas commented Jul 18, 2016

antograssiot commented Jul 18, 2016

nicolas-grekas commented Jul 18, 2016 • edited Loading

antograssiot commented Jul 18, 2016

nicolas-grekas commented Jul 18, 2016

fabpot commented Jul 18, 2016

antograssiot commented Jul 9, 2016 •

edited

Loading

nicolas-grekas Jul 17, 2016 •

edited

Loading

antograssiot Jul 17, 2016 •

edited

Loading

antograssiot Jul 17, 2016 •

edited

Loading

nicolas-grekas commented Jul 17, 2016 •

edited

Loading

antograssiot commented Jul 17, 2016 •

edited

Loading

nicolas-grekas commented Jul 18, 2016 •

edited

Loading