Skip to content
This repository

Fix PHP Notice in Translator class #4013

Closed
wants to merge 4 commits into from

3 participants

Alex Brausewetter Matthew Weier O'Phinney Ben Scholzen
Alex Brausewetter
xoob commented March 12, 2013

Translator::getTranslatedMessage() causes a PHP Notice due to a change made in 7422c37#L0R398

Notice: Undefined index: en_US in .../Zend/I18n/Translator/Translator.php on line 402

This change adds an isset() to fix this, so that the index is checked as it used to be.

Matthew Weier O'Phinney
Owner

Please add a unit test

Alex Brausewetter
xoob commented March 12, 2013

@weierophinney Done.

$ php run-tests.php I18n
1) ZendTest\I18n\Translator\TranslatorTest::testTranslateNonExistantLocale
Undefined index: es_ES
FAILURES!
Tests: 258, Assertions: 2187, Errors: 1.

$ git checkout bugfix/translator

$ php run-tests.php I18n
OK (258 tests, 2189 assertions)
Ben Scholzen
Collaborator

This is not really the best solution, as it will try to load messages for that locale over and over again. It makes much more sense to change line 519 (in your codebase) to the following:

$this->messages[$textDomain] = array($locale => null);

And then in getTranslatedMessage() change your new if statement to an !== null comparision.

Alex Brausewetter
xoob commented March 12, 2013

OK, here is an adapted fix.

Instead of initializing with array($locale => null), I opted to check for if (!$messagesLoaded) { at the end of loadMessages() to handle situations where $locale was changed between calls. I've updated the unit test to check for this as well.

Looking forward to your feedback.

library/Zend/I18n/Translator/Translator.php
@@ -531,8 +531,12 @@ protected function loadMessages($textDomain, $locale)
531 531
             || $this->loadMessagesFromFiles($textDomain, $locale)
532 532
         );
533 533
 
534  
-        if ($messagesLoaded && $cache !== null) {
535  
-            $cache->setItem($cacheId, $this->messages[$textDomain][$locale]);
  534
+        if (!$messagesLoaded) {
  535
+            $this->messages[$textDomain][$locale] = null;
  536
+        } else {
  537
+            if ($cache !== null) {
1
Ben Scholzen Collaborator
DASPRiD added a note March 12, 2013

Merge those two lines into } elseif ($ache…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Ben Scholzen
Collaborator

Looks good like this, except the one thing I noted.

Ben Scholzen DASPRiD referenced this pull request March 13, 2013
Closed

Update Translator.php #4025

Alex Brausewetter
xoob commented March 14, 2013

@DASPRiD Changed to elseif as requested, in dae382d

Ben Scholzen DASPRiD closed this pull request from a commit March 16, 2013
Ben Scholzen Merge branch 'hotfix/4013'
Close #4013
e8540e4
Ben Scholzen DASPRiD closed this in e8540e4 March 16, 2013
Ben Scholzen DASPRiD referenced this pull request from a commit March 16, 2013
Ben Scholzen Merge branch 'hotfix/4013' into develop
Forward port #4013
8103b37
Deleted user Unknown referenced this pull request from a commit March 16, 2013
Ben Scholzen Merge branch 'hotfix/4013'
Close #4013
2a5d02b
Deleted user Unknown referenced this pull request from a commit March 16, 2013
Ben Scholzen Merge branch 'hotfix/4013' into develop
Forward port #4013
315dfb4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
4  library/Zend/I18n/Translator/Translator.php
@@ -531,7 +531,9 @@ protected function loadMessages($textDomain, $locale)
531 531
             || $this->loadMessagesFromFiles($textDomain, $locale)
532 532
         );
533 533
 
534  
-        if ($messagesLoaded && $cache !== null) {
  534
+        if (!$messagesLoaded) {
  535
+            $this->messages[$textDomain][$locale] = null;
  536
+        } elseif ($cache !== null) {
535 537
             $cache->setItem($cacheId, $this->messages[$textDomain][$locale]);
536 538
         }
537 539
     }
21  tests/ZendTest/I18n/Translator/TranslatorTest.php
@@ -191,4 +191,25 @@ public function testTranslatePlurals()
191 191
         $this->assertEquals('Message 5 (en) Plural 1', $pl1);
192 192
         $this->assertEquals('Message 5 (en) Plural 2', $pl2);
193 193
     }
  194
+
  195
+    public function testTranslateNonExistantLocale()
  196
+    {
  197
+        $this->translator->addTranslationFilePattern(
  198
+            'phparray',
  199
+            $this->testFilesDir . '/testarray',
  200
+            'translation-%s.php'
  201
+        );
  202
+
  203
+        // Test that a locale without translations does not cause warnings
  204
+
  205
+        $this->translator->setLocale('es_ES');
  206
+
  207
+        $this->assertEquals('Message 1', $this->translator->translate('Message 1'));
  208
+        $this->assertEquals('Message 9', $this->translator->translate('Message 9'));
  209
+
  210
+        $this->translator->setLocale('fr_FR');
  211
+
  212
+        $this->assertEquals('Message 1', $this->translator->translate('Message 1'));
  213
+        $this->assertEquals('Message 9', $this->translator->translate('Message 9'));
  214
+    }
194 215
 }
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.