ZF-11402: Zend_Form_Element_MultiCheckbox does not accept empty array even when AllowEmpty flag is set

Description

Here is the code with the validator :

<?php class Form_User extends Zend_Form { ... // Cities element $options = array( 1 => 'New York', 2 => 'Los Angeles' 3 => 'Chicago' ); $element = new Zend_Form_Element_MultiCheckbox('cities'); $element->setLabel('Cities') ->setMultiOptions($options) ->setRequired(true) ->addValidators(array( new Zend_Validate_NotEmpty(Zend_Validate_NotEmpty::PHP) )); $this->addElement($element); ... ?>

Then I test it like this:

<?php $form = new Form_User(); $user = array( 'name' => 'someone', 'email' => 'someone@somewhere.com', 'cities' => array() ); Zend_Debug::dump($form->isValid($user)); exit(); // return (bool) true ?>

I would have expected the $form->isValid($user) to return false ...

In my real code I use a form to validate my model before saving it. The user model has an empty array as default value if data relationship is enable in my mapper other null. My user model should have at least 1 city but I can't check it with the form validation when data relationship is enable...

Comments

This is not an issue with Zend_Validate_NotEmpty, as it rejects empty array properly, as evidenced by this test:


/**
 * @group ZF-11402
 */
public function testPHPRejectsEmptyArray()
{
    $validator = new Zend_Validate_NotEmpty(
        Zend_Validate_NotEmpty::PHP
    );
    $this->assertFalse($validator->isValid(array()));        
}

And the result:


++ phpunit --verbose --group ZF-11402 AllTests
PHPUnit 3.5.13 by Sebastian Bergmann.
OK (1 test, 1 assertion)

I would check the documentation for Zend_Form_Element_MultiCheckbox to determine how to properly enforce the condition that one or more of the checkboxes in the group must be selected.

Hi Adam,

Sorry about this, I miss placed the issue. It is not on the Zend_Validator_NotEmpty element but on the Zend_Element_Multi. Correct me if I'm wrong but the validator for Zend_Element_Multi the validation only happens on each individudal value but not set of values itself if it is an array. Believe me, I have red the documentation but I couldn't find any way (expect ugly hacks) to validate the number of elements of a Zend_Element_Multi. I even looked in the code of Zend_Element_Multi and Zend_Element and to my unberstanding, today it is impossible to validate the number of values recieved.

Here is the code of the validation for Zend_Element (nothing interesting in Zend_Element_Multi just adding InArray) :

public function isValid($value, $context = null) { ... foreach ($this->getValidators() as $key => $validator) { ... if ($isArray && is_array($value)) { $messages = array(); $errors = array(); foreach ($value as $val) { if (!$validator->isValid($val, $context)) { $result = false; if ($this->_hasErrorMessages()) { $messages = $this->_getErrorMessages(); $errors = $messages; } else { $messages = array_merge($messages, $validator->getMessages()); $errors = array_merge($errors, $validator->getErrors()); } } } if ($result) { continue; } } .. } ... }

Validation only validate single value not value set so if you give a empty array it will still pass the validation. Not alternative method given by the documentation ...

Olivier,

Ah, I see the issue now. I can reproduce it with this code:


/**
 * @group ZF-11402
 */
public function testValidateShouldNotAcceptEmptyArray()
{
    $this->element->addMultiOptions(array(
        'foo' => 'Foo',
        'bar' => 'Bar',
        'baz' => 'Baz',
    ));
    $this->element->setRegisterInArrayValidator(true);

    $this->assertTrue($this->element->isValid(array('foo')));
    $this->assertTrue($this->element->isValid(array('foo','baz')));

    $this->element->setAllowEmpty(true);
    $this->assertTrue($this->element->isValid(array()));

    $this->element->setAllowEmpty(false);
    $this->assertFalse($this->element->isValid(array()));
}

This results in:


++ phpunit --verbose --group Zend_Form AllTests

There was 1 failure:

1) Zend_Form_Element_MultiCheckboxTest::testValidateShouldNotAcceptEmptyArray
Failed asserting that  is false.

/usr/local/apache2/htdocs/lib/zfdev/trunk/tests/Zend/Form/Element/MultiCheckboxTest.php:301

The last assert in my test - where isValid(array()) is tested - fails. When I make the following change to Zend_Form_Element:


Index: library/Zend/Form/Element.php
===================================================================
--- library/Zend/Form/Element.php       (revision 24092)
+++ library/Zend/Form/Element.php       (working copy)
@@ -1377,15 +1377,23 @@
             if ($isArray && is_array($value)) {
                 $messages = array();
                 $errors   = array();
-                foreach ($value as $val) {
-                    if (!$validator->isValid($val, $context)) {
+                if (empty($value)) {
+                    if ($this->isRequired()
+                        || (!$this->isRequired() && !$this->getAllowEmpty())
+                    ) {
                         $result = false;
-                        if ($this->_hasErrorMessages()) {
-                            $messages = $this->_getErrorMessages();
-                            $errors   = $messages;
-                        } else {
-                            $messages = array_merge($messages, $validator->getMessages());
-                            $errors   = array_merge($errors,   $validator->getErrors());
+                    }
+                } else {
+                    foreach ($value as $val) {
+                        if (!$validator->isValid($val, $context)) {
+                            $result = false;
+                            if ($this->_hasErrorMessages()) {
+                                $messages = $this->_getErrorMessages();
+                                $errors   = $messages;
+                            } else {
+                                $messages = array_merge($messages, $validator->getMessages());
+                                $errors   = array_merge($errors,   $validator->getErrors());
+                            }
                         }
                     }
                 }

The heart of the change is this:


if (empty($value)) {
    if ($this->isRequired()
        || (!$this->isRequired() && !$this->getAllowEmpty())
    ) {
        $result = false;
    }
}

Which sets the result to false if the value is empty and the field is either required or is not required and does not allow empty values. The latter is done to maintain the fact that Required supercedes AllowEmpty, as by defition marking a field as required does not allow it to be empty.

After making the proposed change to Zend_Form_Element, all unit tests in Zend_Form group (including my test testValidateShouldNotAcceptEmptyArray()) now pass.

I've attached the full patch to this issue.

This, however, only handles the empty case. I haven't looked at your other question on how to validate the number of values provided (ie: X values must be provided)

Adam,

Thanks for the patch. For the x values required validation, I just feel that it should happen in Zend_Element_Multi and not in Zend_Element. I guess other validation could be useful at this higher level like SumGreaterThan, SumLessThan, Average,... It would be nice to have some room for specific validation on Zend_Element_Multi.

Assigned Zend_Form as component as requested Removed Zend_Validate as component Reassigned maintainer

Reviewed and applied patches from Adam to trunk and 1.11 release branch.

What about the validation messages? isValid will return false, but there will be no error message(s) associated with the element.

Since the NotEmpty validator is being added to validation stack when the element value is required, we can allow it to run on the known empty value and thus create the error messages.

Library File:


Index: library/Zend/Form/Element.php
===================================================================
--- library/Zend/Form/Element.php   (revision 24480)
+++ library/Zend/Form/Element.php   (working copy)
@@ -1377,13 +1379,13 @@
             if ($isArray && is_array($value)) {
                 $messages = array();
                 $errors   = array();
+
+                // If the array is empty, then throw an empty element in the
+                // values array to allow the validators to run their course.
                 if (empty($value)) {
-                    if ($this->isRequired()
-                        || (!$this->isRequired() && !$this->getAllowEmpty())
-                    ) {
-                        $result = false;
+                    array_push($value, '');
                     }
-                } else {
+
                     foreach ($value as $val) {
                         if (!$validator->isValid($val, $context)) {
                             $result = false;

Unit Test:


Index: tests/Zend/Form/Element/MultiCheckboxTest.php
===================================================================
--- tests/Zend/Form/Element/MultiCheckboxTest.php   (revision 24480)
+++ tests/Zend/Form/Element/MultiCheckboxTest.php   (working copy)
@@ -299,6 +299,15 @@
     
         $this->element->setAllowEmpty(false);
         $this->assertFalse($this->element->isValid(array()));
+
+        $errors = $this->element->getErrors();
+        $this->assertArrayHasKey(0, $errors);
+        $this->assertEquals(1, count($errors));
+
+        $messages = $this->element->getMessages();
+        $this->assertArrayHasKey($errors[0], $messages);
+        $this->assertEquals(1, count($messages));
+        $this->assert($this->element->get
     }
 }

Justis: Thanks for pointing that out! I've taken your suggestion and implemented a revised version of my original fix which allows the validators to run so that the error messages come through properly. Could you try it out and let me know if it fixes the problem you raised?

Adam,

Yes, your most recent patch fixes the issue. Thank you.

Additional fix committed to trunk in r24552 Merged to release-1.11 in r24553