Download & Extend
Issues

If item is hidden in _menu_tree_check_access() skip it right away.

Posted by mikeytown2 on August 2, 2012 at 6:33pm
Project:Drupal core
Version:8.x-dev
Component:menu system
Category:task
Priority:normal
Assigned:Unassigned
Status:needs review
Issue tags:Performance

Issue Summary

Drupal will load unnecessary entities (non-node) if a menu link to them is in the main menu. The reason I say it's loading them unnecessarily has to do with the fact that if a menu item is disabled it will still do an entity_load on it.

How to repo:
Standard Drupal Install
Create a new user
Link to user/2 in Main menu (admin/structure/menu/manage/main-menu)
Disable that link in the menu.
user/2 is still loaded on every page load even though that menu item is disabled.

Login or register to post comments

Comments

#1

Posted by mikeytown2 on August 2, 2012 at 6:34pm
Status:active» needs review
AttachmentSizeStatusTest resultOperations
drupal-1710656-1-skip-hidden-menu-items-D7.patch617 bytesIdleFAILED: [[SimpleTest]]: [MySQL] 39,342 pass(es), 3 fail(s), and 0 exception(s).View details | Re-test

#2

Posted by System Message on August 2, 2012 at 7:16pm
Status:needs review» needs work

The last submitted patch, drupal-1710656-1-skip-hidden-menu-items-D7.patch, failed testing.

#3

Posted by mikeytown2 on August 2, 2012 at 8:59pm
Status:needs work» needs review

This patch should take care of the breadcrumb issue.

AttachmentSizeStatusTest resultOperations
drupal-1710656-3-skip-hidden-menu-items-D7.patch702 bytesIdlePASSED: [[SimpleTest]]: [MySQL] 39,352 pass(es).View details | Re-test

#4

Posted by mikeytown2 on August 2, 2012 at 10:13pm

menu_tree_check_access() on a logged out (anonymous) front page hit
Without Patch
cumulative time: 29ms
calls: 3

With Patch
cumulative time: 4.4ms
calls: 3

Results will vary depending on the site, but overall this should reduce the amount of code being ran.

#5

Posted by mikeytown2 on August 2, 2012 at 10:29pm
Version:7.x-dev» 8.x-dev

Patch for Drupal 8 attached below

AttachmentSizeStatusTest resultOperations
drupal-1710656-5-skip-hidden-menu-items-D8.patch722 bytesIdlePASSED: [[SimpleTest]]: [MySQL] 39,760 pass(es).View details | Re-test

#6

Posted by mikeytown2 on September 13, 2012 at 6:05pm

Patch for D6 after seeing this issue here http://drupal.stackexchange.com/questions/42232/ubercart-entries-in-menu...

Benchmark
Before: 22ms
After: 20ms
So a slight improvement on our site, but depending on the menu structure it could have a big improvement. I can verify that less code is being ran because the cachegrind file shrunk by 300kb with this patch.

AttachmentSizeStatusTest resultOperations
drupal-1710656-6-skip-hidden-menu-items-D6-do-not-test.patch2.02 KBIgnoredNoneNone

#7

Posted by oliverpolden on September 14, 2012 at 10:22am

The Drupal 6 patch works a treat. Is it safe to apply in a live environment?

#8

Posted by mikeytown2 on September 14, 2012 at 10:36am

The patch passed all D7 & D8 tests, so that's a good sign. My guess is it is safe to apply to D6 but I can't guarantee that its 100% safe.

#9

Posted by mukesh_k_see_no... on September 22, 2012 at 7:09pm

Yes Mike, it works. After apply in D7, I have also found some noticeable difference in profiling results.

#10

Posted by AntiNSA on February 24, 2013 at 2:48pm

Subscribing. can anyone verify they tested drupal 6 no problems?

#11

Posted by mikeytown2 on February 25, 2013 at 11:08pm

I use the D6 patch on our production boxes. No issues have been reported.

nobody click here