10000 [Form] Fixed violation mapping if multiple forms are using the same (or part of the same) property path by alekitto · Pull Request #17099 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] Fixed violation mapping if multiple forms are using the same (or part of the same) property path #17099

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

[Form] Fixed violation mapping if multiple forms are using the same (or part of the same) property path #17099

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75af679
[Form] Fixed violation mapping if multiple forms are using the same (…
alekitto Dec 22, 2015
06728b7
yoda style comparisons
alekitto Dec 28, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[Form] Fixed violation mapping if multiple forms are using the same (… 
…or part of the same) property path

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #5656
| License       | MIT
| Doc PR        |
  • Loading branch information
@alekitto
alekitto committed Dec 28, 2015
commit 75af6797b85263dc891bf6f41b08fc252942c566
55 changes: 20 additions & 35 deletions src/Symfony/Component/Form/Extension/Validator/ViolationMapper/ViolationMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,12 +147,9 @@ public function mapViolation(ConstraintViolation $violation, FormInterface $form
*/
private function matchChild(FormInterface $form, PropertyPathIteratorInterface $it)
{
// Remember at what property path underneath "data"
// we are looking. Check if there is a child with that
// path, otherwise increase path by one more piece
$target = null;
$chunk = '';
$foundChild = null;
$foundAtIndex = 0;
$foundAtIndex = null;

// Construct mapping rules for the given form
$rules = array();
Expand All @@ -164,17 +161,11 @@ private function matchChild(FormInterface $form, PropertyPathIteratorInterface $
}
}

// Skip forms inheriting their parent data when iterating the children
$childIterator = new \RecursiveIteratorIterator(
$children = iterator_to_array(new \RecursiveIteratorIterator(
new InheritDataAwareIterator($form)
);

// Make the path longer until we find a matching child
while (true) {
if (!$it->valid()) {
return;
}
));

while ($it->valid()) {
if ($it->isIndex()) {
$chunk .= '['.$it->current().']';
} else {
Expand All @@ -196,33 +187,27 @@ private function matchChild(FormInterface $form, PropertyPathIteratorInterface $
}
}

// Test children unless we already found one
if (null === $foundChild) {
foreach ($childIterator as $child) {
/* @var FormInterface $child */
$childPath = (string) $child->getPropertyPath();

// Child found, mark as return value
if ($chunk === $childPath) {
$foundChild = $child;
$foundAtIndex = $it->key();
}
/** @var FormInterface $child */
foreach ($children as $key => $child) {
$childPath = (string) $child->getPropertyPath();
if ($childPath === $chunk) {
$target = $child;
$foundAtIndex = $it->key();
} elseif (strpos($childPath, $chunk) === 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we usually prefer yoda style (swapping left and right hands of comparisons)

continue;
}

unset($children[$key]);
}

// Add element to the chunk
$it->next();
}

// If we reached the end of the path or if there are no
// more matching mapping rules, return the found child
if (null !== $foundChild && (!$it->valid() || count($rules) === 0)) {
// Reset index in case we tried to find mapping
// rules further down the path
$it->seek($foundAtIndex);

return $foundChild;
}
if ($foundAtIndex !== null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yoda style

$it->seek($foundAtIndex);
}

return $target;
}

/**
Expand Down
21 changes: 21 additions & 0 deletions src/Symfony/Component/Form/Tests/Extension/Validator/ViolationMapper/ViolationMapperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1474,4 +1474,25 @@ public function testErrorMappingForFormInheritingParentData($target, $childName,
$this->assertEquals(array($this->getFormError()), $grandChild->getErrors(), $grandChildName.' should have an error, but has none');
}
}

public function testBacktrackIfSeveralSubFormsWithSamePropertyPath()
{
$violation = $this->getConstraintViolation('data.address[street]');
$parent = $this->getForm('parent');
$child1 = $this->getForm('subform1', 'address');
$child2 = $this->getForm('subform2', 'address');
$grandChild = $this->getForm('street');

$parent->add($child1);
$parent->add($child2);
$child2->add($grandChild);

$this->mapper->mapViolation($violation, $parent);

// The error occurred on the child of the second form with the same path
$this->assertCount(0, $parent->getErrors(), $parent->getName().' should not have an error, but has one');
$this->assertCount(0, $child1->getErrors(), $child1->getName().' should not have an error, but has one');
$this->assertCount(0, $child2->getErrors(), $child2->getName().' should not have an error, but has one');
$this->assertEquals(array($this->getFormError()), $grandChild->getErrors(), $grandChild->getName().' should have an error, but has none');
}
}
0