diogo/symfony
Archived
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

bug #10410 [Form] Fix "Array was modified outside object" in ResizeFormListener. (Chekote)

Browse Source 
This PR was squashed before being merged into the 2.3 branch (closes #10410).

Discussion
----------

[Form] Fix "Array was modified outside object" in ResizeFormListener.

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #10405
| License       | MIT
| Doc PR        |

This is the second pull request for this issue. The history of this is as follows:

Original Fix was added Feb 11th under Pull Request #10232.
Users began complaining of Doctrine ArrayCollection not being updated in forms.
Revert was added Feb 15th under Pull Request #10269.
Issue #10405 was opened on 7th Mar for the original bug.

Pull Request #10269 has a failing test that illustrates users concerns.
I have added failing tests to this pull request to illustrate the problems described in #10405.

All tests now pass, and all forms of $data are now supported, including arrays, DoctrineCollection, ArrayObject, and other IteratorAggregates.

__Details as follows:__

The onSubmit() method of the ResizeFormListener class is assuming the data is an array, and calling unset directly inside a foreach. This works fine in most scenarios, but if data is an instance of IteratorAggregate that is iterating over $data by reference, it breaks with the following error:

Symfony\Component\Form\Extension\Core\EventListener\ResizeFormListener::onSubmit(): ArrayIterator::next(): Array was modified outside object and internal position is no longer valid in ./vendor/symfony/symfony/src/Symfony/Component/Form/Extension/Core/EventListener/ResizeFormListener.php line 142

This is because the foreach loop is using an Iterator in the background, but the ResizeFormListener has unset the underlying data directly, causing the value that the Iterators internal pointer is pointing to to change.

The Iterator provided by IteratorAggregate may or may not have the ability to modify it's underlying data. So it is not possible to rely on the Iterator to modify the data. Instead, it is simpler to avoid modifying $data at all while we are iterating over it. So instead, we simply iterate over $data once to determine the keys we need to delete, and store them. Then we iterate over the keys and delete them from $data.

Commits
-------

aa63fae [Form] Fix "Array was modified outside object" in ResizeFormListener.
This commit is contained in:
Fabien Potencier 2014-03-19 17:44:48 +01:00
commit fc251dbcd4
2 changed files with 21 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
src/Symfony/Component/Form/Extension/Core/EventListener/ResizeFormListener.php
View File

@ -139,11 +139,17 @@ class ResizeFormListener implements EventSubscriberInterface
// The data mapper only adds, but does not remove items, so do this
// here
if ($this->allowDelete) {
$toDelete = array();
foreach ($data as $name => $child) {
if (!$form->has($name)) {
unset($data[$name]);
$toDelete[] = $name;
}
}
foreach ($toDelete as $name) {
unset($data[$name]);
}
}
$event->setData($data);

15
src/Symfony/Component/Form/Tests/Extension/Core/EventListener/ResizeFormListenerTest.php
View File

@ -254,7 +254,20 @@ class ResizeFormListenerTest extends \PHPUnit_Framework_TestCase
$this->assertEquals(array(), $event->getData());
}
public function testOnSubmitDealsWithIteratorAggregate()
public function testOnSubmitDealsWithObjectBackedIteratorAggregate()
{
$this->form->add($this->getForm('1'));
$data = new \ArrayObject(array(0 => 'first', 1 => 'second', 2 => 'third'));
$event = new FormEvent($this->form, $data);
$listener = new ResizeFormListener('text', array(), false, true);
$listener->onSubmit($event);
$this->assertArrayNotHasKey(0, $event->getData());
$this->assertArrayNotHasKey(2, $event->getData());
}
public function testOnSubmitDealsWithArrayBackedIteratorAggregate()
{
$this->form->add($this->getForm('1'));