This function in the system module, system_admin_menu_block(), does a query against the menu_links table looking for an entry for the given path from the system module. That query is not guaranteed to produce results - for whatever reason - yes, probably because of a bug somewhere else, it does not matter why. But it is being added with the '+' operator to the $item argument to the function. When the result is not an array, this results in a fatal error.

Sure seems like this is something that ought to be added to the coding standards. That use of the '+' operator with arrays is responsible for a number of bugs like this. It's one thing to be optimistic, but accepting a full stop error as a consequence is a bit much. Is it so much more efficient than checking that the value is an array, or that at least it's not empty?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

brad.bulger’s picture

FileSize
843 bytes

i also changed the query slightly to use the table alias ml for both columns - likely making no real difference, just for consistency

Damien Tournoud’s picture

Version: 6.14 » 7.x-dev

If the mlid cannot be determined, this function *has to fail* one way or the other (the mlid is used just a few lines below). Failing with a fatal error is not such a bad way to fail.

Anyway, moving to D7 has this has to be considered in the current development version before being (eventually) backported.

brad.bulger’s picture

I see your point. It's something of a philosophical issue, I suppose, about what kind of errors are preferable. I have not looked at this in D7 so it may be moot there in any case.

mgenereu’s picture

I just upgraded from 6.6 to 6.16 and my "User Management" link caused this error. The items underneath worked just fine. I restored a back up database and reverted to 6.6 and confirmed it was working fine. Through this whole process I was constantly using "clear cache" without error messages. Further investigation revealed that while the user.module defines "admin/user", the menu_links table did not have the entry. The final solution was to wipe out the menu_links table and then "clear cache" again. Everything worked great after that. I still don't know what during the upgrade damaged the "admin/user" entry and only that entry but I was figuring "clear cache" would have fixed it.

--- Edit ---

Bah... that proved to be a bad solution... still looking for how to fix this...

--- Nother Edit ---

"DELETE FROM menu_links WHERE module = 'system'" followed by a "clear cache" seems to have cleared up the issue.

jdwfly’s picture

I ran into this problem after installing Simplenews. All I did to fix the problem was clear the cache and it was working again.

kenorb’s picture

Some discussion about this bug here:
#595724: After core upgrade: Fatal error: Unsupported operand types ... /modules/system/system.module on line 627

+1 for patch from #1
If people could vote, I'm sure that more than 90% will say it's a critical bug which should be fixed.
Why waste hours or days to workaround, if this could be fixed in very simple way in the source of the problem.
Instead showing WSOD, we can just report that to watchdog about some invalid menu item (elseif). Website can work without it and it's not something that have to crash the whole website because of some small part.
Sometimes you even don't realize that you have this error message on 1 of the thousands pages.
Otherwise we will have to create some special forum for helping people how to fix that kind of bugs and how to workaround this.

Heine’s picture

Instead showing WSOD, we can just report that to watchdog about some invalid menu item (elseif). Website can work without it and it's not something that have to crash the whole website because of some small part.

Then why +1 on #1? As per #2, we need to fail somewhere, and the patch in #1 does not fail with an error message.

Anyone care to repro and fix the "origin bug" ? This feels a bit like taking asperin when having a tumour. Maybe in a separate issue?

kenorb’s picture

For me it's a bad logic of thinking.
It's like side-walk on the express-way. Theoretically cars must drive on the street. But mistakes happen and practically they kill thousands of people (websites) every year. Solution is not to clean up every time smashed bodies, because drivers shouldn't do that and others should learn from it something. Solution is to prevent that happening by prohibiting side-walks on the express-way or to create some kind of wall to protect them from mistakes of others.
If it's possible to do some simple condition, by not killing our website each time when somebody do some small mistake or because our table is corrupted, why to not improve it making life easier?

Heine’s picture

Sigh, let me spell it out.

I'm in favor of failing with a polite error message, rather than crashing the site if we can prevent it. The patch in #1 doesn't fail predictably.

That doesn't absolve us from finding the cause of this error and fix that; maybe in a separate issue? Also, the cause of the bug also helps us to determine whether it's worthwhile or desirable to 'fix' _this_ issue with a message.

kenorb’s picture

Functions are designed to return different types:
In this case:

$item += db_fetch_array(db_query("SELECT mlid, menu_name FROM {menu_links} ml WHERE ml.router_path = '%s' AND module = 'system'", $item['path']));

According to API:
http://api.drupal.org/api/function/db_fetch_array/6

Return value
An associative array representing the next row of the result, or FALSE. The keys of this object are the names of the table fields selected by the query, and the values are the field values for this result row.

function db_fetch_array($result) {
  if ($result) {
    return pg_fetch_assoc($result);
  }
}

It's clear that db_fetch_array() can return "next row of the result, or FALSE". If the code doesn't work properly in the second case and it's crashing (even that kind of return is correct and it's clearly specified in API), for me code is bad written.

GuyPaddock’s picture

I would also tend to agree that crashing with a fatal error is not a good solution. It gives the admin very little chance to know what's going on, let alone to do something about it. At least if we log to watchdog, or display a polite error, we aid in the diagnosis of the real issue that caused the missing entries.

I think a "design-by-contract" perspective is helpful here. As kenorb says in #11, basically the "contract" of db_fetch_array() says that FALSE is an acceptable return, so it is system_admin_menu_block() that is not properly honoring the contract by depending on an array to be returned. Regardless of the actual underlying issue that causes the corrupt data, we do a disservice to users / admins when we don't at least do something to minimize the time it takes them to diagnose the actual issue. The logic should be looking out for errors, not depending on them not to happen.

