Invalid foreach line 1445

Posted by nrussell on October 28, 2009 at 6:41pm

Issue Summary

In reviewing this issue, I also noticed there was another foreach in this function not properly being checked. It is important PHP practice to insure that the array you attempting to iterate through inside a foreach is first checked for data. PHP will generated a warning if the array does not exists inside a foreach statement.

Attached is a patch for these two conditions.

Attachment	Size
location.patch	1.42 KB

Comments

#1

Posted by agentrickard on November 5, 2009 at 4:04pm

Status:

active

» reviewed & tested by the community

Confirmed.

#2

Posted by nickl on November 26, 2009 at 5:45am

Status:

reviewed & tested by the community

» needs work

Should the protecting if statements not check for arrays instead of checking if the value is set positively.

<?php

if (is_array($variables['hide'])) {
  foreach($variables['hide'] as $key) {
    //snip
  }
}
?>

And also :

<?php
  if (is_array($fields)) {
    foreach ($fields as $key => $value) {
      //snip
    }
  }
?>

Because the foreach will still fail if $fields = 3; for example.

#3

Posted by nickl on December 3, 2009 at 5:45pm

Status:

needs work

» needs review

Rolled new patch with above suggestions implemented.

Attachment	Size
location_26.patch	1.53 KB

#4

Posted by nrussell on December 8, 2009 at 6:55pm

Yes this is correct nicki, if a value is passed that is not an array, the if will return true in my condition still leaving the foreach to generate a warning. Thanks for the update and drilling in the specifics.

#5

Posted by YesCT on April 11, 2010 at 7:53am

Someone please confirm that this applies to the current dev (to see if it needs a re-roll) and does not break anything.
This might be a nice patch to test for someone who has never reviewed a patch before:
http://drupal.org/patch/review

#6

Posted by hutch on April 11, 2010 at 11:52am

Version:

6.x-3.1-rc1

» 6.x-3.x-dev

location_26.patch applies cleanly and testing with is_array before iteration makes good sense to me.
tested on latest dev, although I did not attempt to force an error, not sure how and don't think it's necessary.

I would recommend this patch