Issues

ZF-9364: Zend_Form isValid|isValidPartial overwrites Translator of SubForms and Elements

Description

Zend_Form isValid and isValidPartial calls $element->setTranslator() not respecting if an individual Translator was set for $element.

Comments

That would be better fixed in the isValid() method of Zend_Form_Element which is overriding the translator for validators. Hence, it would allow us to use the resources/translators arrays and our custom translator for the elements.

line 1330 in Zend_Form_Element:

[code] foreach ($this->getValidators() as $key => $validator) { if (method_exists($validator, 'setTranslator')) { $validator->setTranslator($this->getTranslator()); } [/code]

Dominique, you are right that the Validation translator is overwritten in Zend_Form_Element::isValid(), but that really is a different issue.

But i realized now the example above is not at the state of the patch, i will update that.

The description isn't too helpful :)

The actual problem reported here is that if you attach a translator object to the form and to one of the elements, then the translator object attached to the element is overridden by the one attached to the form when isValid() or isValidPartial() is called. It is reasonable that the law of specificity applies here and that if someone has taken the trouble to apply a translator to the element, then it should be used in preference to the form level one.

Correct patch:


Index: library/Zend/Form.php
===================================================================
--- library/Zend/Form.php   (revision 21616)
+++ library/Zend/Form.php   (working copy)
@@ -2031,7 +2031,7 @@
         }
 
         foreach ($this->getElements() as $key => $element) {
-            if (null !== $translator) {
+            if (null !== $translator && !$element->getTranslator()) {
                 $element->setTranslator($translator);
             }
             if (!isset($data[$key])) {
@@ -2079,7 +2079,7 @@
 
         foreach ($data as $key => $value) {
             if (null !== ($element = $this->getElement($key))) {
-                if (null !== $translator) {
+                if (null !== $translator && !$element->getTranslator()) {
                     $element->setTranslator($translator);
                 }
                 $valid = $element->isValid($value, $data) && $valid;

Fixed in trunk r21617 and release-1.10 branch r21618.

Thank you Rob, as i am new to ZFdev and new to collaborative OS Development in general, i am not used to the terms. You have correctly described the problem i thought to address here.

I only recognize Zend_Form growed over time, getting more features and loosing concept.

Christian, I was referring to ZF 9275 which is mentioned to be fixed by this patch.

Dominique, i reopend [ZF-9275]

The commited fix is incorrect as $element->getTranslator() will return the default Translator from Zend_Form and if Zend_Form has no default Translator set the one from Zend_Registry if this is set. Only when there is no translator set in the chain $element->getTranslator() returns null.

So in a case when there is an indivdual Translator set for Zend_Form which should overwrite the default Translator from Zend_Form or even Zend_Registry this will not happen.

Cleaner solution, add Method hasTranslator() to Zend_Form and Zend_Form_Element


Index: library/Zend/Form.php
===================================================================
--- library/Zend/Form.php       (Revision 21641)
+++ library/Zend/Form.php       (Arbeitskopie)
@@ -2031,7 +2031,7 @@
         }
 
         foreach ($this->getElements() as $key => $element) {
-            if (null !== $translator && !$element->getTranslator()) {
+            if (null !== $translator && !$element->hasTranslator()) {
                 $element->setTranslator($translator);
             }
             if (!isset($data[$key])) {
@@ -2041,7 +2041,9 @@
             }
         }
         foreach ($this->getSubForms() as $key => $form) {
-            $form->setTranslator($translator);
+            if (null !== $translator && !$form->hasTranslator()) {
+                $form->setTranslator($translator);
+            }
             if (isset($data[$key])) {
                 $valid = $form->isValid($data[$key]) && $valid;
             } else {
@@ -2079,12 +2081,12 @@
 
         foreach ($data as $key => $value) {
             if (null !== ($element = $this->getElement($key))) {
-                if (null !== $translator && !$element->getTranslator()) {
+                if (null !== $translator && !$element->hasTranslator()) {
                     $element->setTranslator($translator);
                 }
                 $valid = $element->isValid($value, $data) && $valid;
             } elseif (null !== ($subForm = $this->getSubForm($key))) {
-                if (null !== $translator) {
+                if (null !== $translator && !$subForm->hasTranslator()) {
                     $subForm->setTranslator($translator);
                 }
                 $valid = $subForm->isValidPartial($data[$key]) && $valid;
@@ -2093,7 +2095,7 @@
         }
         foreach ($this->getSubForms() as $key => $subForm) {
             if (!in_array($key, $validatedSubForms)) {
-                if (null !== $translator) {
+                if (null !== $translator && !$subForm->hasTranslator()) {
                     $subForm->setTranslator($translator);
                 }
 
@@ -2743,6 +2745,20 @@
     }
 
     /**
+     * Was a (default) Translator set?
+     *
+     * @return Boolean
+     */
+    public function hasTranslator()
+    {
+        if (null === $this->_translator &&
+            null === self::$_translatorDefault) {
+            return false;
+        }
+        return true;
+    }
+
+    /**
      * Get global default translator object
      *
      * @return null|Zend_Translate
Index: library/Zend/Form/Element.php
===================================================================
--- library/Zend/Form/Element.php       (Revision 21641)
+++ library/Zend/Form/Element.php       (Arbeitskopie)
@@ -415,6 +415,19 @@
     }
 
     /**
+     * Was a Translator set?
+     *
+     * @return Boolean
+     */
+    public function hasTranslator()
+    {
+        if (null === $this->_translator) {
+            return false;
+        }
+        return true;
+    }
+
+    /**
      * Indicate whether or not translation should be disabled
      *
      * @param  bool $flag

Please review, there is an error in the commited fix.

proove that it works


cd library
php -r 'require_once "Zend/Translate.php"; \
require_once "Zend/Form.php"; \
$translate = new Zend_Translate("Array"); \
$form = new Zend_Form(); \
var_dump($form->hasTranslator()); \
Zend_Form::setDefaultTranslator($translate); \
var_dump($form->hasTranslator());'
// bool(false)
// bool(true)

The commit works as designed to fix the described issue :)

There is however a new issue introduced as a result as noted by Christian: If the element doesn't have a local translator and a translator is set into the registry using the key "Zend_Translate", then the form's translator is not used and the one in the registry is used instead.

As noted, this is because Zend_Form_Element::getTranslator() will return the 'Zend_Translate' translator if there isn't a local one set.

This patch with unit test fixes it:


Index: tests/Zend/Form/FormTest.php
===================================================================
--- tests/Zend/Form/FormTest.php    (revision 21644)
+++ tests/Zend/Form/FormTest.php    (working copy)
@@ -3847,6 +3847,42 @@
         $this->assertEquals(2, count($messages));
         $this->assertEquals('Element message', $messages['foo']['isEmpty']);
         $this->assertEquals('Form message', $messages['bar']['isEmpty']);
+    }
+    
+    /**
+     * @group ZF-9364
+     */
+    public function testElementTranslatorPreferredOverDefaultTranslator()
+    {
+        $defaultTranslations = array(
+            'isEmpty' => 'Default message',
+        );
+        $formTranslations = array(
+            'isEmpty' => 'Form message',
+        );        
+        $elementTranslations = array(
+            'isEmpty' => 'Element message',
+        );
+        $defaultTranslate = new Zend_Translate('array', $defaultTranslations);
+        $formTranslate = new Zend_Translate('array', $formTranslations);
+        $elementTranslate = new Zend_Translate('array', $elementTranslations);
+        
+        Zend_Registry::set('Zend_Translate', $defaultTranslate);
+        $this->form->setTranslator($formTranslate);        
+        $this->form->addElement('text', 'foo', array('required'=>true, 'translator'=>$elementTranslate));
+        $this->form->addElement('text', 'bar', array('required'=>true));
+
+        $this->assertFalse($this->form->isValid(array('foo'=>'', 'bar'=>'')));
+        $messages = $this->form->getMessages();
+        $this->assertEquals(2, count($messages));
+        $this->assertEquals('Element message', $messages['foo']['isEmpty']);
+        $this->assertEquals('Form message', $messages['bar']['isEmpty']);
+        
+        $this->assertFalse($this->form->isValidPartial(array('foo'=>'', 'bar'=>'')));
+        $messages = $this->form->getMessages();
+        $this->assertEquals(2, count($messages));
+        $this->assertEquals('Element message', $messages['foo']['isEmpty']);
+        $this->assertEquals('Form message', $messages['bar']['isEmpty']);
     }    
 
     /**
Index: library/Zend/Form.php
===================================================================
--- library/Zend/Form.php   (revision 21644)
+++ library/Zend/Form.php   (working copy)
@@ -2031,7 +2031,7 @@
         }
 
         foreach ($this->getElements() as $key => $element) {
-            if (null !== $translator && !$element->getTranslator()) {
+            if (null !== $translator && !$element->hasTranslator()) {
                 $element->setTranslator($translator);
             }
             if (!isset($data[$key])) {
@@ -2079,7 +2079,7 @@
 
         foreach ($data as $key => $value) {
             if (null !== ($element = $this->getElement($key))) {
-                if (null !== $translator && !$element->getTranslator()) {
+                if (null !== $translator && !$element->hasTranslator()) {
                     $element->setTranslator($translator);
                 }
                 $valid = $element->isValid($value, $data) && $valid;
Index: library/Zend/Form/Element.php
===================================================================
--- library/Zend/Form/Element.php   (revision 21644)
+++ library/Zend/Form/Element.php   (working copy)
@@ -413,6 +413,16 @@
         }
         return $this->_translator;
     }
+    
+    /**
+     * Does this element have its own specific translator?
+     * 
+     * @return bool
+     */
+    public function hasTranslator()
+    {
+        return (bool)$this->_translator;
+    }
 
     /**
      * Indicate whether or not translation should be disabled

Fixed again. Trunk: r21648, release-1.0 branch: r21649