Can't use remove() in a loop

Bug #458371 reported by Anthony Bush
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
CoughPHP
Fix Committed
Medium
Anthony Bush
1.4
Fix Committed
Medium
Anthony Bush

Bug Description

The following code does not work (for collections containing more than one element):

 <?php
 foreach ($collection as $element) {
  $collection->remove($element);
 }
 assert(count($collection) == 0);

Only the first element is removed. However, this code does work:

 <?php
 foreach ($collection as $element) {
  $collection->offsetUnset($element);
 }
 assert(count($collection) == 0);

and this works too:

 <?php
 foreach ($collection as $element) {
  unset($collection[$element]);
 }
 assert(count($collection) == 0);

I've found that the reason for this is due to CoughCollection saving the removed element (for later processing) AFTER the unset is performed.

 $this->offsetUnset($key);
 $this->removedElements[] = $objectToRemove;

Changing this to:

 $this->removedElements[] = $objectToRemove;
 $this->offsetUnset($key);

fixes the problem. There's two places in CoughCollection to fix, removeByKey(), and removeByReference().

Anthony Bush (awbush)
Changed in coughphp:
status: Confirmed → Fix Committed
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.