0

I'm currently working on a generic form creation class and had an issue yesterday. I made a snippet to reproduce the problem.

Essentially I want to delete elements that are grouped from the original elements array after the whole group has been drawn and I'm doing this while looping over the elements array.

The code snippet should cover the problem, am I missing something here? From my knowledge deleting an element while foreach is completely safe and legal since foreach internally only uses a copy that may be modified during the loop.

$ids = array('a' => array(), 'b' => array(), 'c' => array());
$groups['g1'] = array('a', 'c');
foreach($ids as $id => $element) {

    //var_dump($ids);
    $g_id = '';

    // search the id in all groups
    foreach($groups as $group_id => $group) {
        if(in_array($id, $group)) {
            $g_id = $group_id;
            break;
        }
    }

    // element is part of a group
    if($g_id !== '') {

        //echo $g_id;

        // element a and c gets unset within loop and should not be in $ids anymore
        foreach($groups[$g_id] as $field_id) {
            unset($ids[$field_id]);

            echo $field_id;
        }
        unset($groups[$g_id]);
    } else {
        if($id === 'a' || $id === 'c')
            echo $id;   
    }
}

Element 'c' gets unset within the foreach(groups ..) loop but is afterwards again outputted in the else branch. Also when i var_dump($fields) at the beginning i always get 'a', 'b' and 'c' inside. I'm using PHP 5.4.7.

Thanks in advance

EDIT: i made a mistake in the sample code, its now updated. All comments about using the wrong index (it would have been 0,1 etc) were correct of course. The values when using var_dump are unset now, but i still get into the else with 'c' one time.

EDIT2: Im not done with the original code but after reading through the comments I currently came up with following solution to the posted code snippet above:

$ids=array("a"=>array(),"b"=>array(),"c"=>array(),"d"=>array(),"e"=>array());
$groups=array(array("a"),array("c", "e"));
array_walk($groups,function($v,$i)use(&$ids){

    $in_both = array_intersect(array_keys($ids),$v);
    //var_dump($in_both);
    foreach($in_both as $b) {
        unset($ids[$b]);
    }
});
print_r($ids);

or

$ids=array("a"=>array(),"b"=>array(),"c"=>array(),"d"=>array(),"e"=>array());
$groups=array(array("a"),array("c"));
array_walk($ids,function($v,$i)use(&$ids, $groups){
    $in_both = array();
    foreach($groups as $g) {
        if(in_array($i,$g)) {
            $in_both = array_intersect(array_keys($ids),$g);
        }
    }

    foreach($in_both as $b) {
        unset($ids[$b]);
    }
});
print_r($ids);

Using a foreach does not work for me in this case, because i need to change the $ids array while the loop is iterating over it.

In the very most basic situation a code something like this:

$ids = array('a', 'b');

while(count($ids)) {
    array_pop($ids);
    echo 'pop';
}

echo 'empty';

Allthough foreach can change the original values from the array it will not change the copy of the array used for the iteration as nl-x already stated. Thanks to Passerby for the idea of using array_walk for this.

EDIT3: Updated code snipped once more. The second snipped allthough behaves undefined as well. Deleting elements from an array while iterating over its seems to be a bad idea.

Chris
  • 1,226
  • 1
  • 12
  • 26
  • Look [here](http://stackoverflow.com/questions/10057671/how-foreach-actually-works). I suggest to use `for` loop instead. – BlitZ Apr 18 '13 at 08:08
  • 3
    `$ids[$field_id]` in your example doesn't exists, because it will interprets to `$ids["a"]`, not `$ids[0]`. Also, I have difficulty following your question, would you mind to make it a little bit more clear, like "I have two arrays that looks like this [array code], and I would like to delete if...so it can later look like this [array code]."? – Passerby Apr 18 '13 at 08:22
  • @Passerby Excellent observation +1. But that error does not change anything in the output. If you change `$ids = array('a', 'b', 'c');` into `$ids = array('a'=>'a', 'b'=>'b', 'c'=>'c');`, nothing changes. – nl-x Apr 18 '13 at 10:30
  • @nl-x As I've said, I don't quite follow OP's question/logic. I spot that error only because it's a little obvious. – Passerby Apr 18 '13 at 10:34
  • @nl-x: The real problem here is not changing output, but unsetting an array element. – soju Apr 18 '13 at 10:46
  • updated post, made a mistake in my sample. In my actual code I use something like $ids = array('a' => array(), 'b' => array(), 'c' => array()); see updated post – Chris Apr 18 '13 at 11:17
  • I already made a comment to your post, telling this looks like the answer. I accepted your answer already and may report back a final time when I fixed everything in the real code. Thank you so far :) – Chris Apr 18 '13 at 11:25
  • Actually this sentence helped me the most: (Clearing a and c from ids will have no impact on the foreach($ids as $id) as foreach will continue with the untouched copy even after ids array has been cleared.) Your explanation is completly right about my code sample above. But the actual problem was that is forgot the foreach loop uses a copy for the loop itself and unsetting values "on the fly" within the loop does not work. Because if c was unset in the first loop why should never reach c again, that brought my initial confusion. – Chris Apr 18 '13 at 14:37

