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

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)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tekante’s picture

Patch 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.

comicsal’s picture

Hopefully, 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.

WhiplashInfo’s picture

Version: 7.0-beta2 » 7.0-rc1
FileSize
520.03 KB

I 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

berenddeboer’s picture

Status: Active » Reviewed & tested by the community

Patch works for me...

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Hm. 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.

dobrzyns’s picture

Version: 7.0-rc1 » 7.0-rc3

I 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).

spacereactor’s picture

Version: 7.0-rc3 » 7.0

i having this problem with Nodequeue too. using drupal 7.0 and Nodequeue 7.x-2.x-dev.

Dave Reid’s picture

bfroehle’s picture

The 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

  foreach (list_themes() as $theme) {
    $items['admin/appearance/settings/' . $theme->name] = array(
      'access arguments' => array($theme),
...

$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

-  array_multisort($sort, SORT_NUMERIC, $menu);
+  array_multisort($sort, SORT_NUMERIC, $menu, SORT_NONE);

to indicate that we don't care about further sorting by $menu.

bfroehle’s picture

Status: Needs work » Needs review
FileSize
528 bytes

Here goes nothing...

Edit: There's another similar array_multisort in _menu_navigation_links_rebuild().

Anticosti’s picture

#10 Patch works perfect for me.
Thanks!

yuriy.babenko’s picture

Subscribing (patch in #10 helped - thanks!)

js’s picture

subscribing, and thanks!

bfroehle’s picture

If 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:

function MODULE_menu() {
  $data = array(
    'key' => 'value',
  );

  $items['object1'] = array(
    'title' => 'object1',
    'page arguments' => (object) $data,
  );
  ...
}

and see if you can get an error to trigger by just going to any page on the site.

David_Rothstein’s picture

There'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.

bfroehle’s picture

David 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.

bfroehle’s picture

Another 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.

femrich’s picture

I 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...

blischalk’s picture

Subscribing

rbahaguejr’s picture

I'm still getting this error with nodequeue.

Steven Jones’s picture

Status: Needs review » Needs work

I think that webchick is saying in #5 that this needs tests, so marking as needing work.

Steven Jones’s picture

Assigned: Unassigned » Steven Jones
Steven Jones’s picture

Status: Needs work » Needs review
FileSize
1.69 KB

Okay, 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.

Steven Jones’s picture

Assigned: Unassigned » Steven Jones
Issue tags: +nodequeue
FileSize
2.63 KB

So, 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.

rfay’s picture

Version: 7.0 » 7.x-dev
Issue tags: -Needs tests +nodequeue

Status: Needs review » Needs work
Issue tags: -nodequeue

The last submitted patch, 972536-24.patch, failed testing.

Steven Jones’s picture

Status: Needs work » Needs review

#23: 972536-23.patch queued for re-testing.

Steven Jones’s picture

Issue tags: +nodequeue

#24: 972536-24.patch queued for re-testing.

rfay’s picture

Yay, passed!

rfay’s picture

Version: 7.x-dev » 7.0

It 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...

rfay’s picture

#24: 972536-24.patch queued for re-testing.

Steven Jones’s picture

Assigned: Steven Jones » Unassigned

@rfay, yeah, do what you want.

bfroehle’s picture

FYI: 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).

rfay’s picture

#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 :-)

bfroehle’s picture

+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:

// Initialize $sort and $menu.
$sort['A'] = 2;
$sort['B'] = 1;
$sort['C'] = 1;
$menu['A']['access'] = array(1);
$menu['B']['access'] = array((object) array('a', 'b'));
$menu['C']['access'] = array(0);

// The next line triggers the bug.
array_multisort($sort, SORT_NUMERIC, $menu);
foreach ($menu as $path => $item) {
  // ...
}

The proposed patch instead does

// Initialize $sort and $menu.
// ...

asort($sort);
foreach (array_keys($sort) as $path) {
  $item = $menu[$path];
  // ...
}
Steven Jones’s picture

Issue tags: -nodequeue

#23: 972536-23.patch queued for re-testing.

bennash’s picture

Assigned: Steven Jones » Unassigned

#23: 972536-23.patch queued for re-testing.

Jerome F’s picture

#24 seems to work for me.

bennash’s picture

#24: 972536-24.patch queued for re-testing.

bdragon’s picture

subscribing.

Agileware’s picture

The patch in #24 stops the error appearing for me too.

Jerome F’s picture

Status: Needs review » Reviewed & tested by the community
Jerome F’s picture

Status: Reviewed & tested by the community » Needs work

I still get :

Notice : Object of class stdClass could not be converted to int in _menu_router_build() (line 3466 in (...)/Sites/acquia-drupal/includes/menu.inc).
Steven Jones’s picture

@Jerome F please document what steps you took to reproduce the error.

Jerome F’s picture

Status: Needs work » Reviewed & tested by the community

I 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.

mansspams’s picture

subscribing, #24 works here

rfay’s picture

Version: 7.0 » 8.x-dev
Status: Reviewed & tested by the community » Needs review

D8 first. Let's get these fixed on D8 and into D7.

Steven Jones’s picture

Issue tags: +Needs backport to D7

Add a backport tag.

Steven Jones’s picture

#24: 972536-24.patch queued for re-testing.

Jerome F’s picture

Status: Reviewed & tested by the community » Needs review

#24 tested again - removes the notice for me in Drupal 7.x.dev

bfroehle’s picture

Status: Needs review » Reviewed & tested by the community
Jerome F’s picture

FileSize
2.02 KB

Patch 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

bfroehle’s picture

Status: Needs review » Needs work

JeromeF: 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!

bfroehle’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.1 KB

Please ignore the patch in #52. I've re-uploaded the patch in #24, which is the agreed upon RTBC patch.

Jerome F’s picture

It'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.

Alan D.’s picture

A 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.

rootwork’s picture

subscribe

DamienMcKenna’s picture

Subscribe.

amateescu’s picture

Issue tags: -nodequeue

For 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()

chx’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.69 KB
175 bytes

I 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.

Status: Needs review » Needs work

The last submitted patch, 972536_60.patch, failed testing.

bfroehle’s picture

Status: Needs work » Needs review
FileSize
258 bytes

So 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.

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.88 KB

Let 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!

Alan D.’s picture

@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!

bfroehle’s picture

Alan 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 as

array_multisort($sort, SORT_ASC, SORT_NUMERIC, 
                $menu, SORT_ASC, SORT_REGULAR);

This 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).

chx’s picture

bfroehle 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.

bfroehle’s picture

Aha, okay, I get it now! Thanks for the clarification chx.

Alan D.’s picture

I 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). :(

xjm’s picture

Tagging issues not yet using summary template.

Steven Jones’s picture

I added an issue summary.

xjm’s picture

Issue summary: View changes

Add issue summary

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Formatting.

xjm’s picture

Thanks Steven! Edit: I added a few more details about the PHP bug, related tasks, and @amateescu's workaround to the summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Missing period.

Steven Jones’s picture

Issue tags: -Needs backport to D7

#63: 972536-63.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, 972536-63.patch, failed testing.

xjm’s picture

Patch needs to be adjusted for changes from #1010480: Optimize _menu_navigation_links_rebuild() in _menu_navigation_links_rebuild(); the first hunk doesn't apply.

xjm’s picture

Issue summary: View changes

Added note about tests.

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
2.98 KB

Re-rolled for 8.x.

Steven Jones’s picture

Performance 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 to asort 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.

Status: Needs review » Needs work

The last submitted patch, drupal-menu-int-972536-75.patch, failed testing.

Steven Jones’s picture

Status: Needs work » Needs review
FileSize
3.3 KB

Ouch, that was a lot of exceptions. We'll try that again, after I've re-rolled properly.

xjm’s picture

Re: #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().

jhedstrom’s picture

Subscribe.

xjm’s picture

jamsilver’s picture

Attached a backport to Drupal 7.

jamsilver’s picture

And one that works.

Status: Needs review » Needs work

The last submitted patch, drupal-menu-int-972536-83-D7.patch, failed testing.

roma7’s picture

Working D7 version of last patch

webflo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Needs backport to D7

The last submitted patch, drupal-menu-int-972536-86.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

pwolanin’s picture

Version: 8.x-dev » 7.x-dev
Issue summary: View changes

moving back to 7.x - the relevant code is being removed in 8.x

kyletaylored’s picture

Status: Needs work » Needs review
FileSize
3.46 KB

re-rolled for testing.

Status: Needs review » Needs work

The last submitted patch, 91: drupal-menu-int-972536-91.patch, failed testing.

Majdi’s picture

#91 works for me

njbarrett’s picture

Issue summary: View changes

Fixed the link to the nodequeue commit which fixed the bug

alberto56’s picture

Status: Needs work » Needs review
FileSize
1.59 KB

In 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

Status: Needs review » Needs work
alberto56’s picture

Issue summary: View changes