Okay, this has to be the weirdest drupal bug I have come to see thus far and I have to admit I didn't see this thing all the way through, partly because of it's quirkiness, partly because of a deadline I already busted.

I wrote a module which enables the site admin to create menu-blocks (completely independent from the module with that name!) that display the menu items with optional images and orders the items with some taxonomy-magic.

Now I wanted to display the main menu two times, one time as a dropdown menu like described above, and one time in the footer.
Everything works fine with that, output is just like expected. But, and this is the strange thing: If there are two blocks, the following type of error occurs for every menu-item that links to a node:

Notice: Trying to get property of non-object in node_page_title() (Zeile 2109 von /var/www/localhost/htdocs/catcruising/www/modules/node/node.module).

I've tracked this down to menu_build_tree, which I use in order to get the menu that is to be rendered.

The trace goes roughly like this: menu_build_tree -> menu_tree_check_access -> _menu_tree_check_access -> _menu_link_translate -> _menu_item_localize

As far as I can tell the following line in _menu_item_localize is the culprit:

$item['title'] = call_user_func_array($callback, menu_unserialize($item['title_arguments'], $map));

For some reason I cannot grasp, the second time around the node-titles are tried to be translated, calling node_page_title with the argument ($node) being a simple integer, instead of a node-object.

In case it's not yet obvious: It seems that you cannot call menu_build_tree for the same menu multiple times in the same page request.

My current workaround for this problem is setting a global variable with the menu tree, which works just fine. But I think if I tried to output the same menu once with the default drupal means and once using my module (or another) this bug would surface again.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ghedipunk’s picture

Version: 7.14 » 7.x-dev

Issue continues to exist in 7.15, so switching this to -dev.

Fetching a node to pass to node_page_title() seems like it will fix a lot of issues at this point. I heartily recommend checking to see if our $callback is node_page_title, and if our title arguments unserialize to an integer, loading the corresponding node. (I also suggest continuing to use node_page_title(), in case its implementation ever changes in the future, despite it simply returning the $node->title property.)

// line 694 of menu.inc:
// node_page_title requires a fully formed node, or at least an object with
// a 'title' property.  Make sure that we're passing a node and not just a nid.
elseif ($callback == 'node_page_title') {
  $title_args = menu_unserialize($item['title_arguments'], $map);
  $title_args = array_shift($title_args);
  if (is_numeric($title_args)) {
    $title_args = node_load($title_args);
  }
  $item['title'] = node_page_title($title_args);
}

This will fix _menu_item_localize(), which is clearly broken when the title callback is node_page_title()... However, this won't solve the overall problem of _menu_item_localize() being called in the first place despite no need for translation to happen.

ghedipunk’s picture

Status: Active » Needs review
FileSize
972 bytes

Trying my hand at an actual patch... bear with me, this is my first one (translation: be hard on me, I need to learn).

Iztok’s picture

I can confirm the behaviour, and that patch fixes my problem. I hope this gets to 7.16.

rooby’s picture

Sorry to say but I don't think this patch would get into drupal core.

Sure it might fix your problem but the solution is a bit of a hack.

The menu system should be generic, but the code in this patch is very specific to this one use case and is more of a work around than a fix.

From what I found at #429910-11: Improper usage of node_page_title() function by _menu_item_localize() function in includes/menu.inc there is a problem with _menu_link_translate() in this part of the code:

<?php
    // menu_tree_check_access() may set this ahead of time for links to nodes.
    if (!isset($item['access'])) {
      if (!empty($item['load_functions']) && !_menu_load_objects($item, $map)) {
        // An error occurred loading an object.
        $item['access'] = FALSE;
        return FALSE;
      }
      _menu_check_access($item, $map);
    }
    // For performance, don't localize a link the user can't access.
    if ($item['access']) {
      _menu_item_localize($item, $map, TRUE);
    }
?>

The problem is that _menu_item_localize() requires that _menu_load_objects() has already been run for the item (that function handles loading the nid into the node).
This piece of code however means that if $item['access'] is already set, _menu_load_objects() is not run, so we don't ahve a node object and we get the error from _menu_item_localize().
This affects you because the menu tree is static and so persists for the full page load no matter how many times the process is run. So the first time it runs $item['access'] is set and then every time after that it is already set due to being static, so the node isn't loaded.

