Make trans + %count% parameter resolve plurals

symfony · Oct 6, 2018 · 036c679 · 036c679
1 parent d1cc2a7
commit 036c679
Show file tree

Hide file tree

Showing 8 changed files with 159 additions and 32 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,8 +4,9 @@ CHANGELOG
 4.2.0
 -----
 
-* add bundle name suggestion on wrongly overridden templates paths
-* added `name` argument in `debug:twig` command and changed `filter` argument as `--filter` option
+ * add bundle name suggestion on wrongly overridden templates paths
+ * added `name` argument in `debug:twig` command and changed `filter` argument as `--filter` option
+ * deprecated the `transchoice` tag and filter, use the `trans` ones instead with a `%count%` parameter
 
 4.1.0
 -----

diff --git a/Extension/TranslationExtension.php b/Extension/TranslationExtension.php
@@ -16,7 +16,6 @@
 use Symfony\Bridge\Twig\TokenParser\TransChoiceTokenParser;
 use Symfony\Bridge\Twig\TokenParser\TransDefaultDomainTokenParser;
 use Symfony\Bridge\Twig\TokenParser\TransTokenParser;
-use Symfony\Component\Translation\LegacyTranslatorTrait;
 use Symfony\Component\Translation\TranslatorInterface as LegacyTranslatorInterface;
 use Symfony\Contracts\Translation\TranslatorInterface;
 use Symfony\Contracts\Translation\TranslatorTrait;
@@ -29,6 +28,8 @@
  * Provides integration of the Translation component with Twig.
  *
  * @author Fabien Potencier <fabien@symfony.com>
+ *
+ * @final since Symfony 4.2
  */
 class TranslationExtension extends AbstractExtension
 {
@@ -38,21 +39,28 @@ class TranslationExtension extends AbstractExtension
         trans as private doTrans;
     }
 
-    use LegacyTranslatorTrait {
-        transChoice as private doTransChoice;
-    }
-
     private $translator;
     private $translationNodeVisitor;
 
-    public function __construct(TranslatorInterface $translator = null, NodeVisitorInterface $translationNodeVisitor = null)
+    /**
+     * @param TranslatorInterface|null $translator
+     */
+    public function __construct($translator = null, NodeVisitorInterface $translationNodeVisitor = null)
     {
+        if (null !== $translator && !$translator instanceof LegacyTranslatorInterface && !$translator instanceof TranslatorInterface) {
+            throw new \TypeError(sprintf('Argument 1 passed to %s() must be an instance of %s, %s given.', __METHOD__, TranslatorInterface::class, \is_object($translator) ? \get_class($translator) : \gettype($translator)));
+        }
         $this->translator = $translator;
         $this->translationNodeVisitor = $translationNodeVisitor;
     }
 
+    /**
+     * @deprecated since Symfony 4.2
+     */
     public function getTranslator()
     {
+        @trigger_error(sprintf('The "%s()" method is deprecated since Symfony 4.2.', __METHOD__), E_USER_DEPRECATED);
+
         return $this->translator;
     }
 
@@ -63,7 +71,7 @@ public function getFilters()
     {
         return array(
             new TwigFilter('trans', array($this, 'trans')),
-            new TwigFilter('transchoice', array($this, 'transchoice')),
+            new TwigFilter('transchoice', array($this, 'transchoice'), array('deprecated' => '4.2', 'alternative' => 'trans" with parameter "%count%')),
         );
     }
 
@@ -101,19 +109,28 @@ public function getTranslationNodeVisitor()
         return $this->translationNodeVisitor ?: $this->translationNodeVisitor = new TranslationNodeVisitor();
     }
 
-    public function trans($message, array $arguments = array(), $domain = null, $locale = null)
+    public function trans($message, array $arguments = array(), $domain = null, $locale = null, $count = null)
     {
+        if (null !== $count) {
+            $arguments['%count%'] = $count;
+        }
         if (null === $this->translator) {
             return $this->doTrans($message, $arguments, $domain, $locale);
         }
 
         return $this->translator->trans($message, $arguments, $domain, $locale);
     }
 
