minor #33329 [Translator] Nullable message id? (yceruto)

This PR was merged into the 4.4 branch. Discussion ---------- [Translator] Nullable message id? | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - https://github.com/symfony/symfony/blob/610a4e978f081eba3ba34f4b48181711f5eedfd2/src/Symfony/Component/Translation/DataCollectorTranslator.php#L144 The message id shouldn't be `null`, but it's breaking the current code now. Shouldn't we first deprecate of passing `null` even if it's well documented? Out there can be a lot of `->trans($var)` and `var|trans()` (like the current ones fixed here) that will break without previous warning. WDTY? Commits ------- 55eac63 Nullable message id?
symfony · Sep 3, 2019 · 9690562 · 9690562
2 parents e69336f + 55eac63
commit 9690562
Show file tree

Hide file tree

Showing 6 changed files with 28 additions and 6 deletions.
diff --git a/src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_4_layout.html.twig b/src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_4_layout.html.twig
@@ -124,7 +124,7 @@
         {{- block('form_widget_simple') -}}
         {%- set label_attr = label_attr|merge({ class: (label_attr.class|default('') ~ ' custom-file-label')|trim }) -%}
         <label for="{{ form.vars.id }}" {% with { attr: label_attr } %}{{ block('attributes') }}{% endwith %}>
-            {%- if attr.placeholder is defined -%}
+            {%- if attr.placeholder is defined and attr.placeholder is not none -%}
                 {{- translation_domain is same as(false) ? attr.placeholder : attr.placeholder|trans({}, translation_domain) -}}
             {%- endif -%}
         </label>

diff --git a/src/Symfony/Component/Translation/DataCollectorTranslator.php b/src/Symfony/Component/Translation/DataCollectorTranslator.php
@@ -141,7 +141,7 @@ public function getCollectedMessages()
         return $this->messages;
     }
 
-    private function collectMessage(?string $locale, ?string $domain, string $id, string $translation, ?array $parameters = [])
+    private function collectMessage(?string $locale, ?string $domain, ?string $id, string $translation, ?array $parameters = [])
     {
         if (null === $domain) {
             $domain = 'messages';

diff --git a/src/Symfony/Component/Translation/LoggingTranslator.php b/src/Symfony/Component/Translation/LoggingTranslator.php
@@ -131,7 +131,7 @@ public function __call($method, $args)
     /**
      * Logs for missing translations.
      */
-    private function log(string $id, ?string $domain, ?string $locale)
+    private function log(?string $id, ?string $domain, ?string $locale)
     {
         if (null === $domain) {
             $domain = 'messages';

diff --git a/src/Symfony/Component/Translation/Tests/TranslatorTest.php b/src/Symfony/Component/Translation/Tests/TranslatorTest.php
@@ -491,6 +491,19 @@ public function testTransChoiceNullLocale()
         $this->addToAssertionCount(1);
     }
 
+    public function testTransNullId()
+    {
+        $translator = new Translator('en');
+        $translator->addLoader('array', new ArrayLoader());
+        $translator->addResource('array', ['foo' => 'foofoo'], 'en');
+
+        $this->assertSame('', $translator->trans(null));
+
+        (\Closure::bind(function () use ($translator) {
+            $this->assertSame([], $translator->catalogues);
+        }, $this, Translator::class))();
+    }
+
     public function getTransFileTests()
     {
         return [
@@ -512,6 +525,7 @@ public function getTransTests()
             ['Symfony est super !', 'Symfony is great!', 'Symfony est super !', [], 'fr', ''],
             ['Symfony est awesome !', 'Symfony is %what%!', 'Symfony est %what% !', ['%what%' => 'awesome'], 'fr', ''],
             ['Symfony est super !', new StringClass('Symfony is great!'), 'Symfony est super !', [], 'fr', ''],
+            ['', null, '', [], 'fr', ''],
         ];
     }
 

diff --git a/src/Symfony/Component/Translation/Translator.php b/src/Symfony/Component/Translation/Translator.php
@@ -210,11 +210,14 @@ public function getFallbackLocales()
      */
     public function trans($id, array $parameters = [], $domain = null, $locale = null)
     {
+        if ('' === $id = (string) $id) {
+            return '';
+        }
+
         if (null === $domain) {
             $domain = 'messages';
         }
 
-        $id = (string) $id;
         $catalogue = $this->getCatalogue($locale);
         $locale = $catalogue->getLocale();
         while (!$catalogue->defines($id, $domain)) {
@@ -242,6 +245,10 @@ public function transChoice($id, $number, array $parameters = [], $domain = null
     {
         @trigger_error(sprintf('The "%s()" method is deprecated since Symfony 4.2, use the trans() one instead with a "%%count%%" parameter.', __METHOD__), E_USER_DEPRECATED);
 
+        if ('' === $id = (string) $id) {
+            return '';
+        }
+
         if (!$this->formatter instanceof ChoiceMessageFormatterInterface) {
             throw new LogicException(sprintf('The formatter "%s" does not support plural translations.', \get_class($this->formatter)));
         }
@@ -250,7 +257,6 @@ public function transChoice($id, $number, array $parameters = [], $domain = null
             $domain = 'messages';
         }
 
-        $id = (string) $id;
         $catalogue = $this->getCatalogue($locale);
         $locale = $catalogue->getLocale();
         while (!$catalogue->defines($id, $domain)) {

diff --git a/src/Symfony/Contracts/Translation/TranslatorTrait.php b/src/Symfony/Contracts/Translation/TranslatorTrait.php
@@ -43,7 +43,9 @@ public function getLocale()
      */
     public function trans($id, array $parameters = [], $domain = null, $locale = null)
     {
-        $id = (string) $id;
+        if ('' === $id = (string) $id) {
+            return '';
+        }
 
         if (!isset($parameters['%count%']) || !is_numeric($parameters['%count%'])) {
             return strtr($id, $parameters);