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.

CommentFileSizeAuthor
#3 location_26.patch1.53 KBnickl
location.patch1.42 KBtherealwebguy

Comments

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Active » Reviewed & tested by the community

Confirmed.

nickl’s picture

nickl commented
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. 

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

And also :

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

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

nickl’s picture

nickl commented
Status: Needs work » Needs review
StatusFileSize
new location_26.patch1.53 KB

Rolled new patch with above suggestions implemented.

therealwebguy’s picture

therealwebguy commented

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.

yesct’s picture

yesct commented
Issue tags: +location bdragon consider commit

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

hutch’s picture

hutch commented
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

yesct’s picture

yesct commented
Status: Needs review » Reviewed & tested by the community
ankur’s picture

ankur commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD, DRUPAL-6--3

yesct’s picture

yesct commented
Issue tags: -location bdragon consider commit

http://drupal.org/cvs?commit=353314
http://drupal.org/cvs?commit=353312

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.