8000 [2.3] [Form] Modified iterator_to_array's 2nd parameter to false in ViolationMapper by issei-m · Pull Request #18761 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[2.3] [Form] Modified iterator_to_array's 2nd parameter to false in ViolationMapper #18761

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

[2.3] [Form] Modified iterator_to_array's 2nd parameter to false in ViolationMapper #18761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f1f58b5
[Form] fixed ViolationMapper bug
issei-m May 10, 2016
dc2f88d
[Form] fixed ViolationMapperTest
issei-m May 12, 2016
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
8 changes: 3 additions & 5 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 @@ -161,9 +161,7 @@ private function matchChild(FormInterface $form, PropertyPathIteratorInterface $
}
}

$children = iterator_to_array(new \RecursiveIteratorIterator(
new InheritDataAwareIterator($form)
));
$children = iterator_to_array(new \RecursiveIteratorIterator(new InheritDataAwareIterator($form)), false);

while ($it->valid()) {
if ($it->isIndex()) {
Expand All @@ -188,7 +186,7 @@ private function matchChild(FormInterface $form, PropertyPathIteratorInterface $
}

/** @var FormInterface $child */
foreach ($children as $key => $child) {
foreach ($children as $i => $child) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a specific reason for this change? If not, I like the original more. $i implies that it's being used for incrementation, but you're simply referring to the key here.

Copy link
Contributor Author
@issei-m issei-m May 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I have an intention to make the key of $children meaningless. (The name of iterator_to_array's 2nd parameter is $use_keys)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's used to unset something below. if you mean to make it meaningless, you should remove it altogether

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moreover, i implies it's just counter indeed. But also implies it's an integer, IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alekitto How do you think about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the change: in my opinion is more clear that the variable contains an integer counter, not a meaningful key name.
Moreover it makes it consistent with the existing codebase, for example: $i is used in DI Definition::removeMethodCall method in a similar way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alekitto Thank you for advicing!

$childPath = (string) $child->getPropertyPath();
if ($childPath === $chunk) {
$target = $child;
Expand All @@ -197,7 +195,7 @@ private function matchChild(FormInterface $form, PropertyPathIteratorInterface $
continue;
}

unset($children[$key]);
unset($children[$i]);
}

$it->next();
Expand Down
29 changes: 23 additions & 6 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 @@ -1477,22 +1477,39 @@ public function testErrorMappingForFormInheritingParentData($target, $childName,

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');
$child3 = $this->getForm('subform3', null, null, array(), true);
$child4 = $this->getForm('subform4', null, null, array(), true);
$grandChild1 = $this->getForm('street');
$grandChild2 = $this->getForm('street', '[sub_address1_street]');
$grandChild3 = $this->getForm('street', '[sub_address2_street]');

$parent->add($child1);
$parent->add($child2);
$child2->add($grandChild);
$parent->add($child3);
$parent->add($child4);
$child2->add($grandChild1);
$child3->add($grandChild2);
$child4->add($grandChild3);

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

$violation1 = $this->getConstraintViolation('data.address[street]');
$violation2 = $this->getConstraintViolation('data[sub_address1_street]');
$violation3 = $this->getConstraintViolation('data[sub_address2_street]');
$this->mapper->mapViolation($violation1, $parent);
$this->mapper->mapViolation($violation2, $parent);
$this->mapper->mapViolation($violation3, $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');
$this->assertCount(0, $child3->getErrors(), $child3->getName().' should not have an error, but has one');
$this->assertCount(0, $child4->getErrors(), $child4->getName().' should not have an error, but has one');
$this->assertEquals(array($this->getFormError($violation1, $grandChild1)), $grandChild1->getErrors(), $grandChild1->getName().' should have an error, but has none');
$this->assertEquals(array($this->getFormError($violation2, $grandChild2)), $grandChild2->getErrors(), $grandChild2->getName().' should have an error, but has none');
$this->assertEquals(array($this->getFormError($violation3, $grandChild3)), $grandChild3->getErrors(), $grandChild3->getName().' should have an error, but has none');
}
}
0