+    /**
+     * @deprecated since Symfony 4.2, use the trans() method instead with a %count% parameter
+     */
     public function transchoice($message, $count, array $arguments = array(), $domain = null, $locale = null)
     {
-        if (null === $this->translator || !$this->translator instanceof LegacyTranslatorInterface) {
-            return $this->doTransChoice($message, $count, array_merge(array('%count%' => $count), $arguments), $domain, $locale);
+        if (null === $this->translator) {
+            return $this->doTrans($message, array_merge(array('%count%' => $count), $arguments), $domain, $locale);
+        }
+        if ($this->translator instanceof TranslatorInterface) {
+            return $this->translator->trans($message, array_merge(array('%count%' => $count), $arguments), $domain, $locale);
         }
 
         return $this->translator->transChoice($message, $count, array_merge(array('%count%' => $count), $arguments), $domain, $locale);

diff --git a/Node/TransNode.php b/Node/TransNode.php
@@ -60,19 +60,12 @@ public function compile(Compiler $compiler)
         $method = !$this->hasNode('count') ? 'trans' : 'transChoice';
 
         $compiler
-            ->write('echo $this->env->getExtension(\'Symfony\Bridge\Twig\Extension\TranslationExtension\')->getTranslator()->'.$method.'(')
+            ->write('echo $this->env->getExtension(\'Symfony\Bridge\Twig\Extension\TranslationExtension\')->trans(')
             ->subcompile($msg)
         ;
 
         $compiler->raw(', ');
 
-        if ($this->hasNode('count')) {
-            $compiler
-                ->subcompile($this->getNode('count'))
-                ->raw(', ')
-            ;
-        }
-
         if (null !== $vars) {
             $compiler
                 ->raw('array_merge(')
@@ -98,7 +91,17 @@ public function compile(Compiler $compiler)
                 ->raw(', ')
                 ->subcompile($this->getNode('locale'))
             ;
+        } elseif ($this->hasNode('count')) {
+            $compiler->raw(', null');
         }
+
+        if ($this->hasNode('count')) {
+            $compiler
+                ->raw(', ')
+                ->subcompile($this->getNode('count'))
+            ;
+        }
+
         $compiler->raw(");\n");
     }
 

diff --git a/Tests/Extension/TranslationExtensionTest.php b/Tests/Extension/TranslationExtensionTest.php
@@ -45,6 +45,15 @@ public function testTrans($template, $expected, array $variables = array())
         $this->assertEquals($expected, $this->getTemplate($template)->render($variables));
     }
 
+    /**
+     * @group legacy
+     * @dataProvider getTransChoiceTests
+     */
+    public function testTransChoice($template, $expected, array $variables = array())
+    {
+        $this->testTrans($template, $expected, $variables);
+    }
+
     /**
      * @expectedException        \Twig\Error\SyntaxError
      * @expectedExceptionMessage Unexpected token. Twig was looking for the "with", "from", or "into" keyword in "index" at line 3.
@@ -64,6 +73,7 @@ public function testTransComplexBody()
     }
 
     /**
+     * @group legacy
      * @expectedException        \Twig\Error\SyntaxError
      * @expectedExceptionMessage A message inside a transchoice tag must be a simple text in "index" at line 2.
      */
@@ -87,6 +97,69 @@ public function getTransTests()
 
             array('{% trans into "fr"%}Hello{% endtrans %}', 'Hello'),
 