This is also the root of the problem that means if you add a node/% menu item to the maintenance menu, under the Administration menu item (just using the menu UI), you will end up with this error via toolbar_get_menu_tree().

I will see if I can make a patch to fix it.

rooby’s picture

Title: menu_build_tree can only be called once for every menu and page request » _menu_load_objects() is not always called when building menu trees
FileSize
998 bytes

Here is a quickie patch.

The patch fixes my issue:

Notice: Trying to get property of non-object in node_page_title() (line 2123 of /var/www/mysite/modules/node/node.module).

but I have not yet done thorough testing or reviewed possible performance implications.

Can you test it and see if it fixes your problem with two menu blocks?

star-szr’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

@rooby - Can you please try to reproduce this with a fresh install of Drupal 8? If the issue still exists in Drupal 8 it should be fixed there per the backport policy.

rooby’s picture

Yeah no worries, that was next on my list.
I'll do it tonight.

anou’s picture

Hello,
Issue still happening on D7.18.
I'm using the menu_build_tree() function in a custom module. Notices are displayed when submit my module settings form.

Patch from #5 has solved it for me.
Thanks

bbinkovitz’s picture

I had the problem in Drupal 7.19 and the patch resolved it.

nooby’s picture

#5 worked for me (Drupal 7.19)

rooby’s picture

Status: Needs work » Needs review
FileSize
1018 bytes

I can confirm this is still a problem in drupal 8.
In my case though it manifests itself in a different error:

PHP Fatal error: Call to a member function label() on a non-object in /var/www/drupal8/core/modules/node/node.module on line 1858

Recoverable fatal error: Argument 1 passed to node_page_title() must be an instance of Drupal\node\Plugin\Core\Entity\Node, string given in node_page_title() (line 1857 of /var/www/drupal8/core/modules/node/node.module).

which is the equivalent of the error mentioned in #5 and the OP.

To reproduce the error:

  1. Install drupal with default settings.
  2. Create a new node.
  3. Add a menu item to the "Administration" menu as a child of the "Administration" menu item and point it to node/1
  4. Create a new test user and give them the administrator role.
  5. Run drush cc all
  6. Log into the site as the new test user.