6 Answers6

1

$ids[$field_id] does not exist, you are using the value instead of the key.

You should simply unset using the right key :

if (in_array($field_id, $ids))
    unset($ids[array_search($field_id, $ids)]);
soju
  • 25,111
  • 3
  • 68
  • 70
  • This doesn't change the output, as I replied to Passerby as well – nl-x Apr 18 '13 at 10:32
  • This has been tested, and it works... This will not change your output since you simply echo `$field_id`... You should try `print_r($ids);` at the end of your script. – soju Apr 18 '13 at 10:36
  • Thanks for the answer, i updated my post, i had an error in my example code, this was not the problem, but you got an upvote of course :) – Chris Apr 18 '13 at 11:16
1

I'm double checking now. But I think unsetting an array with foreach is not really safe.

What I usually would do is take a foreach, and start with the highest indexes and descrease the index along the way. for($i = count($arr)-1; $i >= 0; $i--) { unset($array[$i]); }

I'll edit this post in a few minutes.

edit: i was confused. The for with $i++ is indeed the culprit. foreach is safe (in php! not in all languages)

<?php

$arr = Array(1,2,3,4,5,6,7,8,9,10);
foreach ($arr as $key=>$val)
    unset($arr[$key]);
echo implode(',',$arr); // returns nothing


$arr = Array(1,2,3,4,5,6,7,8,9,10);
for ($i=0; $i<count($arr); $i++)
    unset($arr[$i]);
echo implode(',',$arr); // returns 6,7,8,9,10


$arr = Array(1,2,3,4,5,6,7,8,9,10);
for ($i=count($arr)-1; $i>=0; $i--)
    unset($arr[$i]);
echo implode(',',$arr); // returns nothing

?>
nl-x
  • 11,762
  • 7
  • 33
  • 61
1

Spent some time reading your code, and I guess your procedure is:

  • For every element in $ids, check if it exists in some sub-array in $groups;
  • If it exists, delete everything in $ids that also exists in this sub-array.

Following the above logic, I come up with this:

$ids=array("a","b","c","d","e");
$groups=array(array("a","c"),array("c","e"));
array_walk($groups,function($v,$i)use(&$ids){
    $ids=array_diff($ids,$v);
});
print_r($ids);//debug

Live demo

Passerby
  • 9,715
  • 2
  • 33
  • 50
1

Chris, if I understand correctly, you don't expect 'C' to be outputted in the else branch?

But it should be outputted. Your logic is:

  • you do foreach ids and start with id 'a'.
  • then you clear ids a and c from ids and delete the group g1 that contained 'a'. During this step the deleted ids will be outputted, being a and c. (Clearing a and c from ids will have no impact on the foreach($ids as $id) as foreach will continue with the untouched copy even after ids array has been cleared.)
  • then you do id 'b': it is not found in any group. (actually, there isn't any group left by now anyway)
  • so for 'b' you enter the else branch. But the if() inside the else branch prevents output
  • then you do id 'c', which is also not found in any group, because you have already deleted group g1! There are no groups left, remember?
  • so for 'c' you also enter the else branch. And this time the if() inside the else branch allows the output! The output being just c

So the total output is indeed acc.

It is good to know that a foreach() that continues with a untouched copy even after its elements were cleared, is a specific PHP thing. Other language do no necessarily do the same.

nl-x
  • 11,762
  • 7
  • 33
  • 61
  • This answer is probably exactly my problem. The actual code is quite a bit more complicated than my sample but i will definately have a look at that and report back. – Chris Apr 18 '13 at 11:21
0

If you want to remove the element from the array, shouldn't you 'splice' it out instead with array_splice?

From the PHP manual: http://php.net/manual/en/function.array-splice.php

Arnelle Balane
  • 5,437
  • 1
  • 26
  • 32
0

Your foreach wont make any changes since a copy of array is used.. you will need to use pass by reference in order for this to work. one of the way is mentioned below

  while(list($key,$value) = each($array)){
   if(your reason to unset)
       unset($array[$key]);
}

this will remove the element from the array.

Dinesh
  • 3,065
  • 1
  • 18
  • 19