From c9c49006633a725f2e967fbb975950f9820c0d74 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Mon, 21 May 2012 11:18:32 +0200 Subject: [PATCH] [Form] Fixed: errors are not mapped to unsynchronized forms anymore --- src/Symfony/Component/Form/CHANGELOG.md | 1 + .../Form/Extension/Core/Type/FormType.php | 2 +- .../DelegatingValidationListener.php | 1 - .../ViolationMapper/ViolationMapper.php | 19 ++++--- src/Symfony/Component/Form/FormConfig.php | 27 ++++++++++ .../Component/Form/FormConfigInterface.php | 11 ++++ .../ViolationMapper/ViolationMapperTest.php | 52 ++++++++++++------- .../Component/Form/UnmodifiableFormConfig.php | 14 +++++ .../Form/Util/VirtualFormAwareIterator.php | 3 +- 9 files changed, 102 insertions(+), 28 deletions(-) diff --git a/src/Symfony/Component/Form/CHANGELOG.md b/src/Symfony/Component/Form/CHANGELOG.md index 2b1a6d32d4..1804009d22 100644 --- a/src/Symfony/Component/Form/CHANGELOG.md +++ b/src/Symfony/Component/Form/CHANGELOG.md @@ -73,6 +73,7 @@ CHANGELOG * added option "mapped" which should be used instead of setting "property_path" to false * "data_class" now *must* be set if a form maps to an object and should be left empty otherwise * improved error mapping on forms + * errors are not mapped to unsynchronized forms anymore * changed Form constructor to accept a single `FormConfigInterface` object * changed argument order in the FormBuilder constructor * deprecated Form methods diff --git a/src/Symfony/Component/Form/Extension/Core/Type/FormType.php b/src/Symfony/Component/Form/Extension/Core/Type/FormType.php index 5bd3b5e508..03269184fc 100644 --- a/src/Symfony/Component/Form/Extension/Core/Type/FormType.php +++ b/src/Symfony/Component/Form/Extension/Core/Type/FormType.php @@ -47,6 +47,7 @@ class FormType extends AbstractType // BC compatibility, when "property_path" could be false ->setPropertyPath(is_string($options['property_path']) ? $options['property_path'] : null) ->setMapped($options['mapped']) + ->setVirtual($options['virtual']) ->setAttribute('read_only', $options['read_only']) ->setAttribute('by_reference', $options['by_reference']) ->setAttribute('error_mapping', $options['error_mapping']) @@ -58,7 +59,6 @@ class FormType extends AbstractType ->setAttribute('invalid_message', $options['invalid_message']) ->setAttribute('invalid_message_parameters', $options['invalid_message_parameters']) ->setAttribute('translation_domain', $options['translation_domain']) - ->setAttribute('virtual', $options['virtual']) ->setAttribute('single_control', $options['single_control']) ->setData($options['data']) ->setDataMapper(new PropertyPathMapper()) diff --git a/src/Symfony/Component/Form/Extension/Validator/EventListener/DelegatingValidationListener.php b/src/Symfony/Component/Form/Extension/Validator/EventListener/DelegatingValidationListener.php index 395958c64d..362093d6f7 100644 --- a/src/Symfony/Component/Form/Extension/Validator/EventListener/DelegatingValidationListener.php +++ b/src/Symfony/Component/Form/Extension/Validator/EventListener/DelegatingValidationListener.php @@ -18,7 +18,6 @@ use Symfony\Component\Form\FormError; use Symfony\Component\Form\FormEvents; use Symfony\Component\Form\Event\DataEvent; use Symfony\Component\Form\Exception\FormException; -use Symfony\Component\Form\Util\VirtualFormAwareIterator; use Symfony\Component\Form\Util\PropertyPath; use Symfony\Component\Validator\Constraint; use Symfony\Component\Validator\ValidatorInterface; diff --git a/src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php b/src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php index db014eca12..6c8bc9c92f 100644 --- a/src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php +++ b/src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php @@ -91,11 +91,18 @@ class ViolationMapper } } - $this->scope->addError(new FormError( - $violation->getMessageTemplate(), - $violation->getMessageParameters(), - $violation->getMessagePluralization() - )); + // Only add the error if the form is synchronized + // If the form is not synchronized, it already contains an + // error about being invalid. Further errors could be a result + // of the failed transformation and thus should not be + // displayed. + if ($this->scope->isSynchronized()) { + $this->scope->addError(new FormError( + $violation->getMessageTemplate(), + $violation->getMessageParameters(), + $violation->getMessagePluralization() + )); + } } /** @@ -204,7 +211,7 @@ class ViolationMapper // Process child form $scope = $scope->get($it->current()); - if ($scope->getAttribute('virtual')) { + if ($scope->getConfig()->getVirtual()) { // Form is virtual // Cut the piece out of the property path and proceed $propertyPathBuilder->remove($i); diff --git a/src/Symfony/Component/Form/FormConfig.php b/src/Symfony/Component/Form/FormConfig.php index d979f94949..2ff8bdf604 100644 --- a/src/Symfony/Component/Form/FormConfig.php +++ b/src/Symfony/Component/Form/FormConfig.php @@ -43,6 +43,11 @@ class FormConfig implements FormConfigInterface */ private $mapped; + /** + * @var Boolean + */ + private $virtual; + /** * @var array */ @@ -289,6 +294,14 @@ class FormConfig implements FormConfigInterface return $this->mapped; } + /** + * {@inheritdoc} + */ + public function getVirtual() + { + return $this->virtual; + } + /** * {@inheritdoc} */ @@ -537,6 +550,20 @@ class FormConfig implements FormConfigInterface return $this; } + /** + * Sets whether the form should be virtual. + * + * @param Boolean $virtual Whether the form should be virtual. + * + * @return self The configuration object. + */ + public function setVirtual($virtual) + { + $this->virtual = $virtual; + + return $this; + } + /** * Set the types. * diff --git a/src/Symfony/Component/Form/FormConfigInterface.php b/src/Symfony/Component/Form/FormConfigInterface.php index 11c8f3f852..acceb84477 100644 --- a/src/Symfony/Component/Form/FormConfigInterface.php +++ b/src/Symfony/Component/Form/FormConfigInterface.php @@ -47,6 +47,17 @@ interface FormConfigInterface */ function getMapped(); + /** + * Returns whether the form should be virtual. + * + * When mapping data to the children of a form, the data mapper + * should ignore virtual forms and map to the children of the + * virtual form instead. + * + * @return Boolean Whether the form is virtual. + */ + function getVirtual(); + /** * Returns the form types used to construct the form. * diff --git a/src/Symfony/Component/Form/Tests/Extension/Validator/ViolationMapper/ViolationMapperTest.php b/src/Symfony/Component/Form/Tests/Extension/Validator/ViolationMapper/ViolationMapperTest.php index 910f800930..317e962daf 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Validator/ViolationMapper/ViolationMapperTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Validator/ViolationMapper/ViolationMapperTest.php @@ -12,6 +12,10 @@ namespace Symfony\Component\Form\Tests\Extension\Validator\ViolationMapper; use Symfony\Component\Form\Extension\Validator\ViolationMapper\ViolationMapper; +use Symfony\Component\Form\Exception\TransformationFailedException; +use Symfony\Component\Form\CallbackTransformer; +use Symfony\Component\Form\Form; +use Symfony\Component\Form\FormConfig; use Symfony\Component\Form\FormError; use Symfony\Component\Form\Util\PropertyPath; use Symfony\Component\Form\FormBuilder; @@ -35,11 +39,6 @@ class ViolationMapperTest extends \PHPUnit_Framework_TestCase */ private $dispatcher; - /** - * @var \PHPUnit_Framework_MockObject_MockObject - */ - private $factory; - /** * @var ViolationMapper */ @@ -62,27 +61,27 @@ class ViolationMapperTest extends \PHPUnit_Framework_TestCase } $this->dispatcher = $this->getMock('Symfony\Component\EventDispatcher\EventDispatcherInterface'); - $this->factory = $this->getMock('Symfony\Component\Form\FormFactoryInterface'); $this->mapper = new ViolationMapper(); $this->message = 'Message'; $this->params = array('foo' => 'bar'); } - protected function getBuilder($name = 'name', $propertyPath = null, $dataClass = null, $errorMapping = array(), $virtual = false) + protected function getForm($name = 'name', $propertyPath = null, $dataClass = null, $errorMapping = array(), $virtual = false, $synchronized = true) { - $builder = new FormBuilder($name, $dataClass, $this->dispatcher, $this->factory); - $builder->setPropertyPath(new PropertyPath($propertyPath ?: $name)); - $builder->setAttribute('error_mapping', $errorMapping); - $builder->setAttribute('virtual', $virtual); - $builder->setErrorBubbling(false); - $builder->setMapped(true); + $config = new FormConfig($name, $dataClass, $this->dispatcher); + $config->setMapped(true); + $config->setVirtual($virtual); + $config->setPropertyPath($propertyPath); + $config->setAttribute('error_mapping', $errorMapping); - return $builder; - } + if (!$synchronized) { + $config->appendClientTransformer(new CallbackTransformer( + function ($normData) { return $normData; }, + function () { throw new TransformationFailedException(); } + )); + } - protected function getForm($name = 'name', $propertyPath = null, $dataClass = null, $errorMapping = array(), $virtual = false) - { - return $this->getBuilder($name, $propertyPath, $dataClass, $errorMapping, $virtual)->getForm(); + return new Form($config); } /** @@ -1338,4 +1337,21 @@ class ViolationMapperTest extends \PHPUnit_Framework_TestCase $this->assertEquals(array($this->getFormError()), $grandChild->getErrors(), $grandChildName. ' should have an error, but has none'); } } + + public function testDontMapToUnsynchronizedForms() + { + $violation = $this->getConstraintViolation('children[address].data.street'); + $parent = $this->getForm('parent'); + $child = $this->getForm('address', 'address', null, array(), false, false); + + $parent->add($child); + + // bind to invoke the transformer and mark the form unsynchronized + $parent->bind(array()); + + $this->mapper->mapViolation($violation, $parent); + + $this->assertFalse($parent->hasErrors(), $parent->getName() . ' should not have an error, but has one'); + $this->assertFalse($child->hasErrors(), $child->getName() . ' should not have an error, but has one'); + } } diff --git a/src/Symfony/Component/Form/UnmodifiableFormConfig.php b/src/Symfony/Component/Form/UnmodifiableFormConfig.php index 02519fab47..ab925316c1 100644 --- a/src/Symfony/Component/Form/UnmodifiableFormConfig.php +++ b/src/Symfony/Component/Form/UnmodifiableFormConfig.php @@ -42,6 +42,11 @@ class UnmodifiableFormConfig implements FormConfigInterface */ private $mapped; + /** + * @var Boolean + */ + private $virtual; + /** * @var array */ @@ -118,6 +123,7 @@ class UnmodifiableFormConfig implements FormConfigInterface $this->name = $config->getName(); $this->propertyPath = $config->getPropertyPath(); $this->mapped = $config->getMapped(); + $this->virtual = $config->getVirtual(); $this->types = $config->getTypes(); $this->clientTransformers = $config->getClientTransformers(); $this->normTransformers = $config->getNormTransformers(); @@ -164,6 +170,14 @@ class UnmodifiableFormConfig implements FormConfigInterface return $this->mapped; } + /** + * {@inheritdoc} + */ + public function getVirtual() + { + return $this->virtual; + } + /** * {@inheritdoc} */ diff --git a/src/Symfony/Component/Form/Util/VirtualFormAwareIterator.php b/src/Symfony/Component/Form/Util/VirtualFormAwareIterator.php index fe43d1984b..2caeff504e 100644 --- a/src/Symfony/Component/Form/Util/VirtualFormAwareIterator.php +++ b/src/Symfony/Component/Form/Util/VirtualFormAwareIterator.php @@ -28,7 +28,6 @@ class VirtualFormAwareIterator extends \ArrayIterator implements \RecursiveItera public function hasChildren() { - return $this->current()->hasAttribute('virtual') - && $this->current()->getAttribute('virtual'); + return $this->current()->getConfig()->getVirtual(); } }