Heine’s picture

I agree. However, replacing a Fatal error + msg with an empty page is not my idea of minimizing admin time for diagnosis.

Heine’s picture

Status: Active » Needs work
GuyPaddock’s picture

Priority: Normal » Critical
Status: Active » Needs work

Repro-steps:

  1. Install any contributed module that creates menu items. For example, in my case I installed the UberCart "Flatrate" fulfillment module, which creates the menu path "admin/store/settings/quotes/methods/flatrate".
  2. Customize the menu items that the module creates. For example, in my case, I moved all of the UberCart menu items that are normally under "Store administration" (admin/store) into a new menu called "Store administration" so that I could put the links into their own block.
  3. Disable and uninstall the contributed module installed in step 1.

From this point on, the menu_links table is corrupt. Any attempt to access parent menu items of the menu item that corresponded to the uninstalled module will result in the "Unsupported operand types" message. For example, after the uninstall, I can no longer access "admin/store" nor "admin/store/reports".

The only fix is to truncate the menu tables and rebuild the cache, then customize the menus all over again.

GuyPaddock’s picture

I should also note that if you attempt to "reset" the affected menu items to their defaults, Drupal will clone the parent menu items of the affected entries multiple times in the menu hierarchy. For me, each time I reset a customized menu item after the table became corrupt, I ended up with another copy of the "Administration" menu item. It looks like there is no way to recover from the corruption except to truncate the affected tables.

kenorb’s picture

GuyPaddock’s picture

Ugh. Scratch my repro-steps in #16. I can't seem to get them to work a second time. It's definitely related to customizing menu items, though...

brad.bulger’s picture

#13: i've been converted to that point of view by the discussion here - the patch does not really deal with what it means if that query fails, and how to deal with it. which i am not entirely sure i know, but it can at least display an error and bail out of the function.

the code for this function in 7.x is not much different from 6.x.

catch’s picture

This shouldn't use watchdog('php' - that should be reserved for actual PHP notices or warnings but we're eating that error here. Probably watchdog('menu') would be fine.

brad.bulger’s picture

yeah i wondered about that. i went by what other similar watchdog calls were doing nearby, for lack of a better idea. "menu" sounds fine.

catch’s picture

Status: Needs work » Needs review

CNR for the test bot.

Two things I think we need here:

1. Step by step guide to reproduce in Drupal 7 with no contrib modules, looks like GuyPaddock's instructions from #15 could be adapted with locale or shortcuts module. That would allow a test case to be written.

2. A followup patch to address the root cause of the invalid data, we need to figure out whether it's actually the menu_links table that gets corrupted, or whether this code just makes bad assumptions about it. I have a feeling that issue is going to open a can of worms, so agree with a quick fix here, but it'd be nice to be able to reference it from this issue.

Status: Needs review » Needs work

The last submitted patch, system.admin_menu_block-d6-1.patch, failed testing.

pbucalo’s picture

subscribing

Damien Tournoud’s picture

Priority: Critical » Normal

I'm not sure what the whole debate #3 -> #13 was all about, because everyone is saying the same thing. This *has to fail*, hiding the error under the carpet is unacceptable.

That said...

+      watchdog('menu', 'No system menu_links record found for router_path "%path"', array('%path' => $item['path']), WATCHDOG_ERROR);
+      drupal_set_message(t('No system menu_links record found for router_path "%path"', array('%path' => $item['path'])), 'error');

... needs work: this is precisely what trigger_error() is for.

Mascher’s picture

Status: Needs work » Needs review

#1: system.mblock.patch queued for re-testing.

Mascher’s picture

#21: system.admin_menu_block-d6-1.patch queued for re-testing.

Mascher’s picture

I have some error with /admin/rules

Fatal error: Unsupported operand types in /var/www/fantasy-portal.ru/htdocs/modules/system/system.module on line 627

Rules 6.x-1.2
Drupal core 6.19

Status: Needs review » Needs work

The last submitted patch, system.admin_menu_block-d6-1.patch, failed testing.

Mascher’s picture

need new patch :)

kristin.e’s picture

subscribing - Store Admin page has WSOD with error message
home/username/public_html/modules/system/system.module on line 627 after core upgrade to 6.20

fuscata’s picture

We are still testing this, but so far this seems to have solved the problem:

mysql> SELECT `module` FROM `menu_links` WHERE `router_path` = 'admin/store/settings';
+------------+
| module     |
+------------+
| admin_menu |
+------------+
1 row in set (0.00 sec)

mysql> UPDATE `menu_links` SET `module` = 'system' WHERE `router_path` = 'admin/store/settings';
Query OK, 1 row affected (0.00 sec)
Rows matched: 1  Changed: 1  Warnings: 0
brisath’s picture

I'm getting WSOD messages also in 6.x so would this be the same issue. The message reads "Unsupported operand types in /home/mysite/public_html/sites/all/modules/ubercart/uc_store/includes/uc_price.inc on line 85"

ryank76’s picture

subscribing - getting this WSOD messages in Ubercart 6.x-2.6

rajiff’s picture

Version: 7.x-dev » 6.14

Hi

I did a change to the system.module at line 626 as below

    $data = db_fetch_array(db_query("SELECT mlid, menu_name FROM {menu_links} ml WHERE ml.router_path = '%s' AND module = 'system'", $item['path']));
    if($data != null || $data != '') $item += $data;

This will at least show a graceful page and not the fatal error

Regarding actual error, my menu links are not getting built properly for some reason, links are all there but there is a mismatch between routerpath in menu_router and menu_link, don't know why, which is actually the root cause of the issue.

any one found any thing, please post here...

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.