Problem/Motivation

Sometimes lots of the following error is raised in function menu_unserialize():

warning: Invalid argument supplied for foreach() in .../includes/menu.inc.

This is caused by ill-programmed contrib/custom modules, that do not store the argument list in array-format (of course, mostly when there are 0 or 1 arguments).

Proposed resolution

The proposed patches in the issue queue all are trying to detect the error in on the page load.
We shouldn't do this, and a error detaction needs to be done on database-insertion.

As the originator.. I somewhat agree with "not babysitting broken code," however there is that long standing strategy of defensive coding (Kernighan & Plauger) to think about. And as will_in_wi said (http://drupal.org/node/277018#comment-1521118) it's valuable to a module developer help them work out where problems are, rather than leave them mystified.

The strategy of doing the check at the database insertion sounds far far far far better ...

Remaining tasks

Locate where the error is generated, and write a patch for that.

User interface changes

None.

API changes

None.

(A list of related issues.)

Original report by [username]

(Text of the original report, for legacy issues whose initial post was not the issue summary. Use rarely.)I have a partially working module and when the module is active the following message is printed multiple times

warning: Invalid argument supplied for foreach() in /home/.nurse/reikiman/davidherron.com/includes/menu.inc on line 258.

Clearly the module has done something to cause this message to exist.. but ignore my module for the moment because I want to point to an issue with the above function.

At line 258 in menu.inc is the function menu_unserialize. It goes

  if ($data = unserialize($data)) {
    foreach ($data as $k => $v) { // line 258
      ...
    }
  }

According to the unserialize documentation -- http://us3.php.net/manual/en/function.unserialize.php -- the function can succeed and return something other than an array. If the function fails FALSE is returned, and menu_unserialize would skip over the whole thing. Clearly unserialize is returning something other than FALSE and the documentation says "can be a boolean, integer, float, string, array or object".

Hmm... http://drupal.org/node/197056#comment-653073 ... that note may be applicable in my situation. In any case since unserialize can legitimately return something other than an array it would be good for menu_unserialize to be more robust about the data it receives.

If I change this to

  if ($data = unserialize($data)) {
    if (is_array($data)) {
    foreach ($data as $k => $v) { // line 258
      ...
    }
    } else {
       watchdog('menu', 'menu_unserialize problem %data', array('%data'=>$data), WATCHDOG_ERROR);
    }
  }

In this case the unsightly error isn't printed on the page but the watchdog entries don't tell me anything useful.

Files: 
CommentFileSizeAuthor
#3 menu-unserialize-error-clarify.patch665 byteswill_in_wi
Passed: 10931 passes, 0 fails, 0 exceptions View

Comments

reikiman’s picture

Title: menu_unserialize can throw unsightly messages if there's an unserialization problem » menu_unserialize can throw unsightly messages if unserialize returns non-array value

Actually... the comment here http://drupal.org/node/197056#comment-653073 is very applicable to my situation.

However.. a better error could be printed if the unserialize in menu_unserialize returns a non-array.

Such as:-

function menu_unserialize($data_s, $map) {
  if ($data = unserialize($data_s)) {
    if (is_array($data)) {
    foreach ($data as $k => $v) {
      if (is_int($v)) {
        $data[$k] = isset($map[$v]) ? $map[$v] : '';
      }
    }
    } else {
      watchdog('menu', 'unserialize returned non-array for %data', array('%data' => $data_s), WATCHDOG_ERROR);
    }
    return $data;
  }
  else {
    return array();
  }
}
pwolanin’s picture

Version: 6.2 » 7.x-dev

bugs need to be fixed in 7.x first

will_in_wi’s picture

Status: Active » Needs review
FileSize
665 bytes
Passed: 10931 passes, 0 fails, 0 exceptions View

Simple roll of the patch above.

cburschka’s picture

I may have misunderstood this issue, but I don't think we need to handle incorrect data returned by module code. The hook system (which hook_menu() is a part of) works by a "contract" interface, ie. the implicit assumption that the data returned by the hook implementation will be valid. There's no way for menu_unserialize to unpack a non-array value as long as all hooks are implemented correctly.

will_in_wi’s picture

Not necessarily arguing for the patch, but I think that the point is to make it easier to develop modules. I know that I have implemented hooks wrong and then couldn't figure out where the message was coming from. This would change current behaviour to fail more gracefully when a new (or experienced at 4am) dev writes a bad hook.

The downside of this is if this type of code is implemented all over drupal, it could increase the size and complexity of the code significantly. It also might be a performance hit, although I am not sure what the performance of an is_array call is.

A better way to do this IMHO would be to change the api to send an unserialized version of $data to the function and then use type hinting. This, of course, would only work in D7.

pwolanin’s picture

Status: Needs review » Needs work

In fact, I think if we care about preventing this error, we should be checking when we do a menu rebuild (i.e. just once before the data is serialized) , NOT when the data is being unserialized. In general I'd agree with Arancaytar that we cannot babysit code to this extent. So this is pretty close to a 'won't fix'

webchick’s picture

Peter and I discussed this in IRC. Core has a policy of "not babysitting broken code," so I'm not too hot on committing patches like this. However, the fact that it spews errors everywhere points that the system is not being quite as fail-safe as it could be.

Peter suggested and I agree with moving the validation to when the rows are inserted into the database, rather than when they are read, since the former happens once in awhile and the latter happens on every page load. Let's take a look at what a patch like that might look like.

reikiman’s picture

As the originator.. I somewhat agree with "not babysitting broken code," however there is that long standing strategy of defensive coding (Kernighan & Plauger) to think about. And as will_in_wi said (http://drupal.org/node/277018#comment-1521118) it's valuable to a module developer help them work out where problems are, rather than leave them mystified.

The strategy of doing the check at the database insertion sounds far far far far better ...

pillarsdotnet’s picture

Version: 8.x-dev » 7.x-dev

Wrote the following short test program to find problems in my database:

$dev = new mysqli('localhost', 'user', 'pass', 'database');
$res = $dev->query('
SELECT path, access_arguments, page_arguments, title_arguments FROM menu_router
');
while ($row = $res->fetch_assoc()) {
  foreach (array('access_arguments', 'page_arguments', 'title_arguments') as $field) {
    $path = $row['path'];
    $raw = $row[$field];
    $data = @unserialize($raw);
    if ($data && !is_array($data)) {
      $string = var_export($data, TRUE);
      print "$path $field unserialize('$raw') === $string\n";
    }
  }
}

Perhaps something similar could be written as a menu_requirements() function?

pillarsdotnet’s picture

Version: 7.x-dev » 8.x-dev

bumping version.

johnv’s picture

Title: menu_unserialize can throw unsightly messages if unserialize returns non-array value » Warning: Invalid argument supplied for foreach() in menu_unserialize() if unserialize returns non-array value
Version: 7.x-dev » 8.x-dev

Marked #1914974: Invalid argument supplied for foreach() in menu_unserialize() (line 400 of includes/menu.inc). as a duplicate, which has a different solution to the problem:

function menu_unserialize($data, $map) {
  if ($data = unserialize($data)) {
+    if ((!is_array($data)) && (!is_object($data))) {
+      return $data;
+    }
    foreach ($data as $k => $v) {
      if (is_int($v)) {
        $data[$k] = isset($map[$v]) ? $map[$v] : '';
johnv’s picture

Status: Needs work » Active

Updated issue summary.
As discussed in #4-#8: the proposed patches may be helpful for individual programmers to locate an error, but non of them will be committed in core, since the solution must be found in another location.
Hence, setting this back tot 'Active'.

johnv’s picture

Issue summary: View changes

Updated issue summary.

illmatix’s picture

Is the patch in #3 doesn't end up logging a proper response and $data_s isn't defined.

arielberg’s picture

i didn't follow the post but i got the same error and turns out it was just that i gave 'access arguments' a string instead of an array in my hook_menu function...

   'access arguments' => 'my permision',  

to

   'access arguments' => array('my permision'),  
pwolanin’s picture

So at this point, I think this issue should probably be closed - I don't see how this is anything other than accommodating broken code.

I we do want to address it (and given the routing changes in D8) I would consider either that values expected to be arrays are cast that way before being serialized OR an exception should be thrown if a scalar is supplied when an array is expected.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.