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?
Comment | File | Size | Author |
---|---|---|---|
#21 | system.admin_menu_block-d7-1.patch | 1.2 KB | brad.bulger |
#21 | system.admin_menu_block-d6-1.patch | 1.32 KB | brad.bulger |
#19 | system.admin_menu_block-d6.patch | 1.32 KB | brad.bulger |
#19 | system.admin_menu_block-d7.patch | 1.19 KB | brad.bulger |
#1 | system.mblock.patch | 843 bytes | brad.bulger |
Comments
Comment #1
brad.bulger CreditAttribution: brad.bulger commentedi also changed the query slightly to use the table alias ml for both columns - likely making no real difference, just for consistency
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedIf 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.
Comment #3
brad.bulger CreditAttribution: brad.bulger commentedI 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.
Comment #4
mgenereu CreditAttribution: mgenereu commentedI 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.
Comment #5
jdwfly CreditAttribution: jdwfly commentedI ran into this problem after installing Simplenews. All I did to fix the problem was clear the cache and it was working again.
Comment #6
kenorb CreditAttribution: kenorb commentedSome 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.
Comment #7
Heine CreditAttribution: Heine commentedThen 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?
Comment #8
kenorb CreditAttribution: kenorb commentedFor 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?
Comment #9
Heine CreditAttribution: Heine commentedSigh, 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.
Comment #10
kenorb CreditAttribution: kenorb commentedThe cause is different each time:
#451638: Unsupported operand types
#471964: Rules administration menu broken - Fatal error: Unsupported operand types in modules/system/system.module on line 626
#458880: admin/rules on the administration menu no longer works
#421250: Unsupported operand types in system.module on line 626
#346945: Events not appearing after upgrade to Drupal 6.8
#595724: After core upgrade: Fatal error: Unsupported operand types ... /modules/system/system.module on line 627
#630680: SMS Framework administration menu broken - Fatal error: Unsupported operand types in modules/system/system.module on line 626
#446814: Unsupported Operand Type when accessing /admin/content/simplenews
#400638: PHP Fatal error when trying to access admin/settings/ldap
http://groups.drupal.org/node/60343
and many more.
See more:
http://www.google.com/search?hl=en&q=site:drupal.org/node+"Unsupported+operand+types"+"system.module+on+line"
About 179 results
It's like fixing each time different part of the car wasting your money and time for hours on end instead of changing it, even if you can afford new one.
One line could save thousands of raised bugs in future, and tons of hours to fix them. It's forcing developer to fix it immediate, because without it website doesn't work at all.
Comment #11
kenorb CreditAttribution: kenorb commentedFunctions are designed to return different types:
In this case:
According to API:
http://api.drupal.org/api/function/db_fetch_array/6
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.
Comment #12
GuyPaddock CreditAttribution: GuyPaddock commentedI 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.
Comment #13
Heine CreditAttribution: Heine commentedI agree. However, replacing a Fatal error + msg with an empty page is not my idea of minimizing admin time for diagnosis.
Comment #14
Heine CreditAttribution: Heine commentedComment #15
GuyPaddock CreditAttribution: GuyPaddock commentedRepro-steps:
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.
Comment #16
GuyPaddock CreditAttribution: GuyPaddock commentedI 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.
Comment #17
kenorb CreditAttribution: kenorb commented#16 #149562: Menu module causes duplicate menu items
Comment #18
GuyPaddock CreditAttribution: GuyPaddock commentedUgh. 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...
Comment #19
brad.bulger CreditAttribution: brad.bulger commented#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.
Comment #20
catchThis 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.
Comment #21
brad.bulger CreditAttribution: brad.bulger commentedyeah i wondered about that. i went by what other similar watchdog calls were doing nearby, for lack of a better idea. "menu" sounds fine.
Comment #22
catchCNR 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.
Comment #24
pbucalo CreditAttribution: pbucalo commentedsubscribing
Comment #25
Damien Tournoud CreditAttribution: Damien Tournoud commentedI'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...
... needs work: this is precisely what
trigger_error()
is for.Comment #26
Mascher CreditAttribution: Mascher commented#1: system.mblock.patch queued for re-testing.
Comment #27
Mascher CreditAttribution: Mascher commented#21: system.admin_menu_block-d6-1.patch queued for re-testing.
Comment #28
Mascher CreditAttribution: Mascher commentedI 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
Comment #30
Mascher CreditAttribution: Mascher commentedneed new patch :)
Comment #31
kristin.e CreditAttribution: kristin.e commentedsubscribing - 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
Comment #32
fuscata CreditAttribution: fuscata commentedWe are still testing this, but so far this seems to have solved the problem:
Comment #33
brisath CreditAttribution: brisath commentedI'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"
Comment #34
ryank76 CreditAttribution: ryank76 commentedsubscribing - getting this WSOD messages in Ubercart 6.x-2.6
Comment #35
rajiff CreditAttribution: rajiff commentedHi
I did a change to the system.module at line 626 as below
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...