Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

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.

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

Merged
merged 2 commits into from
Feb 17, 2014

Conversation

norberttech
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #10256, #10263
License MIT
Doc PR

Unseting key on iterator is not an solution because in php "Array assignment always involves value copying. " So if \IteratorAggregate implementation create new iterator from array (just like Doctrine ArrayCollection does) such operation will not affect original data.

I'm just not sure if it's good to use Doctirn ArrayCollection in this test case or maybe its better to create own implementation of such Collection as a fixture.

fabpot added a commit that referenced this pull request Feb 17, 2014
…ResizeFormListener." (norzechowicz)

This PR was merged into the 2.3 branch.

Discussion
----------

[Form] Revert "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 | #10256, #10263
| License       | MIT
| Doc PR        |

Unseting key on iterator is not an solution because in php "Array assignment always involves value copying. " So if \IteratorAggregate implementation create new iterator from array (just like [Doctrine ArrayCollection](https://github.com/doctrine/common/blob/2.3/lib/Doctrine/Common/Collections/ArrayCollection.php#L362) does)  such operation will not affect original data.

I'm just not sure if it's good to use Doctirn ArrayCollection in this test case or maybe its better to create own implementation of such Collection as a fixture.

Commits
-------

f62e30d Revert "Fix "Array was modified outside object" in ResizeFormListener."
462b7af Added failing test
@fabpot fabpot merged commit f62e30d into symfony:2.3 Feb 17, 2014
@Koc
Copy link
Contributor

Koc commented Feb 18, 2014

@fabpot should we create new ticket for original issue as fix for it was reverted?

fabpot added a commit that referenced this pull request Mar 19, 2014
…rmListener. (Chekote)

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.