menu_unserialize can throw unsightly messages if unserialize returns non-array value
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | menu system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
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.
#1
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();
}
}
#2
bugs need to be fixed in 7.x first
#3
Simple roll of the patch above.
#4
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.
#5
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.
#6
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'
#7
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.
#8
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 ...