Project:Location
Version:6.x-3.x-dev
Component:Code
Category:bug report
Priority:normal
Assigned:Unassigned
Status:closed (fixed)

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.

AttachmentSize
location.patch1.42 KB

Comments

#1

Status:active» reviewed & tested by the community

Confirmed.

#2

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

Status:needs work» needs review

Rolled new patch with above suggestions implemented.

AttachmentSize
location_26.patch 1.53 KB

#4

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

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

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

#7

Status:needs review» reviewed & tested by the community

#8

Status:reviewed & tested by the community» fixed

Committed to HEAD, DRUPAL-6--3

#9

#10

Status:fixed» closed (fixed)

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

nobody click here