Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

bugfix removing multiple fieldsets does't work #7446

Merged
merged 3 commits into from
May 6, 2015

Conversation

svycka
Copy link
Contributor

@svycka svycka commented Apr 14, 2015

at the same time iterating collection items and removing causes problem:
When we have 3 items but we delete second and third. It fails and does not remove third element. So we have to move remove out of foreach

at the same time iterating collection items and removing causes problem:
When we have 3 items but we delete second and third. It fails and does not remove third element. So we have to move remove out of foreach
@svycka
Copy link
Contributor Author

svycka commented Apr 14, 2015

fixes #7443

@Maks3w
Copy link
Member

Maks3w commented Apr 14, 2015

Please add a unit test

@Maks3w
Copy link
Member

Maks3w commented Apr 14, 2015

So the issue is modify and read the iterator (PriorityList) in the same loop.

Should this to be fixed in PriorityList instead?

@svycka
Copy link
Contributor Author

svycka commented Apr 14, 2015

@Maks3w I don't think this should be allowed in PriorityList. Also don't see how this could be implemented.

foreach PriorityList
    remove
    remove
endforeach
PriorityList->commit

does not feel right

@svycka
Copy link
Contributor Author

svycka commented Apr 15, 2015

@Maks3w if you don't like how it is fixed I could change:

foreach ($this->iterator as $name => $elementOrFieldset) {
// to
foreach ($this->iterator->getIterator() as $name => $elementOrFieldset) {

this would also fix the issue.

@Maks3w
Copy link
Member

Maks3w commented Apr 15, 2015

I delegate to other @zendframework/community-review-team member for their opinions.

To have a temportal $removed array is weird for me

@svycka
Copy link
Contributor Author

svycka commented Apr 15, 2015

I have found commit witch introduced the bug
PR: #6240
commit: 9a5dd60#diff-31fe6580a729ea0519189ac9ef45928aL212

@Maks3w Maks3w added this to the 2.4.1 milestone May 5, 2015
$removed[] = $name;
}

foreach ($removed as $name) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just remove as we discover valid names?

Copy link
Member

Choose a reason for hiding this comment

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

Interestingly, because doing so causes unexpected behavior.

Copy link
Member

Choose a reason for hiding this comment

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

IMO this should be fixed in PriorityList instead

Copy link
Member

Choose a reason for hiding this comment

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

I've tried a few different approaches at this time. The problem is that doing removal while inside the loop leads to inconsistent results; the underlying iterator is not updated in place correctly. As such, the first pass is to determine which we can remove, and then we remove each.

weierophinney added a commit that referenced this pull request May 6, 2015
bugfix removing multiple fieldsets does't work
weierophinney added a commit that referenced this pull request May 6, 2015
@weierophinney weierophinney merged commit ae3ec5e into zendframework:master May 6, 2015
weierophinney added a commit that referenced this pull request May 6, 2015
@svycka svycka deleted the patch-2 branch May 6, 2015 15:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants