Steps to reproduce, install toolbar.module, stick a debug_backtrace() in system_admin_menu_block(), watch the entirety of /admin being built on every page.

I think the proper fix for this is just to use a custom menu for the main toolbar links - this will be needed by the latest proposal at #508458: Config and modules page anyway.

CommentFileSizeAuthor
#12 menu.png344.63 KBcatch
#6 remove_access.patch3.92 KBcatch
#3 _menu_link_translate.png317.91 KBcatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jrchamp’s picture

jrchamp CreditAttribution: jrchamp commented

subscribing

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

So there has to be around this, but in the meantime I found some bugs in system_admin_menu_block() which were causing duplicate rendering and queries: #521798: system_admin_menu_block() static caching is broken .

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
FileSize
_menu_link_translate.png317.91 KB

This is taking up approximately 30% of page execution time at the moment.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Title: toolbar_get_menu_tree() causes system_admin_menu_block() to be called on every page » system_admin_menu_block_access() is very, very, slow
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

admin_menu, with client-side caching being disabled, suffers from this, too.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Active » Needs review
FileSize
remove_access.patch3.92 KB

We should just roll this back. You might get a link which doesn't go anywhere, but you'll get there 35% faster.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Needs review

This is now a straight rollback of #296693: Restrict access to empty top level administration pages I may re-open that one at least temporarily since while the original bug is annoying, the fix is worse.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Active

uhm, no. Re-introducing MENU_DYNAMIC_ITEM would be an option, but reverting the code that ensures that admin categories are hidden if there are not any items below them does not do any good.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Title: system_admin_menu_block_access() is very, very, slow » system_admin_menu_block_access() makes no sense
Component: toolbar.module » system.module
Assigned: Unassigned » Damien Tournoud

... and I believe I know the guy that introduced that.

On it.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

possibly related #519046: Clean up toolbar menu code

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
FileSize
menu.png344.63 KB

webgrind screenshot, looks like Moshe's issue found some paths that rightly shouldn't be affected by this, but fixing this one would narrow that down a lot.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Assigned: Damien Tournoud » Unassigned

I'm not giving up hope of finding time to work on this, but I leave the floor open.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I've not been able to think of a good solution to this, but posted a patch over at #519046: Clean up toolbar menu code which fixes most of the symptoms without hopefully adding too much extra cruft.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

Another reason system_admin_menu_block_access() does not make total sense is that there might be underlying tabs in the IA on D7. #545952: D7UX IA: move modules to config/modules is not viable as it is because ATM there are no default enabled modules exposing configuration to the configuration tab (admin/config path), so system_admin_menu_block_access() decides people should not have access, even though there is a tab underneath, which has content (but that is not rendered by system_admin_menu_block()).

Of course once we have core config items moved there, this will not be visible anymore, but it is still an issue for cases when the default tab on the item does not have accessible content but there are subtabs otherwise which have content.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Active » Closed (duplicate)

#296693: Restrict access to empty top level administration pages was rolled back, and is still open for fixes, marking this as duplicate.