+            // trans with count
+            array(
+                '{% trans from "messages" %}{0} There is no apples|{1} There is one apple|]1,Inf] There is %count% apples{% endtrans %}',
+                'There is no apples',
+                array('count' => 0),
+            ),
+            array(
+                '{% trans %}{0} There is no apples|{1} There is one apple|]1,Inf] There is %count% apples{% endtrans %}',
+                'There is 5 apples',
+                array('count' => 5),
+            ),
+            array(
+                '{% trans %}{0} There is no apples|{1} There is one apple|]1,Inf] There is %count% apples (%name%){% endtrans %}',
+                'There is 5 apples (Symfony)',
+                array('count' => 5, 'name' => 'Symfony'),
+            ),
+            array(
+                '{% trans with { \'%name%\': \'Symfony\' } %}{0} There is no apples|{1} There is one apple|]1,Inf] There is %count% apples (%name%){% endtrans %}',
+                'There is 5 apples (Symfony)',
+                array('count' => 5),
+            ),
+            array(
+                '{% trans into "fr"%}{0} There is no apples|{1} There is one apple|]1,Inf] There is %count% apples{% endtrans %}',
+                'There is no apples',
+                array('count' => 0),
+            ),
+            array(
+                '{% trans count 5 into "fr"%}{0} There is no apples|{1} There is one apple|]1,Inf] There is %count% apples{% endtrans %}',
+                'There is 5 apples',
+            ),
+
+            // trans filter
+            array('{{ "Hello"|trans }}', 'Hello'),
+            array('{{ name|trans }}', 'Symfony', array('name' => 'Symfony')),
+            array('{{ hello|trans({ \'%name%\': \'Symfony\' }) }}', 'Hello Symfony', array('hello' => 'Hello %name%')),
+            array('{% set vars = { \'%name%\': \'Symfony\' } %}{{ hello|trans(vars) }}', 'Hello Symfony', array('hello' => 'Hello %name%')),
+            array('{{ "Hello"|trans({}, "messages", "fr") }}', 'Hello'),
+
+            // trans filter with count
+            array('{{ "{0} There is no apples|{1} There is one apple|]1,Inf] There is %count% apples"|trans(count=count) }}', 'There is 5 apples', array('count' => 5)),
+            array('{{ text|trans(count=5, arguments={\'%name%\': \'Symfony\'}) }}', 'There is 5 apples (Symfony)', array('text' => '{0} There is no apples|{1} There is one apple|]1,Inf] There is %count% apples (%name%)')),
+            array('{{ "{0} There is no apples|{1} There is one apple|]1,Inf] There is %count% apples"|trans({}, "messages", "fr", count) }}', 'There is 5 apples', array('count' => 5)),
+        );
+    }
+
+    /**
+     * @group legacy
+     */
+    public function getTransChoiceTests()
+    {
+        return array(
+            // trans tag
+            array('{% trans %}Hello{% endtrans %}', 'Hello'),
+            array('{% trans %}%name%{% endtrans %}', 'Symfony', array('name' => 'Symfony')),
+
+            array('{% trans from elsewhere %}Hello{% endtrans %}', 'Hello'),
+
+            array('{% trans %}Hello %name%{% endtrans %}', 'Hello Symfony', array('name' => 'Symfony')),
+            array('{% trans with { \'%name%\': \'Symfony\' } %}Hello %name%{% endtrans %}', 'Hello Symfony'),
+            array('{% set vars = { \'%name%\': \'Symfony\' } %}{% trans with vars %}Hello %name%{% endtrans %}', 'Hello Symfony'),
+
+            array('{% trans into "fr"%}Hello{% endtrans %}', 'Hello'),
+
             // transchoice
             array(
                 '{% transchoice count from "messages" %}{0} There is no apples|{1} There is one apple|]1,Inf] There is %count% apples{% endtranschoice %}',
@@ -145,8 +218,8 @@ public function testDefaultTranslationDomain()
                     {%- trans from "custom" %}foo{% endtrans %}
                     {{- "foo"|trans }}
                     {{- "foo"|trans({}, "custom") }}
-                    {{- "foo"|transchoice(1) }}
-                    {{- "foo"|transchoice(1, {}, "custom") }}
+                    {{- "foo"|trans(count=1) }}
+                    {{- "foo"|trans({"%count%":1}, "custom") }}
                 {% endblock %}
             ',
 
@@ -174,12 +247,12 @@ public function testDefaultTranslationDomainWithNamedArguments()
 
                 {%- block content %}
                     {{- "foo"|trans(arguments = {}, domain = "custom") }}
-                    {{- "foo"|transchoice(count = 1) }}
-                    {{- "foo"|transchoice(count = 1, arguments = {}, domain = "custom") }}
+                    {{- "foo"|trans(count = 1) }}
+                    {{- "foo"|trans(count = 1, arguments = {}, domain = "custom") }}
                     {{- "foo"|trans({}, domain = "custom") }}
                     {{- "foo"|trans({}, "custom", locale = "fr") }}
-                    {{- "foo"|transchoice(1, arguments = {}, domain = "custom") }}
-                    {{- "foo"|transchoice(1, {}, "custom", locale = "fr") }}
+                    {{- "foo"|trans(arguments = {"%count%":1}, domain = "custom") }}
+                    {{- "foo"|trans({"%count%":1}, "custom", locale = "fr") }}
                 {% endblock %}
             ',
 

diff --git a/Tests/Node/TransNodeTest.php b/Tests/Node/TransNodeTest.php
@@ -34,7 +34,7 @@ public function testCompileStrict()
 
         $this->assertEquals(
             sprintf(
-                'echo $this->env->getExtension(\'Symfony\Bridge\Twig\Extension\TranslationExtension\')->getTranslator()->trans("trans %%var%%", array_merge(array("%%var%%" => %s), %s), "messages");',
+                'echo $this->env->getExtension(\'Symfony\Bridge\Twig\Extension\TranslationExtension\')->trans("trans %%var%%", array_merge(array("%%var%%" => %s), %s), "messages");',
                 $this->getVariableGetterWithoutStrictCheck('var'),
                 $this->getVariableGetterWithStrictCheck('foo')
              ),

diff --git a/Tests/Translation/TwigExtractorTest.php b/Tests/Translation/TwigExtractorTest.php
@@ -50,15 +50,21 @@ public function testExtract($template, $messages)
         }
     }
 
+    /**
+     * @group legacy
+     * @dataProvider getLegacyExtractData
+     */
+    public function testLegacyExtract($template, $messages)
+    {
+        $this->testExtract($template, $messages);
+    }
+
     public function getExtractData()
     {
         return array(
             array('{{ "new key" | trans() }}', array('new key' => 'messages')),
             array('{{ "new key" | trans() | upper }}', array('new key' => 'messages')),
             array('{{ "new key" | trans({}, "domain") }}', array('new key' => 'domain')),
-            array('{{ "new key" | transchoice(1) }}', array('new key' => 'messages')),
-            array('{{ "new key" | transchoice(1) | upper }}', array('new key' => 'messages')),
-            array('{{ "new key" | transchoice(1, {}, "domain") }}', array('new key' => 'domain')),
             array('{% trans %}new key{% endtrans %}', array('new key' => 'messages')),
             array('{% trans %}  new key  {% endtrans %}', array('new key' => 'messages')),
             array('{% trans from "domain" %}new key{% endtrans %}', array('new key' => 'domain')),
@@ -67,11 +73,27 @@ public function getExtractData()
 
             // make sure 'trans_default_domain' tag is supported
             array('{% trans_default_domain "domain" %}{{ "new key"|trans }}', array('new key' => 'domain')),
-            array('{% trans_default_domain "domain" %}{{ "new key"|transchoice }}', array('new key' => 'domain')),
             array('{% trans_default_domain "domain" %}{% trans %}new key{% endtrans %}', array('new key' => 'domain')),
 
             // make sure this works with twig's named arguments
             array('{{ "new key" | trans(domain="domain") }}', array('new key' => 'domain')),
+        );
+    }
+
+    /**
+     * @group legacy
+     */
+    public function getLegacyExtractData()
+    {
+        return array(
+            array('{{ "new key" | transchoice(1) }}', array('new key' => 'messages')),
+            array('{{ "new key" | transchoice(1) | upper }}', array('new key' => 'messages')),
+            array('{{ "new key" | transchoice(1, {}, "domain") }}', array('new key' => 'domain')),
+
+            // make sure 'trans_default_domain' tag is supported
+            array('{% trans_default_domain "domain" %}{{ "new key"|transchoice }}', array('new key' => 'domain')),
+
+            // make sure this works with twig's named arguments
             array('{{ "new key" | transchoice(domain="domain", count=1) }}', array('new key' => 'domain')),
         );
     }

diff --git a/TokenParser/TransChoiceTokenParser.php b/TokenParser/TransChoiceTokenParser.php
@@ -23,6 +23,8 @@
  * Token Parser for the 'transchoice' tag.
  *
  * @author Fabien Potencier <fabien@symfony.com>
+ *
+ * @deprecated since Symfony 4.2, use the "trans" tag with a "%count%" parameter instead
  */
 class TransChoiceTokenParser extends TransTokenParser
 {
@@ -38,6 +40,8 @@ public function parse(Token $token)
         $lineno = $token->getLine();
         $stream = $this->parser->getStream();
 
+        @trigger_error(sprintf('The "transchoice" tag is deprecated since Symfony 4.2, use the "trans" one instead with a "%count%" parameter in %s line %d.', $stream->getSourceContext()->getName(), $lineno), E_USER_DEPRECATED);
+
         $vars = new ArrayExpression(array(), $lineno);
 
         $count = $this->parser->getExpressionParser()->parseExpression();

diff --git a/TokenParser/TransTokenParser.php b/TokenParser/TransTokenParser.php
@@ -39,10 +39,17 @@ public function parse(Token $token)
         $lineno = $token->getLine();
         $stream = $this->parser->getStream();
 
+        $count = null;
         $vars = new ArrayExpression(array(), $lineno);
         $domain = null;
         $locale = null;
         if (!$stream->test(Token::BLOCK_END_TYPE)) {
+            if ($stream->test('count')) {
+                // {% trans count 5 %}
+                $stream->next();
+                $count = $this->parser->getExpressionParser()->parseExpression();
+            }
+
             if ($stream->test('with')) {
                 // {% trans with vars %}
                 $stream->next();
@@ -74,7 +81,7 @@ public function parse(Token $token)
 
         $stream->expect(Token::BLOCK_END_TYPE);
 
-        return new TransNode($body, $domain, null, $vars, $locale, $lineno, $this->getTag());
+        return new TransNode($body, $domain, $count, $vars, $locale, $lineno, $this->getTag());
     }
 
     public function decideTransFork($token)