ZF-9710: When having subforms FormErrors produce bad html markup.

Description

This only happens when having subforms in a form with the Zend_Form_Decorator_FormErrors. To produce the error:


class Form_User extends Zend_Form
{
    public function init()
    {
        $this->addElement('text', 'name', array(
            'label' => 'Name',
            'required' => true,
            'validators' => array(
                new Zend_Validate_Alnum(true)
            )
        ));

        $this->addElement('text', 'email', array(
            'label' => 'Email',
            'required' => true,
            'validators' => array(
                new Zend_Validate_EmailAddress()
            )
        ));

        $subform = new Zend_Form_SubForm();

        $subform->addElement('text', 'address', array(
            'label' => 'Address',
            'required' => true,
            'validators' => array(
                new Zend_Validate_Alnum(true)
            )
        ));

        $subform->addElement('text', 'phone', array(
            'label' => 'Phone number',
            'required' => true
        ));

        $this->addSubForm($subform, 'location');

        $this->addElement('submit', 'submit');

        $this->addDecorators(array(
            new Zend_Form_Decorator_FormErrors(),
            new Zend_Form_Decorator_FormElements(),
            new Zend_Form_Decorator_HtmlTag(array(
                'tag' => 'dl', 'class' => 'zend_form'
            )),
            new Zend_Form_Decorator_Form(),
        ));
    }
}

When submiting with missing or wrong values it will produce:


Element Label Errors Element Label Errors Subform element Label Errors

The problem seems to be in the protected method _recurseSubform.

Comments

Fixed in trunk r22270 and merged into 1.10 release branch

It seems to be not fixed. Subform without elements produces:

There seems to be no check on if the subform is actually rendering any markup.

Mimicking the behavior from its own render() method, the following patch fixes the issue [~djmabo] is seeing


Index: FormErrors.php
===================================================================
--- FormErrors.php  (revision 22929)
+++ FormErrors.php  (working copy)
@@ -451,9 +451,13 @@
                              .  $this->getMarkupListItemEnd();
                 }
             } else if ($subitem instanceof Zend_Form && !$this->ignoreSubForms()) {
-                $content .= $this->getMarkupListStart()
-                          . $this->_recurseForm($subitem, $view)
-                          . $this->getMarkupListEnd();
+                $markup = $this->_recurseForm($subitem, $view);
+
+                if (!empty($markup)) {
+                    $content .= $this->getMarkupListStart()
+                              . $markup
+                              . $this->getMarkupListEnd();
+                }
             }
         }
         return $content;

Any comments on the proposed patch?

I confirm this bug is still around in 1.11.1.

The proposed patch works fine for me.

I can confirm this bug is still around in 1.11.2.

This patch will adds a test case for this issue to the FormErrorsTest test class:

Index: Zend/Form/Decorator/FormErrorsTest.php
===================================================================
--- Zend/Form/Decorator/FormErrorsTest.php  (revision 23642)
+++ Zend/Form/Decorator/FormErrorsTest.php  (working copy)
@@ -138,6 +138,18 @@
         $this->assertSame($content, $this->decorator->render($content));
     }
 
+    public function testNotGeneratingSubformErrorMarkupWrappingWhenNoErrors()
+    {
+        $form1 = new Zend_Form_SubForm();
+        $form2 = new Zend_Form();
+        $form2->addSubForm($form1, 'sub');
+        $form2->setView($this->getView());
+        $this->decorator->setElement($form2);
+
+        $content = 'test content';
+        $this->assertSame($content, $this->decorator->render($content));
+    }
+
     public function testRenderRendersAllErrorMessages()
     {
         $this->setupForm();
@@ -289,6 +301,7 @@
         $this->assertNotContains('form-badness', $html);
     }
 
+
     /**
      * @dataProvider markupOptionMethodsProvider
      */

Actually, that test case is flawed -- there's no way it can produce the value "test content" from rendering. The output actually rendered is "

" -- which makes sense to me. Can you clarify the test case?

I disagree. $this->decorator->render($content) does indeed render 'test content' (the FormErrors decorator prepends or appends error messages to the standard output of a form). A number of the other tests use a similar method calling render() (indeed, thats where I copied it from). The complete output is actually:


test content

Where as the correctly patched code outputs:


test content

So, in summary, in the case where a form has no errors, the Zend_Form_Decorator_FormErrors::render($content) merely returns $content.


        $markup = $this->_recurseForm($form, $view);

        if (empty($markup)) {
            return $content;
        }

This is true for the case where a form has NO subforms.

In the case where a form has a subform, during the _recurseForm method, the decorator ALWAYS wraps the output from the recursive response to _recurseForm even when the response is empty.


            } else if ($subitem instanceof Zend_Form && !$this->ignoreSubForms()) {
                $content .= $this->getMarkupListStart()
                          . $this->_recurseForm($subitem, $view)
                          . $this->getMarkupListEnd();
            }

So when FormErrors decorates a form with no errors and no subforms, the return from render is merely the form itself. When FormErrors decorates a form with no errors and at least one subform also with NO errors, the return from render is "

" (the inner

<

ul> from the lines 453-457 above, and the outer

<

ul> caused by code continuing past line 95 highlighted above).

This behavior is invalid, and I believe the test case I've provided correctly identifies the problem.

The patch I've proposed above checks makes _recurseForm check for a non-empty response from $this->_recurseForm($subitem, $view) before wrapping it with the error list start/end markup, just as render() does with the over-all response from _recurseForm().

Unit test case added and patch applied.