From 39c148197f3c4f8efb4e6534259619f473292b69 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Thu, 3 Feb 2011 10:53:51 +0100 Subject: [PATCH] [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 | 73 ++++++++--- .../Form/Resources/config/validation.xml | 1 + .../Symfony/Tests/Component/Form/FormTest.php | 121 +++++++++++++++--- 3 files changed, 159 insertions(+), 36 deletions(-) diff --git a/src/Symfony/Component/Form/Form.php b/src/Symfony/Component/Form/Form.php index f6399af5e3..14dc1f2d75 100644 --- a/src/Symfony/Component/Form/Form.php +++ b/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. * diff --git a/src/Symfony/Component/Form/Resources/config/validation.xml b/src/Symfony/Component/Form/Resources/config/validation.xml index 1520e5ee26..89aafdf55a 100644 --- a/src/Symfony/Component/Form/Resources/config/validation.xml +++ b/src/Symfony/Component/Form/Resources/config/validation.xml @@ -13,6 +13,7 @@ + validateData diff --git a/tests/Symfony/Tests/Component/Form/FormTest.php b/tests/Symfony/Tests/Component/Form/FormTest.php index a2a35181a3..7becaffe6d 100644 --- a/tests/Symfony/Tests/Component/Form/FormTest.php +++ b/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');