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

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

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

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

3: drupal-1710656-3-skip-hidden-menu-items-D7.patch queued for re-testing.

3: drupal-1710656-3-skip-hidden-menu-items-D7.patch queued for re-testing.

I would suggest to postpone this on #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit given that it will conflict most certainly.

The D7 patch saved me a nice ~50ms, thank you:)

RTBC ++

I've thoroughly tested this patch on a complex multi-lingual site with over 500 modules and features and it works great. We've followed Joelpittets' recommended low risk patches and achieved amazing performance gains. Also thanks to Joelpittets' review of patches we've gotten incredible improvements in authenticated user site performance with his patch recommendations. I suggest to others using D7 to check out Joelpittets' recommended patches.

It would be great to get this kind of performance improvement in, but I'm not sure we can do it like this.

With this patch, the return value of some public API functions is going to change (for example, menu_build_tree()), kind of arbitrarily. Before they would include disabled links if the user had access to them, and afterwards they won't necessarily. It is hard to predict the consequences of that kind of change.

Maybe we can do this with some API additions that allow particular menu functions to request that hidden menu items never be loaded (in the case where they are building the menu for display purposes only)? For example, I haven't checked this carefully but it looks to me like both menu_navigation_links() and menu_tree() build the entire menu and then throw away the hidden links afterwards? If there were a way for those specific functions to signal that hidden menu items can be thrown away during the tree-building itself (without bothering to translate or check access for them) then I think we potentially get the same performance benefit but without any unexpected API changes.

We've been using this with drupal core 7.35, drupal core 7.37 and the latest drupal core 7.38 and it's working as expected.

We're still running this on production sites now with drupal core 7.39.

Have not noticed any problem or conflict and we're running full i18n multilingual D7 . Appreciate the performance gain especially on sites with lots of menu links.

RTBC
This patch is still working on Drupal 7 core 7.50 , used it also with Drupal core 7.44

We're doing multilingual sites and have created content programmatically , used it with entity translation or node translation , menu link translation.

Great work @mikeytown2 , your gaining quite a fabulous reputation for yourself here in the core queue.

as for API changes, I'm speaking empirically here, after several years of this patch being used it dosen't have any reported conflict with any contrib module, meaning its compatible with contrib as is. If we change the API that *might* mean forced changes to contrib.

RTBC from a production use standpoint on our end, I keep using this patch over and over again on all core upgrades I do.

I resubmitted @mikeytown2 patches because of the new drupal.org option to select which version of core to test against. I specified 7x-1.x core for the D7 patches and 8.1.x for the D8 patch

no changes to the patches,

*** EDIT *** patch #6 is for Drupal 6, ***EDIT***

we've always used patch #3

I checked the D8 core code, has changed radically since the earlier patch, looks like D8 does not have this performance issue and if it did it'd be a new issue.

WAITING FOR PATCH 3 to finish tests....

Ok, PATCH 3 is still good, we keep using it. PATCH 3 is for Drupal 7, patch 6 is for Drupal 6 so I hid it as it won't apply to D7. Drupal 8 doesn't need this.

***EDIT*** whats up with the testbot ? Unreliable, it passes one time, 2 hours later same code same core, fails. Test again, then it passes? flakey jenkins/testbot. ***EDIT***

reran php 5.3 test, works this time. ALL TESTS ARE GREEN

I think this may pose a BC break -- can we address #25?

hidden, in active trail and administer menu are checked, this explains why patch 3 passes all tests.

From a security standpoint I don't want hidden menu links to be loaded and translated for those that don't have administrate menu access.

The only use case I can think of where you might want to allow this is if we want to switch menu language without another bootstrap for an authenticated user that does not have administrate menu access. The other is a migration but migrate migrations are always done by an admin type account that has 'administrate menu' access.

From a performance standpoint it is a bonus reason for this change. Less calls to the database, less processing.

From a security standpoint this works. Passes all known tests and works with contrib .

I understand the reluctance and fear however after several years it's not even one report about a fringe case of failure. This patch is in use by keeners like @mikeytown2. I will continue to use this patch until it's included in core. Life on the wild side is faster.

***EDIT***
Maybe a new api function could be added, where all menu links are processed regardless of permissions including hidden menu items, it could be called "_menu_tree_bypass_access" a change record could be added to add this in. However I don't think this function is necessary as someone can just add the 'administer menu' permission and achieve the same result. So to save time, keep the code base tighter and more secure lets >not< create this new api function ***EDIT***

We don't do BC breaking API changes in Drupal 7 anymore, so rather than change the existing behavior and adding a parameter/function for excluding hidden menu items I'd rather do the opposite (keep the existing behavior for menu_build_tree, add a parameter $skip_hidden that defaults to false).

@stefan.r , sounds like your idea could work well, so you're saying the caller of that in core (where the performance is to be gained) might/could/should/would/ have the extra param $skip_hidden as true and thus not affecting the API as the updated menu_build_tree would consider this extra param .

Yes, that sounds fine -- with contrib being able to use that flag as well.

try new patch based on #45, see interdiff

Fixed it, I am expecting this to pass.

This interdiff shows difference between 45 and 51 (ignore my other interdiffs)

try this

ok, patch 55 (from comment #54) passes, adding the $skip_hidden flag, backwards compatible change.

This is how I interpreted the recommendation made by @stefan.r

I still prefer patch #3 though, but just for giggles I followed the low-risk backwards compatible option layed out by @stefan.r that will basically allow contrib to take advantage of the $skip_hidden.

Patch #3 however will give you an immediate performance improvement. Patch #55 gives you the 'opportunity' to get you what patch #3 does but from a 'contrib' perspective but does not actually skip hidden links unless called via contrib using core api with $skip_hidden as true from the caller. In Patch #55 $skip_hidden is false by default, meaning it actually does completely process hidden menu items right through to render, as it did before without patching.

Until this has a resolution here is a reroll of #3 based on 7.52 core.

FWIW: This:

+    // Do not load hidden menu items if not in active breadcrumb trail and
+    // user can't administer the menu.
+    if (!empty($item['hidden']) && empty($item['in_active_trail']) && !user_access('administer menu')) {
+      continue;
+    }

Means, if somebody without "administer menu" permission disables a menu item (s)he can't enable it again, as after disabling, the item will disapear.