From 59d6b5513759bc7d96f3c8d347e34133fe27a0bc Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Mon, 21 May 2012 13:02:50 +0200 Subject: [PATCH] [Form] Fixed: error mapping aborts if reaching an unsynchronized form --- .../ViolationMapper/ViolationMapper.php | 25 ++- .../ViolationMapper/ViolationMapperTest.php | 154 +++++++++++++----- 2 files changed, 129 insertions(+), 50 deletions(-) diff --git a/src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php b/src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php index 195ae1b234..5e4778627c 100644 --- a/src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php +++ b/src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php @@ -54,6 +54,18 @@ class ViolationMapper $relativePath = $this->reconstructPath($violationPath, $form); $match = false; + // In general, mapping happens from the root form to the leaf forms + // First, the rules of the root form are applied to determine + // the subsequent descendant. The rules of this descendant are then + // applied to find the next and so on, until we have found the + // most specific form that matches the violation. + + // If any of the forms found in this process is not synchronized, + // mapping is aborted. Non-synchronized forms could not reverse + // transform the value entered by the user, thus any further violations + // caused by the (invalid) reverse transformed value should be + // ignored. + if (null !== $relativePath) { // Set the scope to the root of the relative path // This root will usually be $form. If the path contains @@ -62,13 +74,16 @@ class ViolationMapper $this->setScope($relativePath->getRoot()); $it = new PropertyPathIterator($relativePath); - while (null !== ($child = $this->matchChild($it))) { + while ($this->scope->isSynchronized() && null !== ($child = $this->matchChild($it))) { $this->setScope($child); $it->next(); $match = true; } } + // This case happens if an error happened in the data under a + // virtual form that does not match any of the children of + // the virtual form. if (!$match) { // If we could not map the error to anything more specific // than the root element, map it to the innermost directly @@ -80,7 +95,7 @@ class ViolationMapper // The overhead of setScope() is not needed anymore here $this->scope = $form; - while ($it->valid() && $it->mapsForm()) { + while ($this->scope->isSynchronized() && $it->valid() && $it->mapsForm()) { if (!$this->scope->has($it->current())) { // Break if we find a reference to a non-existing child break; @@ -94,17 +109,13 @@ class ViolationMapper // Follow dot rules until we have the final target $mapping = $this->scope->getAttribute('error_mapping'); - while (isset($mapping['.'])) { + while ($this->scope->isSynchronized() && isset($mapping['.'])) { $dotRule = new MappingRule($this->scope, '.', $mapping['.']); $this->scope = $dotRule->getTarget(); $mapping = $this->scope->getAttribute('error_mapping'); } // 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(), 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 d0674269be..068dfb0a88 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Validator/ViolationMapper/ViolationMapperTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Validator/ViolationMapper/ViolationMapperTest.php @@ -102,6 +102,117 @@ class ViolationMapperTest extends \PHPUnit_Framework_TestCase return new FormError($this->message, $this->params); } + public function testMapToVirtualFormIfDataDoesNotMatch() + { + $violation = $this->getConstraintViolation('children[address].data.foo'); + $parent = $this->getForm('parent'); + $child = $this->getForm('address', 'address', null, array(), true); + $grandChild = $this->getForm('street'); + + $parent->add($child); + $child->add($grandChild); + + $this->mapper->mapViolation($violation, $parent); + + $this->assertFalse($parent->hasErrors(), $parent->getName() . ' should not have an error, but has one'); + $this->assertEquals(array($this->getFormError()), $child->getErrors(), $child->getName() . ' should have an error, but has none'); + $this->assertFalse($grandChild->hasErrors(), $grandChild->getName() . ' should not have an error, but has one'); + } + + public function testFollowDotRules() + { + $violation = $this->getConstraintViolation('data.foo'); + $parent = $this->getForm('parent', null, null, array( + 'foo' => 'address', + )); + $child = $this->getForm('address', null, null, array( + '.' => 'street', + )); + $grandChild = $this->getForm('street', null, null, array( + '.' => 'name', + )); + $grandGrandChild = $this->getForm('name'); + + $parent->add($child); + $child->add($grandChild); + $grandChild->add($grandGrandChild); + + $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'); + $this->assertFalse($grandChild->hasErrors(), $grandChild->getName() . ' should not have an error, but has one'); + $this->assertEquals(array($this->getFormError()), $grandGrandChild->getErrors(), $grandGrandChild->getName() . ' should have an error, but has none'); + } + + public function testAbortMappingIfNotSynchronized() + { + $violation = $this->getConstraintViolation('children[address].data.street'); + $parent = $this->getForm('parent'); + $child = $this->getForm('address', 'address', null, array(), false, false); + // even though "street" is synchronized, it should not have any errors + // due to its parent not being synchronized + $grandChild = $this->getForm('street' , 'street'); + + $parent->add($child); + $child->add($grandChild); + + // 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'); + $this->assertFalse($grandChild->hasErrors(), $grandChild->getName() . ' should not have an error, but has one'); + } + + public function testAbortVirtualFormMappingIfNotSynchronized() + { + $violation = $this->getConstraintViolation('children[address].children[street].data.foo'); + $parent = $this->getForm('parent'); + $child = $this->getForm('address', 'address', null, array(), true, false); + // even though "street" is synchronized, it should not have any errors + // due to its parent not being synchronized + $grandChild = $this->getForm('street' , 'street', null, array(), true); + + $parent->add($child); + $child->add($grandChild); + + // 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'); + $this->assertFalse($grandChild->hasErrors(), $grandChild->getName() . ' should not have an error, but has one'); + } + + public function testAbortDotRuleMappingIfNotSynchronized() + { + $violation = $this->getConstraintViolation('data.address'); + $parent = $this->getForm('parent'); + $child = $this->getForm('address', 'address', null, array( + '.' => 'street', + ), false, false); + // even though "street" is synchronized, it should not have any errors + // due to its parent not being synchronized + $grandChild = $this->getForm('street'); + + $parent->add($child); + $child->add($grandChild); + + // 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'); + $this->assertFalse($grandChild->hasErrors(), $grandChild->getName() . ' should not have an error, but has one'); + } + public function provideDefaultTests() { // The mapping must be deterministic! If a child has the property path "[street]", @@ -1337,47 +1448,4 @@ 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'); - } - - public function testFollowDotRules() - { - $violation = $this->getConstraintViolation('data.foo'); - $parent = $this->getForm('parent', null, null, array( - 'foo' => 'address', - )); - $child = $this->getForm('address', null, null, array( - '.' => 'street', - )); - $grandChild = $this->getForm('street', null, null, array( - '.' => 'name', - )); - $grandGrandChild = $this->getForm('name'); - - $parent->add($child); - $child->add($grandChild); - $grandChild->add($grandGrandChild); - - $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'); - $this->assertFalse($grandChild->hasErrors(), $grandChild->getName() . ' should not have an error, but has one'); - $this->assertEquals(array($this->getFormError()), $grandGrandChild->getErrors(), $grandGrandChild->getName() . ' should have an error, but has none'); - } }