From 9dfebd529edde1f65664a37b009179a78d496335 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Fri, 28 Mar 2014 10:30:00 +0100 Subject: [PATCH] [Form] Disabled violation mapping of unsubmitted forms --- src/Symfony/Component/Form/CHANGELOG.md | 1 + .../ViolationMapper/ViolationMapper.php | 5 +- src/Symfony/Component/Form/Form.php | 16 ++--- .../Component/Form/Tests/CompoundFormTest.php | 16 ----- .../ViolationMapper/ViolationMapperTest.php | 64 ++++++++++++++++++- 5 files changed, 73 insertions(+), 29 deletions(-) diff --git a/src/Symfony/Component/Form/CHANGELOG.md b/src/Symfony/Component/Form/CHANGELOG.md index c210f9e792..344bd9c0ad 100644 --- a/src/Symfony/Component/Form/CHANGELOG.md +++ b/src/Symfony/Component/Form/CHANGELOG.md @@ -11,6 +11,7 @@ CHANGELOG * [BC BREAK] added two optional parameters to FormInterface::getErrors() and changed the method to return a Symfony\Component\Form\FormErrorIterator instance instead of an array + * errors mapped to unsubmitted forms are discarded now 2.4.0 ----- diff --git a/src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php b/src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php index 15ceb0cf08..e0eac4dacd 100644 --- a/src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php +++ b/src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php @@ -294,6 +294,9 @@ class ViolationMapper implements ViolationMapperInterface */ private function acceptsErrors(FormInterface $form) { - return $this->allowNonSynchronized || $form->isSynchronized(); + // Ignore non-submitted forms. This happens, for example, in PATCH + // requests. + // https://github.com/symfony/symfony/pull/10567 + return $form->isSubmitted() && ($this->allowNonSynchronized || $form->isSynchronized()); } } diff --git a/src/Symfony/Component/Form/Form.php b/src/Symfony/Component/Form/Form.php index a5f98c16f5..5bc97b12ee 100644 --- a/src/Symfony/Component/Form/Form.php +++ b/src/Symfony/Component/Form/Form.php @@ -556,8 +556,10 @@ class Form implements \IteratorAggregate, FormInterface } foreach ($this->children as $name => $child) { - if (array_key_exists($name, $submittedData) || $clearMissing) { - $child->submit(isset($submittedData[$name]) ? $submittedData[$name] : null, $clearMissing); + $isSubmitted = array_key_exists($name, $submittedData); + + if ($isSubmitted || $clearMissing) { + $child->submit($isSubmitted ? $submittedData[$name] : null, $clearMissing); unset($submittedData[$name]); if (null !== $this->clickedButton) { @@ -739,18 +741,12 @@ class Form implements \IteratorAggregate, FormInterface return false; } - if (count($this->errors) > 0) { - return false; - } - if ($this->isDisabled()) { return true; } - foreach ($this->children as $child) { - if ($child->isSubmitted() && !$child->isValid()) { - return false; - } + if (count($this->getErrors(true)) > 0) { + return false; } return true; diff --git a/src/Symfony/Component/Form/Tests/CompoundFormTest.php b/src/Symfony/Component/Form/Tests/CompoundFormTest.php index 8f861788d3..59dd36d138 100644 --- a/src/Symfony/Component/Form/Tests/CompoundFormTest.php +++ b/src/Symfony/Component/Form/Tests/CompoundFormTest.php @@ -51,22 +51,6 @@ class CompoundFormTest extends AbstractFormTest $this->assertFalse($this->form->isValid()); } - public function testValidIfChildIsNotSubmitted() - { - $this->form->add($this->getBuilder('firstName')->getForm()); - $this->form->add($this->getBuilder('lastName')->getForm()); - - $this->form->submit(array( - 'firstName' => 'Bernhard', - )); - - // "lastName" is not "valid" because it was not submitted. This happens - // for example in PATCH requests. The parent form should still be - // considered valid. - - $this->assertTrue($this->form->isValid()); - } - public function testDisabledFormsValidEvenIfChildrenInvalid() { $form = $this->getBuilder('person') 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 c2db73eb77..7e063fc466 100644 --- a/src/Symfony/Component/Form/Tests/Extension/Validator/ViolationMapper/ViolationMapperTest.php +++ b/src/Symfony/Component/Form/Tests/Extension/Validator/ViolationMapper/ViolationMapperTest.php @@ -125,6 +125,8 @@ class ViolationMapperTest extends \PHPUnit_Framework_TestCase $parent->add($child); $child->add($grandChild); + $parent->submit(array()); + $this->mapper->mapViolation($violation, $parent); $this->assertCount(0, $parent->getErrors(), $parent->getName().' should not have an error, but has one'); @@ -150,6 +152,8 @@ class ViolationMapperTest extends \PHPUnit_Framework_TestCase $child->add($grandChild); $grandChild->add($grandGrandChild); + $parent->submit(array()); + $this->mapper->mapViolation($violation, $parent); $this->assertCount(0, $parent->getErrors(), $parent->getName().' should not have an error, but has one'); @@ -170,7 +174,7 @@ class ViolationMapperTest extends \PHPUnit_Framework_TestCase $parent->add($child); $child->add($grandChild); - // submit to invoke the transformer and mark the form unsynchronized + // invoke the transformer and mark the form unsynchronized $parent->submit(array()); $this->mapper->mapViolation($violation, $parent); @@ -194,7 +198,7 @@ class ViolationMapperTest extends \PHPUnit_Framework_TestCase $parent->add($child); $child->add($grandChild); - // submit to invoke the transformer and mark the form unsynchronized + // invoke the transformer and mark the form unsynchronized $parent->submit(array()); $this->mapper->mapViolation($violation, $parent); @@ -204,6 +208,54 @@ class ViolationMapperTest extends \PHPUnit_Framework_TestCase $this->assertCount(0, $grandChild->getErrors(), $grandChild->getName().' should not have an error, but has one'); } + public function testAbortMappingIfNotSubmitted() + { + $violation = $this->getConstraintViolation('children[address].data.street'); + $parent = $this->getForm('parent'); + $child = $this->getForm('address', 'address'); + $grandChild = $this->getForm('street' , 'street'); + + $parent->add($child); + $child->add($grandChild); + + // Disable automatic submission of missing fields + $parent->submit(array(), false); + $child->submit(array(), false); + + // $grandChild is not submitted + + $this->mapper->mapViolation($violation, $parent); + + $this->assertCount(0, $parent->getErrors(), $parent->getName().' should not have an error, but has one'); + $this->assertCount(0, $child->getErrors(), $child->getName().' should not have an error, but has one'); + $this->assertCount(0, $grandChild->getErrors(), $grandChild->getName().' should not have an error, but has one'); + } + + public function testAbortDotRuleMappingIfNotSubmitted() + { + $violation = $this->getConstraintViolation('data.address'); + $parent = $this->getForm('parent'); + $child = $this->getForm('address', 'address', null, array( + '.' => 'street', + )); + $grandChild = $this->getForm('street'); + + $parent->add($child); + $child->add($grandChild); + + // Disable automatic submission of missing fields + $parent->submit(array(), false); + $child->submit(array(), false); + + // $grandChild is not submitted + + $this->mapper->mapViolation($violation, $parent); + + $this->assertCount(0, $parent->getErrors(), $parent->getName().' should not have an error, but has one'); + $this->assertCount(0, $child->getErrors(), $child->getName().' should not have an error, but has one'); + $this->assertCount(0, $grandChild->getErrors(), $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]", @@ -743,6 +795,8 @@ class ViolationMapperTest extends \PHPUnit_Framework_TestCase $parent->add($child); $child->add($grandChild); + $parent->submit(array()); + $this->mapper->mapViolation($violation, $parent); if (self::LEVEL_0 === $target) { @@ -1211,6 +1265,8 @@ class ViolationMapperTest extends \PHPUnit_Framework_TestCase $parent->add($distraction); } + $parent->submit(array()); + $this->mapper->mapViolation($violation, $parent); if ($target !== self::LEVEL_0) { @@ -1396,6 +1452,8 @@ class ViolationMapperTest extends \PHPUnit_Framework_TestCase $parent->add($errorChild); $child->add($grandChild); + $parent->submit(array()); + $this->mapper->mapViolation($violation, $parent); if (self::LEVEL_0 === $target) { @@ -1459,6 +1517,8 @@ class ViolationMapperTest extends \PHPUnit_Framework_TestCase $parent->add($child); $child->add($grandChild); + $parent->submit(array()); + $this->mapper->mapViolation($violation, $parent); if (self::LEVEL_0 === $target) {