Zend Framework

Zend_Form isValid|isValidPartial overwrites Translator of SubForms and Elements

Details

  • Type: Sub-task Sub-task
  • Status: Resolved Resolved
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.10.2
  • Fix Version/s: 1.10.3
  • Component/s: Zend_Form
  • Labels:
    None

Description

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

Issue Links

This issue is related to:
ZF-9275 Zend_Form_Element overrides Zend_Validate DefaultTranslator Minor Resolved
ZF-9330 Setting translator for elements and validators Major Closed
This issue is related to:
ZF-11831 Form's default translator incorrectly passed to subforms, causing override of subform elements' default translators Major Resolved

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Dominique Lorre added a comment -

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]

Show
Dominique Lorre added a comment - 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]
Hide
Permalink
Christian Albrecht added a comment -

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.

Show
Christian Albrecht added a comment - 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.
Hide
Permalink
Rob Allen added a comment -

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;
Show
Rob Allen added a comment - 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;
Hide
Permalink
Rob Allen added a comment -

Fixed in trunk r21617 and release-1.10 branch r21618.

Show
Rob Allen added a comment - Fixed in trunk r21617 and release-1.10 branch r21618.
Hide
Permalink
Christian Albrecht added a comment -

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.

Show
Christian Albrecht added a comment - 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.
Hide
Permalink
Dominique Lorre added a comment -

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

Show
Dominique Lorre added a comment - Christian, I was referring to ZF 9275 which is mentioned to be fixed by this patch.
Hide
Permalink
Christian Albrecht added a comment -

Dominique, i reopend ZF-9275

Show
Christian Albrecht added a comment - Dominique, i reopend ZF-9275
Hide
Permalink
Christian Albrecht added a comment - - edited

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
Show
Christian Albrecht added a comment - - edited 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
Hide
Permalink
Christian Albrecht added a comment -

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

Show
Christian Albrecht added a comment - Please review, there is an error in the commited fix.
Hide
Permalink
Christian Albrecht added a comment - - edited

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)
Show
Christian Albrecht added a comment - - edited 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)
Hide
Permalink
Rob Allen added a comment -

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
Show
Rob Allen added a comment - 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
Hide
Permalink
Rob Allen added a comment -

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

Show
Rob Allen added a comment - Fixed again. Trunk: r21648, release-1.0 branch: r21649

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Zend Framework. Try JIRA - bug tracking software for your team.