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.
Comment | File | Size | Author |
---|---|---|---|
#80 | 1697570-59-patchandtest.patch | 2.3 KB | RoSk0 |
#60 | 1697570-59-testonly.patch | 1.06 KB | stefan.r |
#60 | 1697570-59-patchandtest.patch | 2.3 KB | stefan.r |
#55 | menu_load_objects_is-1697570-53.patch | 1.24 KB | larowlan |
#55 | interdiff.txt | 634 bytes | larowlan |
Comments
Comment #1
ghedipunk CreditAttribution: ghedipunk commentedIssue 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.)
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.
Comment #2
ghedipunk CreditAttribution: ghedipunk commentedTrying my hand at an actual patch... bear with me, this is my first one (translation: be hard on me, I need to learn).
Comment #3
Iztok CreditAttribution: Iztok commentedI can confirm the behaviour, and that patch fixes my problem. I hope this gets to 7.16.
Comment #4
rooby CreditAttribution: rooby commentedSorry 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:
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.
Comment #5
rooby CreditAttribution: rooby commentedHere is a quickie patch.
The patch fixes my issue:
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?
Comment #6
star-szr@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.
Comment #7
rooby CreditAttribution: rooby commentedYeah no worries, that was next on my list.
I'll do it tonight.
Comment #8
anouHello,
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
Comment #9
bbinkovitz CreditAttribution: bbinkovitz commentedI had the problem in Drupal 7.19 and the patch resolved it.
Comment #10
nooby CreditAttribution: nooby commented#5 worked for me (Drupal 7.19)
Comment #11
rooby CreditAttribution: rooby commentedI can confirm this is still a problem in drupal 8.
In my case though it manifests itself in a different error:
which is the equivalent of the error mentioned in #5 and the OP.
To reproduce the error:
drush cc all
This patch (direct port of the drupal 7 one in #5) fixes it for me.
Comment #12
cjamesrun CreditAttribution: cjamesrun commentedPatch #5 worked for me. Drupal 7.21 I was having this issue. I thought it was connected to this.
Comment #13
rooby CreditAttribution: rooby commentedThere 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.
Comment #15
markhalliwell#5 worked for me as well in 7.22.... very annoying bug, this should be committed to D7 please.
Comment #16
rooby CreditAttribution: rooby commentedYeah, 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.
Comment #17
johnvLet's do a retest on D7
Comment #18
johnv#5: drupal-menu_always_load_objects-1697570-5.patch queued for re-testing.
Comment #19
hefox CreditAttribution: hefox commentedPatch #5 works well for me
Comment #20
markhalliwell7.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.
Comment #21
rooby CreditAttribution: rooby commentedI 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.
Comment #22
hefox CreditAttribution: hefox commentedReuploading #5, no changes, just to make sure there's no confusion for which is the patch being tested and is rtbc for 7
Comment #23
David_Rothstein CreditAttribution: David_Rothstein commentedThe code currently looks like this in Drupal 8:
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.
Comment #24
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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.
Comment #25
rooby CreditAttribution: rooby commented#13: drupal-menu_always_load_objects-1697570-13.patch queued for re-testing.
Comment #26
rooby CreditAttribution: rooby commentedI'll take a look at this.
Comment #27
rooby CreditAttribution: rooby commentedTry this one.
Comment #28
rooby CreditAttribution: rooby commentedOops, I forgot tests.
I'll do that tomorrow.
Comment #29
Cyberwolf CreditAttribution: Cyberwolf commentedLoading 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.
Comment #31
johnv#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.
Comment #32
rooby CreditAttribution: rooby commented@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.
Comment #33
rooby CreditAttribution: rooby commented@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.
Comment #34
johnvIt 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
Comment #35
cashwilliams CreditAttribution: cashwilliams commentedI 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.
Comment #36
baronmunchowsen CreditAttribution: baronmunchowsen commentedApplying the patch in #29 against Drupal 7.23 worked for me.
Comment #37
twardnw CreditAttribution: twardnw commentedI applied the patch in #29 against 7.24 and it resolved this issue for me.
Comment #38
drummWe ran into this on a DrupalCon site. twardnw tested it.
Comment #39
rj CreditAttribution: rj commentedPatch in #29 against 7.24 resolved this issue for me.
Comment #40
xaa CreditAttribution: xaa commentedPatch in #29 against 7.26 resolved this issue for me too.
Could you explain why the patch could potentially dramatically reduce performance?
Comment #41
rooby CreditAttribution: rooby commentedBecause 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.
Comment #42
tompagabor CreditAttribution: tompagabor commentedPatch in #29 against 7.28 resolved this issue for me.
Comment #43
pwolanin CreditAttribution: pwolanin commentedSeems like this is not really reelvant to D8 now - the linked issue is also now just for 7.
Comment #44
cashwilliams CreditAttribution: cashwilliams commented29: drupal7.menu-system.1697570-29.patch queued for re-testing.
Comment #45
cashwilliams CreditAttribution: cashwilliams commented#29 still passes and has been used in production in a number of sites for over 8 months.
Comment #47
Steven Jones CreditAttribution: Steven Jones commentedSo are we saying that we should pursue a fix for this in #1978176: Build menu_tree without loading so many objects?
Comment #48
David_Rothstein CreditAttribution: David_Rothstein commentedLikely testbot glitch - moving back to RTBC.
Comment #49
David_Rothstein CreditAttribution: David_Rothstein commentedIt 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:
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?
Comment #50
rooby CreditAttribution: rooby commentedI 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.)
Comment #51
larowlanOne 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
Comment #52
larowlanFrom #11
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
Comment #53
rooby CreditAttribution: rooby commentedIn that case, unassigning.
Comment #54
rooby CreditAttribution: rooby commentedWhen you say overridden, what kind of overriding do you mean?
[EDIT] Oh sorry, I see what you mean.
Comment #55
larowlanThe 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
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?
Comment #56
bdombro CreditAttribution: bdombro commentedComment #29 worked for me
Comment #57
guy_schneerson CreditAttribution: guy_schneerson commentedPatch 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
Comment #58
stefan.r CreditAttribution: stefan.r commentedWould this issue be solved by #1978176: Build menu_tree without loading so many objects (which is RTBC)?
Comment #59
johnv@stefan.r , indeed.
Comment #60
stefan.r CreditAttribution: stefan.r commentedComment #61
stefan.r CreditAttribution: stefan.r commentedUpgrading priority as this can make menu links disappear / construct faulty trees with items without titles.
Comment #63
stefan.r CreditAttribution: stefan.r commentedTests 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?
Comment #64
hackwater CreditAttribution: hackwater commentedBoth 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.
Comment #65
labboy0276 CreditAttribution: labboy0276 commentedWe 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.
Comment #66
rooby CreditAttribution: rooby commentedThis is definitely not a 7.36 issue. This issue has been open for nearly 3 years.
Comment #67
labboy0276 CreditAttribution: labboy0276 commentedMost 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.
Comment #68
Anonymous (not verified) CreditAttribution: Anonymous commentedI'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
Comment #69
eric.chenchao CreditAttribution: eric.chenchao commentedConfirm 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.
Comment #70
cammchugh CreditAttribution: cammchugh commentedAnecdotally, 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.
Comment #72
berliner CreditAttribution: berliner commentedAre there any fundamental drawbacks when using the following code in a custom module directly before calling
menu_build_tree()
?Performance-wise it sounds like a bad idea, but it allowed me to solve the described problem without patching core.
Comment #73
NancyDruThe patch from #27 worked for me in 7.41.
Comment #74
belaustegui CreditAttribution: belaustegui commentedWorkaround explained in #72 solved the notices.
I'm opting for this while the core is updated with the patch.
Comment #75
leisurman CreditAttribution: leisurman commentedpatch 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).
Comment #76
leisurman CreditAttribution: leisurman commentedCan any say if the patch from #60 is safe to use. That is a lot of new code.
Comment #77
leisurman CreditAttribution: leisurman commentedIs this a good solution?
https://www.drupal.org/node/1709914#comment-10753712
Comment #78
epavlov CreditAttribution: epavlov commentedDrupal 7.50 here: still not fixed. #72 resolves problem with no performance issues.
Comment #79
ConradFlashback CreditAttribution: ConradFlashback commented+1
Comment #80
RoSk0Lets see if it still working.
Comment #81
RoSk0That patch fixed the issue for me and I will be bold.
Comment #82
stefan.r CreditAttribution: stefan.r commentedComment #83
joseph.olstadSeems 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.
Comment #84
joseph.olstadPrefering
#1978176: Build menu_tree without loading so many objects
Comment #85
DigitWings CreditAttribution: DigitWings commentedI 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).
Comment #86
joseph.olstadDigitwings, 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
Comment #87
DigitWings CreditAttribution: DigitWings commented@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.
Comment #88
DigitWings CreditAttribution: DigitWings commentedmaybe it was a bad idea to close this as duplicate, and instead keep it open and point to the patch in other issue queue?
Comment #89
joseph.olstadDigitWings, 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.
Comment #90
MustangGB CreditAttribution: MustangGB commentedComment #91
cayerdis CreditAttribution: cayerdis commentedI confirm #5 solved my issue for Drupal 7. PHP 5.6
Comment #92
joseph.olstad@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