This patch (direct port of the drupal 7 one in #5) fixes it for me.

cjamesrun’s picture

Patch #5 worked for me. Drupal 7.21 I was having this issue. I thought it was connected to this.

rooby’s picture

There have been some menu changes in HEAD.
This bug still stands.

Here is an updated patch that still fixes the issue.

Note that I need to get up to speed with the new routing system so this is new version is likely not the best solution.
Can someone speak to how/if the new routing system relates to _menu_load_objects()? and if _menu_load_objects() will still remain in drupal 8 come release time?

I will investigate but if someone knows the answer already all the better.

Status: Needs review » Needs work

The last submitted patch, drupal-menu_always_load_objects-1697570-13.patch, failed testing.

markhalliwell’s picture

#5 worked for me as well in 7.22.... very annoying bug, this should be committed to D7 please.

rooby’s picture

Yeah, my guess it that this is not going to be a thing in drupal 8 because the menu system is being majorly changed.

Hopefully someone with knowledge can confirm.

johnv’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review

Let's do a retest on D7

johnv’s picture

hefox’s picture

Patch #5 works well for me

markhalliwell’s picture

Status: Needs review » Reviewed & tested by the community

7.x - Patch in #5 passed. Marking as RTBC since I can confirm this works and fixes the issue in 7.x.

8.x - I know it "should" be the other way around, but until we can get some feedback from those more familiar with 8.x, I'm not sure if this is even a priority or it may even be already fixed with all the menu changes in 8.x. We can port it forward if needed.

rooby’s picture

I think last time I checked drupal 8 it still had a similar issue but the code in that area has changed a little and I haven't spent the time to investigate the changes yet.

I would love to get this into drupal 7 though so I don't have to keep patching all my sites and we don't keep having duplicate issues pop up around the place.

Note, I have a number of sites that have been running this patch live for the last 6 months without issue.

hefox’s picture

Reuploading #5, no changes, just to make sure there's no confusion for which is the patch being tested and is rtbc for 7

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +Needs backport to D7

The code currently looks like this in Drupal 8:

    // menu_tree_check_access() may set this ahead of time for links to nodes.
    if (!isset($item['access'])) {
      if ($route = $item->getRoute()) {
        $item['access'] = menu_item_route_access($route, $item['href'], $map);
      }
      elseif (!empty($item['load_functions']) && !_menu_load_objects($item, $map)) {
        // An error occurred loading an object.
        $item['access'] = FALSE;
        return FALSE;
      }
      // Apply the access check defined in hook_menu() if there is not route
      // defined.
      else {
        _menu_check_access($item, $map);
      }
    }

So it's possible that by the time Drupal 8 is released there will always be a route and the remaining code after that will be gone and thus this bug won't exist... But in the meantime, it's there and I think we should fix this in Drupal 8 to avoid the possibility of regressions.

Especially because this issue could really use a functional test, and the test can definitely go in Drupal 8 either way.

David_Rothstein’s picture

+    // Problems may arise if this step is skipped.

This should ideally explain the possible problem more specifically.... I think the comments in this issue (and an eventual test) would naturally help figure out how to do that.

rooby’s picture

Status: Needs work » Needs review
rooby’s picture

Assigned: Unassigned » rooby

I'll take a look at this.

rooby’s picture

Try this one.

rooby’s picture

Oops, I forgot tests.
I'll do that tomorrow.

Cyberwolf’s picture

Status: Needs work » Needs review
FileSize
696 bytes

Loading the objects in the argument map will solve this issue but will potentially dramatically reduce performance. I only tested (and encountered) this issue on D7 and I see it only occurring when _menu_link_translate() is called twice on the same menu link item that points to a node. This occurs when building the same menu tree more than once with menu_build_tree() because it uses statically cached menu trees from _menu_build_tree().

Attached patch for D7 adds a key "menu_link_translated" to menu link items in _menu_tree_check_access() and checks if this flag is present already before calling _menu_link_translate() on it. Going to test this a bit more.

Status: Needs review » Needs work

The last submitted patch, drupal7.menu-system.1697570-29.patch, failed testing.

johnv’s picture

Status: Needs review » Needs work

#1978176: Build menu_tree without loading so many objects takes another route (in D7/D8), by loading the object as late as possible.
It contains this message as a symptom in the summary, but apparantly, I forgot to add a crosspost here.
Honoustly, I don't know if the new router system contains the same problem, and wether the D7-menu items are still supported in D8.

rooby’s picture

@johnv:

Last I checked in August drupal 8 still has the same problem.
It possibly won't buy the time drupal 8 ships but until then we also need a drupal 8 patch.

@Cyberwolf:
Seems like a decent solution. I'll test too when I get a chance.

rooby’s picture

@johnv:

I like the sound of the patch in the other issue.
I'll try to review that one too when I get some time.

johnv’s picture

It seems like that D8 is about to remove the D7 menu routing (I haven't checked the current status of the code shown in #23 yet, and perhaps I just misunderstand the issue at hand) :
https://groups.drupal.org/node/342698
https://drupal.org/node/2106709

cashwilliams’s picture

I think @johnv is correct in that D8 is removing menu routing, so it may be appropriate to move this back to 7.x-dev.

The patch in #29 fixes the problem on the client's site where I found this bug.

baronmunchowsen’s picture

Applying the patch in #29 against Drupal 7.23 worked for me.

twardnw’s picture

I applied the patch in #29 against 7.24 and it resolved this issue for me.

drumm’s picture

Issue summary: View changes
Issue tags: +affects drupal.org

We ran into this on a DrupalCon site. twardnw tested it.

rj’s picture

Patch in #29 against 7.24 resolved this issue for me.

xaa’s picture

Patch in #29 against 7.26 resolved this issue for me too.

Could you explain why the patch could potentially dramatically reduce performance?

rooby’s picture

Because menus can be large and built many times and adding this extra logic in to be called potentially so many times could be a performance hit depending on the site.
A lot of sites may not suffer at all though.

Based on ticket conversation, the patch in #1978176: Build menu_tree without loading so many objects appears to be a better solution but I haven't had a chance to properly check it out for myself yet.

tompagabor’s picture

Patch in #29 against 7.28 resolved this issue for me.

pwolanin’s picture

Version: 8.x-dev » 7.x-dev

Seems like this is not really reelvant to D8 now - the linked issue is also now just for 7.

cashwilliams’s picture

Status: Needs work » Needs review
cashwilliams’s picture

Status: Needs review » Reviewed & tested by the community

#29 still passes and has been used in production in a number of sites for over 8 months.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: drupal7.menu-system.1697570-29.patch, failed testing.

Steven Jones’s picture

So are we saying that we should pursue a fix for this in #1978176: Build menu_tree without loading so many objects?

David_Rothstein’s picture

Status: Needs work » Reviewed & tested by the community

Likely testbot glitch - moving back to RTBC.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

It sounds like there isn't quite a consensus here yet. Plus, the patch still has no tests and it seems like the kind of bug that would ideally have them.

Also:

+    // Prevent translating menu links twice.
+    if (!isset($item['menu_link_translated'])) {
+      _menu_link_translate($item);
+      $item['menu_link_translated'] = TRUE;
+    }

There are many other places in the codebase that call _menu_link_translate()... so shouldn't any fix for "prevent translating menu links twice" happen inside _menu_link_translate() itself?

rooby’s picture

I agree that the fix should be inside _menu_link_translate() because that is where the problem lies.

Changing when _menu_link_translate() is called doesn't fix the bug that is in that function.

My original patch, while maybe a little heavy handed, helps better see what the actual problem is. (#22 is the latest D7 version of that patch.)

larowlan’s picture

Status: Needs review » Needs work

One thing that manifests with the patch at #22 is that overridden menu-link titles aren't used.
Eg if your node title is 'Some super long title that won't fit the menu' and your menu link is 'Short title' you see the first in the menu.

Working on patch with a test

larowlan’s picture

From #11

Install drupal with default settings.
Create a new node.
Add a menu item to the "Administration" menu as a child of the "Administration" menu item and point it to node/1
Create a new test user and give them the administrator role.
Run drush cc all
Log into the site as the new test user.

The attached test does that, but passes - any pointers on what is different?
So, in summary starts on a test based on #11 but it passes with and without the patch

rooby’s picture

Assigned: rooby » Unassigned

In that case, unassigning.

rooby’s picture

When you say overridden, what kind of overriding do you mean?

[EDIT] Oh sorry, I see what you mean.

larowlan’s picture

The issue with the patch that results in the wrong titles is that _menu_item_localize() should not be called twice else it results in this line always evaluating TRUE

if (!$link_translate || ($item['title'] == $item['link_title'])) {

Which will invoke node_page_title() for nodes even when a custom menu link title is provided.

Fix attached.

So all that remains is to make the test fail against HEAD - any extra detail on steps to reproduce relative to the test?

bdombro’s picture

Comment #29 worked for me

guy_schneerson’s picture

Patch in #29 fixed it for me.
Looks like my issue is caused by Bootstrap module calling menu_build_tree() see #2302513: Book ToC generating errors

stefan.r’s picture

Would this issue be solved by #1978176: Build menu_tree without loading so many objects (which is RTBC)?

johnv’s picture

@stefan.r , indeed.

stefan.r’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs backport to D7
FileSize
2.3 KB
1.06 KB
stefan.r’s picture

Priority: Normal » Major

Upgrading priority as this can make menu links disappear / construct faulty trees with items without titles.

Status: Needs review » Needs work

The last submitted patch, 60: 1697570-59-testonly.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review

Tests have been added, both this issue and #1978176: Build menu_tree without loading so many objects make them go green.

Can this go back to RTBC as per #48/ #49 or alternatively we could get consensus to go for #1978176: Build menu_tree without loading so many objects instead?

hackwater’s picture

Both the patch in this issue and in #1978176: Build menu_tree without loading so many objects work for me, but I like the approach in #1978176: Build menu_tree without loading so many objects better, for what it's worth.

labboy0276’s picture

We fell pray to this issue after upgrading to 7.36 today. Luckily the patch in #60 fixed us up right away. Not sure if it is a 7.36 issue, but this did not happen until after we upgraded.

rooby’s picture

This is definitely not a 7.36 issue. This issue has been open for nearly 3 years.

labboy0276’s picture

Most probably rooby. I am just saying on a site that had zero issues and then we upgraded to 7.36 then this very issue transpired after the upgrade. Regardless what the cause was, the patch in #60 ended up causing more harm then good. #29 did the tick in the end.

Anonymous’s picture

I've been trying to debug this issue myself on a 7.36 installation for a menu with two languages. It seems that the problem is as found above, where items are translated twice. Patch in #29 fixed the issue

eric.chenchao’s picture

Confirm patch in #29 fixed the issue. My case is using menu_tree_page_data() to generate mobile menu with max_depth=1 and get this error.

cammchugh’s picture

Anecdotally, we applied #60 to a production site and saw dramatic increases in CPU load on web & DB servers. I ran Apache Bench locally both pre & post patch to confirm what we were being told by our sysadmins, throughput in requests/second dropped by 75% and CPU load effectively doubled with the patch applied. There's a large menu rendered out on each page request, so we might be an edge case that's particularly affected, but thought it was worth mentioning.

Have yet to try #29, I'm sure it solves the bug as well, but we'll likely restructure our own module code to work around the issue instead for the time being.

berliner’s picture

Are there any fundamental drawbacks when using the following code in a custom module directly before calling menu_build_tree()?

drupal_static_reset('_menu_build_tree');

Performance-wise it sounds like a bad idea, but it allowed me to solve the described problem without patching core.

NancyDru’s picture

The patch from #27 worked for me in 7.41.

belaustegui’s picture

Workaround explained in #72 solved the notices.
I'm opting for this while the core is updated with the patch.

leisurman’s picture

patch 29 did not work for me. patch 5 gave me error
error php EntityMalformedException: Missing bundle property on entity of type node. in entity_extract_ids() (line 7880 of /var/www/html/upgrade2a/includes/common.inc).

but the other error went away:
php Notice: Trying to get property of non-object in
node_page_title() (line 2200 of mysite/modules/node/node.module).

leisurman’s picture

Can any say if the patch from #60 is safe to use. That is a lot of new code.

leisurman’s picture

epavlov’s picture

Drupal 7.50 here: still not fixed. #72 resolves problem with no performance issues.

ConradFlashback’s picture

+1

RoSk0’s picture

Lets see if it still working.

RoSk0’s picture

Status: Needs review » Reviewed & tested by the community

That patch fixed the issue for me and I will be bold.

stefan.r’s picture

Issue tags: +Drupal 7.60 target
joseph.olstad’s picture

Seems like #1978176: Build menu_tree without loading so many objects
Is the way to go, provided that the tests here pass over in #1978176: Build menu_tree without loading so many objects
I'd have liked to see this in sooner than 7.60.

joseph.olstad’s picture

Status: Reviewed & tested by the community » Closed (duplicate)
DigitWings’s picture

I can confirm that the patch in #1978176: Build menu_tree without loading so many objects fixes the problem mentioned in this issue for my particular use case (which involved book module and transliteration module).

joseph.olstad’s picture

Digitwings, please report your successful test results in the other issue as well. It will encourage the core maintainers to take action with more confidence.
#1978176: Build menu_tree without loading so many objects

DigitWings’s picture

@joseph.olstad, I can't technically report in that issue because, the other one caters to performance improvement, which i have not noticed in my small site. But the patch DOES fix the problem mentioned in this issue.

DigitWings’s picture

maybe it was a bad idea to close this as duplicate, and instead keep it open and point to the patch in other issue queue?

joseph.olstad’s picture

DigitWings, I understand that you might not have noticed a performance improvement, it's (if I recall correctly) cache rebuilds, (first page load after cache clear all ) that is faster. However your feedback is important to the other issue because as an added bonus the performance patch also fixes an issue you have, so in that sense, it is appropriate to provide feedback in the other issue as well.

Thanks for your feedback, it will help the core maintainers.

MustangGB’s picture

Issue tags: -Drupal 7.60 target
cayerdis’s picture

I confirm #5 solved my issue for Drupal 7. PHP 5.6

joseph.olstad’s picture

@Cayerdis For perfomance reasons I would not use #5 I recommend instead the RTBC patch in issue number #1978176: Build menu_tree without loading so many objects

Can you please be so kind as to revert patch number 5 and apply the patch found in #1978176: Build menu_tree without loading so many objects

And please report your results here and in #1978176: Build menu_tree without loading so many objects