Problem/Motivation
Menu rebuild causes:
Object of class stdClass could not be converted to int in _menu_router_build() - line 3370
error when using some contrib modules such as nodequeue.
This is due to a bug in the behavior of array_multisort()
, which is used in _menu_navigation_links_rebuild(). array_multisort()
keeps sorting by all dimensions of the array, and if any of the (sub)-children are objects it will trigger a notice that it cannot compare them to other types.
Here is a generic PHP script that will trigger this error:
$sort['A'] = 2;
$sort['B'] = 1;
$sort['C'] = 1;
$menu['A'] = 0;
$menu['B'] = (object) array(0);
$menu['C'] = 0;
array_multisort($sort, SORT_NUMERIC, $menu);
(See also the PHP bug report.)
Proposed resolution
Fix the sorting of menu items to not use array_multisort()
but just a normal asort()
. A patch implementing this change is in #972536-63: Object of class stdClass could not be converted to int in _menu_router_build() - line 3370. A test that reproduces the bug is included in the patch.
A workaround for modules that trigger this error is to change the order of arguments in hook_menu()
declarations. See for example this commit to fix this bug in the nodequeue module, and #2568829: In hook_menu(), put callback arguments near the end of the item list to avoid array_multisort() quirk.
Remaining tasks
- Adjust patch for changes from #1010480: Optimize _menu_navigation_links_rebuild() in
_menu_navigation_links_rebuild()
. - Evaluate the impact this change could have on performance, as
array_multisort()
may have been chosen for performance reasons. - For the related issue of the side effects of passing cached objects to the menu system, see #997918: Menu gets rebuilt continually during install and update (and menu system stores entire, out-of-date theme objects).
User interface changes
None.
API changes
None.
Original report by tekante
When doing a minimal install with Drupal 7 Beta2, the above error is observed after enabling the nodequeue module when using the head revision and patches http://drupal.org/files/issues/nodequeue-d7-db-insert-errors-fix.patch and http://drupal.org/files/issues/nodequeue-941558-16.patch.
Why am I not filing this in the nodequeue queue? I don't believe the error is due to anything nodequeue is doing wrong based on the variety of things that resolve the error. Comparing its info to that of another item (admin/structure/types does not show any obvious errors). There is an issue filed related to this but I wanted to bring it to the attention of the menu system maintainer [959904]
Attached is a file containing a serialized dump of the $sort and $menu states along with comments about various manipulations to the data that result in the error disappearing.
The error disappears in the following conditions:
1. Unsetting arbitrary admin/structure/nodequeue menu entry sub keys
2. Adding a SORT_STRING parameter as the 4th argument to the array_multisort call on line 3370
3. Unsetting an arbitrary subkey of another menu entry (admin/structure/types in the attached file)
My PHP versions are
PHP 5.2.13 (cli) (built: Mar 7 2010 09:15:25)
and
PHP 5.3.2 (cli) (built: Aug 7 2010 00:04:41)
Comment | File | Size | Author |
---|---|---|---|
#95 | 972536-95-core-7.x-error-suppression-array_multisort-workaround.patch | 1.59 KB | alberto56 |
#91 | drupal-menu-int-972536-91.patch | 3.46 KB | kyletaylored |
#86 | drupal-menu-int-972536-86.patch | 1.37 KB | roma7 |
#83 | drupal-menu-int-972536-83-D7.patch | 3.06 KB | jamsilver |
#82 | drupal-menu-int-972536-81.d7.patch | 3.28 KB | jamsilver |
Comments
Comment #1
tekante CreditAttribution: tekante commentedPatch attached adds the SORT_STRING parameter which causes the error to stop occurring for me. Note that this may just be covering up the true cause of the error.
Comment #2
comicsal CreditAttribution: comicsal commentedHopefully, to help narrow down the problem, I noticed that the error only occurred when the 'Smartqueue taxonomy' and 'Nodequeue generate' Module-elements were enabled. As soon as I disabled them the problem disappeared.
I was adding a new Content Type when I originally noticed the error.
Comment #3
WhiplashInfo CreditAttribution: WhiplashInfo commentedI have 7 RC1 installed and when enabling entity and rules modules the Notice: Object of class stdClass could not be converted to int in _menu_router_build() (line 3443 of /home/web25518/domains/fmedk.se/public_html/d7/includes/menu.inc occured.
Se my attached file.
Thanks / Tomas
Comment #4
berenddeboer CreditAttribution: berenddeboer commentedPatch works for me...
Comment #5
webchickHm. I'd like us to have a test that exposes exactly what makes this buggy. That might also help us get to the bottom of the real bug.
Comment #6
dobrzyns CreditAttribution: dobrzyns commentedI often get this when enabling/disabling modules in 7.0-rc3. So, it appears not to be just a nodeque generate or smartque issue.
Exact error: Notice: Object of class stdClass could not be converted to int in _menu_router_build() (line 3458 of /includes/menu.inc).
Comment #7
spacereactor CreditAttribution: spacereactor commentedi having this problem with Nodequeue too. using drupal 7.0 and Nodequeue 7.x-2.x-dev.
Comment #8
Dave ReidMarked #1026140: Receiving Error Notice after updating Token and Pathauto to 7.x-1.0-beta1 as a duplicate of this issue.
Comment #9
bfroehle CreditAttribution: bfroehle commentedThe only thing thats of type stdClass in what we are sorting is the
'access arguments'
field of$menu['admin/appearance/settings/THEME']
, which comes from$theme
here is documented to be an object (and not an array), so that's proper.Ideally PHP would have a SORT_NONE flag so we could instead do
to indicate that we don't care about further sorting by
$menu
.Comment #10
bfroehle CreditAttribution: bfroehle commentedHere goes nothing...
Edit: There's another similar array_multisort in _menu_navigation_links_rebuild().
Comment #11
Anticosti CreditAttribution: Anticosti commented#10 Patch works perfect for me.
Thanks!
Comment #12
yuriy.babenko CreditAttribution: yuriy.babenko commentedSubscribing (patch in #10 helped - thanks!)
Comment #13
js CreditAttribution: js commentedsubscribing, and thanks!
Comment #14
bfroehle CreditAttribution: bfroehle commentedIf somebody is looking to write a test... the way to trigger this is to create a test module, implement hook_menu() and have some of the items return 'access arguments' or 'page arguments' that are of type stdClass. For example:
and see if you can get an error to trigger by just going to any page on the site.
Comment #15
David_Rothstein CreditAttribution: David_Rothstein commentedThere's a report at http://drupal.org/node/1029182#comment-4019108 (an issue which I marked duplicate of this one) that this could have something to do with disabling a module that has a customized menu item.
Still waiting on an exact set of steps to reproduce (I couldn't reproduce it myself), and it could turn out to be a red herring. But it sounds like in this issue there still isn't an understanding of the ultimate root cause of the bug, so maybe that will help.
Comment #16
bfroehle CreditAttribution: bfroehle commentedDavid Rothstein: The root cause is documented in #972536-9: Object of class stdClass could not be converted to int in _menu_router_build() - line 3370. Essentially array_multisort() keeps sorting by all dimensions of the array, and if any of the (sub)-children are objects it will complain that it cannot compare them to other types.
Exact steps to reproduce are difficult, as you have to guarantee a comparison several dimensions into
$menu
, however the serialized version of the variables provided in the original post do give a good indication of what is going on.Comment #17
bfroehle CreditAttribution: bfroehle commentedAnother patch which would fix the problem for the original poster (but not the inherent flaw in the sorting system) is contained in #997918-5: Menu gets rebuilt continually during install and update (and menu system stores entire, out-of-date theme objects) --- namely the changes to system.module and block.module which replace 'access arguments' with the key name instead of the whole $theme.
Comment #18
femrich CreditAttribution: femrich commentedI have also been experiencing this. Since I don't really understand the solutions provided, except that they seem to offer only cosmetic solutions, and since I am not yet using nodequeue, my stopgap solution was to disable nodequeue. This seems to have stopped the error messages. But I am looking forward to a real solution...
Comment #19
blischalk CreditAttribution: blischalk commentedSubscribing
Comment #20
rbahaguejr CreditAttribution: rbahaguejr commentedI'm still getting this error with nodequeue.
Comment #21
Steven Jones CreditAttribution: Steven Jones commentedI think that webchick is saying in #5 that this needs tests, so marking as needing work.
Comment #22
Steven Jones CreditAttribution: Steven Jones commentedComment #23
Steven Jones CreditAttribution: Steven Jones commentedOkay, so what appears to be going on here is that as the menu is sorted with
array_multisort()
, if there any ties in the first array, then the second array is used for sorting. The second array is basically an array of menu items (arrays). This causes issues because apparently PHP will sort arrays of arrays by descending into the 'first child' of the elements to compare and then sorting again. Normally this is fine, because the keys that we have in the menu items are mostly strings and arrays, however you can pop objects in there too. PHP does not like sorting objects!Now, normally the sorting doesn't go anywhere near the 'access arguments' property of the menu item arrays, because it looks in 'first child's first when sorting, but, nodequeue puts the access arguments property first in one of its menu items. This menu item gets sorted with some of the appearance menu items, which have objects in their access arguments. Boom!
Is this an edge case? This is the definition of edge case.
Attached is a test that adds some menu items that cause the PHP notice if Drupal is sorting incorrectly. Giving it to test bot to munch on.
Comment #24
Steven Jones CreditAttribution: Steven Jones commentedSo, this patch now takes the work from #10, and adds the test from #23 and then fixes
_menu_navigation_links_rebuild()
too. If this works, then it'll be a small miracle.EDIT: If you're reading this issue then you can skip on past the testbot madness right to comment #35.
Comment #25
rfayComment #27
Steven Jones CreditAttribution: Steven Jones commented#23: 972536-23.patch queued for re-testing.
Comment #28
Steven Jones CreditAttribution: Steven Jones commented#24: 972536-24.patch queued for re-testing.
Comment #29
rfayYay, passed!
Comment #30
rfayIt definitely passed when set to 7.x. With your permission I'd like to switch it back to 7.0 to see if #1093682: 7.0 testing doesn't work (Major version testing doesn't work, just X.x testing?) is a valid bug...
Comment #31
rfay#24: 972536-24.patch queued for re-testing.
Comment #32
Steven Jones CreditAttribution: Steven Jones commented@rfay, yeah, do what you want.
Comment #33
bfroehle CreditAttribution: bfroehle commentedFYI: feature request filed with PHP for the introduction of a SORT_NONE option. http://bugs.php.net/bug.php?id=54259
(While that still would be inappropriate for our use here, it might be convenient in the future).
Comment #34
rfay#24 passes OK now with version = 7.0, although it's still testing against latest. So #1093682: 7.0 testing doesn't work (Major version testing doesn't work, just X.x testing?) was a red herring. Thanks for your support :-)
Comment #35
bfroehle CreditAttribution: bfroehle commented+1 to marking the patch in #24 as RTBC, although I'm going to refrain from doing so since patch is due to myself in #10. Stephen Jones was wonderful enough to include tests which trigger this bug in #23 and verify that the patch in #10 solves the issue. Also included is a fix for the other use of array_multisort which might also suffer from this.
I know the array_multisort usage was originally included as a way to boost performance. I haven't looked at actual performance numbers for this, but I suspect that the performance change is negligible (or even slightly faster).
For the curious, the abstract way to trigger the bug (and the corresponding usage) is:
The proposed patch instead does
Comment #36
Steven Jones CreditAttribution: Steven Jones commented#23: 972536-23.patch queued for re-testing.
Comment #37
bennash CreditAttribution: bennash commented#23: 972536-23.patch queued for re-testing.
Comment #38
Jerome F CreditAttribution: Jerome F commented#24 seems to work for me.
Comment #39
bennash CreditAttribution: bennash commented#24: 972536-24.patch queued for re-testing.
Comment #40
bdragon CreditAttribution: bdragon commentedsubscribing.
Comment #41
Agileware CreditAttribution: Agileware commentedThe patch in #24 stops the error appearing for me too.
Comment #42
Jerome F CreditAttribution: Jerome F commentedComment #43
Jerome F CreditAttribution: Jerome F commentedI still get :
Comment #44
Steven Jones CreditAttribution: Steven Jones commented@Jerome F please document what steps you took to reproduce the error.
Comment #45
Jerome F CreditAttribution: Jerome F commentedI don't know actually - I tried again, reinstalled the last dev of Drupal, ran the patch and it seems all right now. I can't reproduce either.
Comment #46
mansspams CreditAttribution: mansspams commentedsubscribing, #24 works here
Comment #47
rfayD8 first. Let's get these fixed on D8 and into D7.
Comment #48
Steven Jones CreditAttribution: Steven Jones commentedAdd a backport tag.
Comment #49
Steven Jones CreditAttribution: Steven Jones commented#24: 972536-24.patch queued for re-testing.
Comment #50
Jerome F CreditAttribution: Jerome F commented#24 tested again - removes the notice for me in Drupal 7.x.dev
Comment #51
bfroehle CreditAttribution: bfroehle commentedComment #52
Jerome F CreditAttribution: Jerome F commentedPatch in #49 was not applied to D7 last Dev 2011-Apr-17 and is not up to date. I updated it.
EDIT see #53 - sorry for that
Comment #53
bfroehle CreditAttribution: bfroehle commentedJeromeF: The patch in #24 (which is the one referenced from #49) does apply cleanly to both 7.x-dev and 8.x-dev.
Also, please be careful when re-rolling patches, as in this case you omitted complete part of the patch!
Comment #54
bfroehle CreditAttribution: bfroehle commentedPlease ignore the patch in #52. I've re-uploaded the patch in #24, which is the agreed upon RTBC patch.
Comment #55
Jerome F CreditAttribution: Jerome F commentedIt's also ok with last dev in D7 today.
If you please, I think it would be nice to commit this patch in D8 and then in D7, if you agree.
Comment #56
Alan D. CreditAttribution: Alan D. commentedA side note: Are the argument parameters cached? If so, this would lead to massive performance issues with the menu cache holding potentially huge class objects (like the entire $theme cache from #9 code). Also, if these objects change latter, does the resulting code use the cached, potentially stale, object or do they reload the class instance?
So, IMHO, a primary key should be stored and the object reloaded (a wildcard loader for #9 code). And without strong arguments against, I would mark this issue as "won´t fix" and have the documentation updated.
Comment #57
rootworksubscribe
Comment #58
DamienMcKennaSubscribe.
Comment #59
amateescu CreditAttribution: amateescu commentedFor everybody who subscribed here because of the nodequeue issue, I decided to fix this in the nodequeue module because I have no idea when/if this patch will make it into core.
Reference issue from the nodequeue module: #959904: Notice: Object of class stdClass could not be converted to int in _menu_router_build()
Comment #60
chx CreditAttribution: chx commentedI have written a little script that tries to reproduce this to no avail. So I am uploading the test from the RTBC'd patch to see whether it fails.
Comment #62
bfroehle CreditAttribution: bfroehle commentedSo yes, the tests do capture the bug. :)
chx: See #35 for a simple way to reproduce the bug. I've attached it as a PHP script for your convenience.
Comment #63
chx CreditAttribution: chx commentedLet it be then. I have simplified your reproduction script, filed http://bugs.php.net/bug.php?id=54980 because it is not a feature you need but a bugfix. Linked the bug report from a comment. That's the only change. Thanks for the reproduction script!
Comment #64
Alan D. CreditAttribution: Alan D. commented@chx or pwolanin
I'm asking you too directly as you are both listed as the main maintainers and will have the best knowledge to answer these two simple questions.
Is passing in and (presumably) using cached objects via the access arguments, etc a good idea? Should this be fixed?
I.e: This could result in clogging up the cache and strange bugs due to the use of stale data.
Two real life examples for the reasons why I'm concerned with this practice.
Lightbox2 had an innocence looking array of settings stored in the cck cache, but since this was based on # content types * imagecache settings * another parameter, we soon got MySQL errors on our local AND production servers due to the excessive packet size being thrown around.
I was involved in another project that mistakenly used cached objects, and we ended up with very strange errors when the trail of the cached object was obscured and it was used where it shouldn't have been, [updated and UOW pattern kicked in and saved the changes in the background]. Only a few sales were lost, but being the jewelry industry, this amounted to a few thousand dollars worth of sales missing, leading to some headaches with the book-keepers!
Comment #65
bfroehle CreditAttribution: bfroehle commentedAlan D.: I agree we shouldn't be storing cached objects here -- the two occurrences of this in core are addressed in #997918-5: Menu gets rebuilt continually during install and update (and menu system stores entire, out-of-date theme objects) which has been committed to 8.x, but not 7.x.
However, the use of an object (generically) as an access argument isn't in any way prohibited and this patch addresses that.
chx: As far as I know,
array_multisort($sort, SORT_NUMERIC, $menu);
is interpreted asThis means that $menu will be recursively sorted along all of its axes. This was the impetus for my "SORT_NONE" PHP feature request -- to turn off sorting along $menu as well.
(For those not in this discussion, the patch in #63 is ready to be committed to 8.x and 7.x).
Comment #66
chx CreditAttribution: chx commentedbfroehle check my bug report on php.net, the error is dependent on the first array which makes me think it's a PHP bug. Ie. the second array is not always sorted.
Comment #67
bfroehle CreditAttribution: bfroehle commentedAha, okay, I get it now! Thanks for the clarification chx.
Comment #68
Alan D. CreditAttribution: Alan D. commentedI never read anything to say that the menu system could handle complex data structures. Similar to the field system that doesn't tell you that it can not handle primary keys that are not integers.
It's sad not to have a response about the potential side effects of this. I guess that I will have to manually check every module before installation for crappy practices. Even if performance issues are seen, due to changes in the uninstall process, does this mean that we must do manual db removal of the left over menu items in the menu tables too? (i.e: hook_menu() is not longer called to remove the menu items there). :(
Comment #69
xjmTagging issues not yet using summary template.
Comment #70
Steven Jones CreditAttribution: Steven Jones commentedI added an issue summary.
Comment #70.0
xjmAdd issue summary
Comment #70.1
xjmUpdated issue summary.
Comment #70.2
xjmFormatting.
Comment #71
xjmThanks Steven! Edit: I added a few more details about the PHP bug, related tasks, and @amateescu's workaround to the summary.
Comment #71.0
xjmUpdated issue summary.
Comment #71.1
xjmUpdated issue summary.
Comment #71.2
xjmMissing period.
Comment #72
Steven Jones CreditAttribution: Steven Jones commented#63: 972536-63.patch queued for re-testing.
Comment #74
xjmPatch needs to be adjusted for changes from #1010480: Optimize _menu_navigation_links_rebuild() in
_menu_navigation_links_rebuild()
; the first hunk doesn't apply.Comment #74.0
xjmAdded note about tests.
Comment #75
Steven Jones CreditAttribution: Steven Jones commentedRe-rolled for 8.x.
Comment #76
Steven Jones CreditAttribution: Steven Jones commentedPerformance implications of this patch:
Firstly, changes are in code that is only called when doing a menu rebuild, so this doesn't effect every page load, only when rebuilding the menu cache.
We replace two calls to
array_multisort
toasort
but we are now essentially sorting less data, so these calls should be faster, not slower.Instead of additionally sorting the second array, we are looping over the first array to give us the iteration order of the second. This means that we've added a call to
array_keys
which will use more memory, and consume some CPU. I would suspect that the effect of this is minimal though. I don't think we need a benchmark here.Comment #78
Steven Jones CreditAttribution: Steven Jones commentedOuch, that was a lot of exceptions. We'll try that again, after I've re-rolled properly.
Comment #79
xjmRe: #76: As menu rebuild is plagued by performance issues that have the side effect of breaking websites, I think benchmarks are a good idea whenever we tinker with it. See #512962: Optimize menu_router_build() / _menu_router_save().
Comment #80
jhedstromSubscribe.
Comment #81
xjmComment #82
jamsilver CreditAttribution: jamsilver commentedAttached a backport to Drupal 7.
Comment #83
jamsilver CreditAttribution: jamsilver commentedAnd one that works.
Comment #86
roma7 CreditAttribution: roma7 commentedWorking D7 version of last patch
Comment #87
webflo CreditAttribution: webflo commentedComment #88.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #89
pwolanin CreditAttribution: pwolanin commentedmoving back to 7.x - the relevant code is being removed in 8.x
Comment #91
kyletaylored CreditAttribution: kyletaylored commentedre-rolled for testing.
Comment #93
Majdi CreditAttribution: Majdi commented#91 works for me
Comment #94
njbarrett CreditAttribution: njbarrett as a volunteer commentedFixed the link to the nodequeue commit which fixed the bug
Comment #95
alberto56 CreditAttribution: alberto56 at Dcycle commentedIn my opinion #91 is the right approach; however because tests are still failing and there remain tasks to be accomplished (see "Remaining tasks" in the issue description), and this is causing my continuous integration server to mark my builds are failures, I am including a very simple patch which simply suppresses the error, in case others might need a quick and dirty fix.
I am setting this to "needs review" just to see what the testbot thinks of it; however I do not suggest this as a real solution, just a workaround. If tests pass, this issue should still be moved back to "needs work", because the enclosed patch is not a real solution.
This patch is suggested only as a temporary workaround to prevent unwanted notices; if you want to move this issue forward please continue working from patch #91
Comment #97
alberto56 CreditAttribution: alberto56 at Dcycle commented