-
Notifications
You must be signed in to change notification settings - Fork 2.5k
bugfix removing multiple fieldsets does't work #7446
Conversation
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
fixes #7443 |
Please add a unit test |
So the issue is modify and read the iterator (PriorityList) in the same loop. Should this to be fixed in PriorityList instead? |
@Maks3w I don't think this should be allowed in PriorityList. Also don't see how this could be implemented.
does not feel right |
@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. |
I delegate to other @zendframework/community-review-team member for their opinions. To have a temportal |
I have found commit witch introduced the bug |
$removed[] = $name; | ||
} | ||
|
||
foreach ($removed as $name) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
bugfix removing multiple fieldsets does't work
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