[Form] Fixed form validation

Separated validation of data and form had serious drawbacks. When a form had nested form whose data was not connected to the data of the root form, this data would not be validated.

The new implementation validates the whole object graph at once. Class Form has a new method validateData(), that manually passes the data to the GraphWalker of the Validator and overrides the Default group with the groups set in the form.

src/Symfony/Component/Form/Form.php

@@ -14,6 +14,7 @@ namespace Symfony\Component\Form;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\FileBag;
 use Symfony\Component\Validator\ValidatorInterface;
+use Symfony\Component\Validator\ExecutionContext;
 use Symfony\Component\Form\Exception\FormException;
 use Symfony\Component\Form\Exception\MissingOptionsException;
 use Symfony\Component\Form\Exception\AlreadySubmittedException;
@@ -667,7 +668,13 @@ class Form extends Field implements \IteratorAggregate, FormInterface
      */
     public function getValidationGroups()
     {
-        return $this->getOption('validation_groups');
+        $groups = $this->getOption('validation_groups');
+
+        if (!$groups && $this->hasParent()) {
+            $groups = $this->getParent()->getValidationGroups();
+        }
+
+        return $groups;
     }
 
     /**
@@ -736,28 +743,22 @@ class Form extends Field implements \IteratorAggregate, FormInterface
             throw new MissingOptionsException('The option "validator" is required for validating', array('validator'));
         }
 
-        // Only validate data if it is a domain object
-        if (is_object($this->getData())) {
-            // Validate the submitted data in the domain object in the sets
-            // validation group(s)
-            if ($violations = $validator->validate($this->getData(), $groups)) {
-                foreach ($violations as $violation) {
-                    $propertyPath = new PropertyPath($violation->getPropertyPath());
-                    $iterator = $propertyPath->getIterator();
-                    $iterator->next(); // point at the first data element
-                    $error = new DataError($violation->getMessageTemplate(), $violation->getMessageParameters());
-
-                    $this->addError($error, $iterator);
-                }
-            }
-        }
-
-        // Validate the submitted data in the fields in group "Default"
+        // Validate the form in group "Default"
+        // Validation of the data in the custom group is done by validateData(),
+        // which is constrained by the Execute constraint
         if ($violations = $validator->validate($this)) {
             foreach ($violations as $violation) {
                 $propertyPath = new PropertyPath($violation->getPropertyPath());
                 $iterator = $propertyPath->getIterator();
-                $error = new FieldError($violation->getMessageTemplate(), $violation->getMessageParameters());
+                $template = $violation->getMessageTemplate();
+                $parameters = $violation->getMessageParameters();
+
+                if ($iterator->current() == 'data') {
+                    $iterator->next(); // point at the first data element
+                    $error = new DataError($template, $parameters);
+                } else {
+                    $error = new FieldError($template, $parameters);
+                }
 
                 $this->addError($error, $iterator);
             }
@@ -845,6 +846,40 @@ class Form extends Field implements \IteratorAggregate, FormInterface
         return $this->getOption('context');
     }
 
+    /**
+     * Validates the data of this form
+     *
+     * This method is called automatically during the validation process.
+     *
+     * @param ExecutionContext $context  The current validation context
+     */
+    public function validateData(ExecutionContext $context)
+    {
+        $groups = $this->getValidationGroups();
+        $propertyPath = $context->getPropertyPath();
+        $graphWalker = $context->getGraphWalker();
+
+        if (null === $groups) {
+            $groups = array(null);
+        }
+
+        // The Execute constraint is called on class level, so we need to
+        // set the property manually
+        $context->setCurrentProperty('data');
+
+        // Adjust the property path accordingly
+        if (!empty($propertyPath)) {
+            $propertyPath .= '.';
+        }
+
+        $propertyPath .= 'data';
+
+        foreach ($groups as $group) {
+            // Don't use potential overridden versions of getData()!
+            $graphWalker->walkReference(Form::getData(), $group, $propertyPath, true);
+        }
+    }
+
     /**
      * Merges two arrays without reindexing numeric keys.
      *

src/Symfony/Component/Form/Resources/config/validation.xml

@@ -13,6 +13,7 @@
   </class>
 
   <class name="Symfony\Component\Form\Form">
+    <constraint name="Execute">validateData</constraint>
     <property name="fields">
       <constraint name="Valid" />
     </property>

tests/Symfony/Tests/Component/Form/FormTest.php

@@ -26,6 +26,7 @@ use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\File\UploadedFile;
 use Symfony\Component\Validator\ConstraintViolation;
 use Symfony\Component\Validator\ConstraintViolationList;
+use Symfony\Component\Validator\ExecutionContext;
 use Symfony\Tests\Component\Form\Fixtures\Author;
 use Symfony\Tests\Component\Form\Fixtures\TestField;
 use Symfony\Tests\Component\Form\Fixtures\TestForm;
@@ -198,7 +199,31 @@ class FormTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals(array('group1', 'group2'), $form->getValidationGroups());
     }
 
-    public function testBindUsesValidationGroups()
+    public function testValidationGroupsAreInheritedFromParentIfEmpty()
+    {
+        $parentForm = new Form('parent', array(
+            'validation_groups' => 'group',
+        ));
+        $childForm = new Form('child');
+        $parentForm->add($childForm);
+
+        $this->assertEquals(array('group'), $childForm->getValidationGroups());
+    }
+
+    public function testValidationGroupsAreNotInheritedFromParentIfSet()
+    {
+        $parentForm = new Form('parent', array(
+            'validation_groups' => 'group1',
+        ));
+        $childForm = new Form('child', array(
+            'validation_groups' => 'group2',
+        ));
+        $parentForm->add($childForm);
+
+        $this->assertEquals(array('group2'), $childForm->getValidationGroups());
+    }
+
+    public function testBindValidatesData()
     {
         $form = new Form('author', array(
             'validation_groups' => 'group',
@@ -206,24 +231,12 @@ class FormTest extends \PHPUnit_Framework_TestCase
         ));
         $form->add(new TestField('firstName'));
 
-        // both the form and the object are validated
-        $this->validator->expects($this->exactly(2))
-            ->method('validate');
-
-// PHPUnit limitation here
-//        // form data is validated in custom group
-//        $this->validator->expects($this->once())
-//            ->method('validate')
-//            ->with($this->equalTo($form->getData()), $this->equalTo(array('group')));
-//
-//        // form itself is validated in group "Default"
-//        $this->validator->expects($this->once())
-//            ->method('validate')
-//            ->with($this->equalTo($form));
+        $this->validator->expects($this->once())
+            ->method('validate')
+            ->with($this->equalTo($form));
 
         // concrete request is irrelevant
-        // data is an object
-        $form->bind($this->createPostRequest(), new Author());
+        $form->bind($this->createPostRequest());
     }
 
     public function testBindDoesNotValidateArrays()
@@ -1036,6 +1049,68 @@ class FormTest extends \PHPUnit_Framework_TestCase
         $this->assertSame(array($form['visibleField']), $visibleFields);
     }
 
+    public function testValidateData()
+    {
+        $graphWalker = $this->createMockGraphWalker();
+        $metadataFactory = $this->createMockMetadataFactory();
+        $context = new ExecutionContext('Root', $graphWalker, $metadataFactory);
+        $object = $this->getMock('\stdClass');
+        $form = new Form('author', array('validation_groups' => array(
+            'group1',
+            'group2',
+        )));
+
+        $graphWalker->expects($this->exactly(2))
+                ->method('walkReference')
+                ->with($object,
+                    // should test for groups - PHPUnit limitation
+                    $this->anything(),
+                    'data',
+                    true);
+
+        $form->setData($object);
+        $form->validateData($context);
+    }
+
+    public function testValidateDataAppendsPropertyPath()
+    {
+        $graphWalker = $this->createMockGraphWalker();
+        $metadataFactory = $this->createMockMetadataFactory();
+        $context = new ExecutionContext('Root', $graphWalker, $metadataFactory);
+        $context->setPropertyPath('path');
+        $object = $this->getMock('\stdClass');
+        $form = new Form('author');
+
+        $graphWalker->expects($this->once())
+                ->method('walkReference')
+                ->with($object,
+                    null,
+                    'path.data',
+                    true);
+
+        $form->setData($object);
+        $form->validateData($context);
+    }
+
+    public function testValidateDataSetsCurrentPropertyToData()
+    {
+        $graphWalker = $this->createMockGraphWalker();
+        $metadataFactory = $this->createMockMetadataFactory();
+        $context = new ExecutionContext('Root', $graphWalker, $metadataFactory);
+        $object = $this->getMock('\stdClass');
+        $form = new Form('author');
+        $test = $this;
+
+        $graphWalker->expects($this->once())
+                ->method('walkReference')
+                ->will($this->returnCallback(function () use ($context, $test) {
+                    $test->assertEquals('data', $context->getCurrentProperty());
+                }));
+
+        $form->setData($object);
+        $form->validateData($context);
+    }
+
     /**
      * Create a group containing two fields, "visibleField" and "hiddenField"
      *
@@ -1153,6 +1228,18 @@ class FormTest extends \PHPUnit_Framework_TestCase
         return $this->getMock('Symfony\Component\Form\CsrfProvider\CsrfProviderInterface');
     }
 
+    protected function createMockGraphWalker()
+    {
+        return $this->getMockBuilder('Symfony\Component\Validator\GraphWalker')
+                ->disableOriginalConstructor()
+                ->getMock();
+    }
+
+    protected function createMockMetadataFactory()
+    {
+        return $this->getMock('Symfony\Component\Validator\Mapping\ClassMetadataFactoryInterface');
+    }
+
     protected function createPostRequest(array $values = array(), array $files = array())
     {
         $server = array('REQUEST_METHOD' => 'POST');