Skip to content

Commit

Permalink
[Form] Fixed: error mapping aborts if reaching an unsynchronized form
Browse files Browse the repository at this point in the history
  • Loading branch information
@webmozart
webmozart committed May 22, 2012
1 parent 9eda5f5 commit 59d6b55
Show file tree
Hide file tree
Showing 2 changed files with 129 additions and 50 deletions.
25 changes: 18 additions & 7 deletions src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php
View file
Expand Up @@ -54,6 +54,18 @@ public function mapViolation(ConstraintViolation $violation, FormInterface $form
$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
Expand All @@ -62,13 +74,16 @@ public function mapViolation(ConstraintViolation $violation, FormInterface $form
$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
Expand All @@ -80,7 +95,7 @@ public function mapViolation(ConstraintViolation $violation, FormInterface $form
// 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;
Expand All @@ -94,17 +109,13 @@ public function mapViolation(ConstraintViolation $violation, FormInterface $form
// 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(),
Expand Down
154 changes: 111 additions & 43 deletions src/Symfony/Component/Form/Tests/Extension/Validator/ViolationMapper/ViolationMapperTest.php
View file
Expand Up @@ -102,6 +102,117 @@ protected function getFormError()
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]",
Expand Down Expand Up @@ -1337,47 +1448,4 @@ public function testVirtualFormErrorMapping($target, $childName, $childPath, $gr
$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');
}
}

0 comments on commit 59d6b55

Please sign in to comment.