Hide empty admin categories

That issue has been identified several months ago. Here is a first patch for it.

The code in #2 uses system_admin_menu_block() which does allot of extra stuff then is necessary to determine if there are children. Plus _system_settings_access doesn't seem like the right name for the access callback for two reasons.
1) It could be used by contrib, no need for private declaration (_).
2) It relates to other items besides settings.

In my attempt at this in #263616: Move SimpleTest out of "Site building" I used the following code:

Which alleviates the call to the other function.

Should think about the name of the function. The reasoning behind my choice is that the items that only display a list of children use system_admin_menu_block_page so it would only seem nature that system_admin_menu_block_page_access would be used for them.

I forgot to add: "... but of course, I only want to be proven wrong!"

Thank you, everyone, for your superhumanly quick response to this!

I was wondering, if we are going to see a back-port of the fix in D6, as mentioned by webchick.

After applying the above patches and then applying this one tests pass.

Still applies after dbtng.

Hmmmm...two interesting things:

• The notice only comes up once...at least for me.
• Adding debug code it becomes clear that user_access() returns TRUE, but the $content is empty.

It would seem the issue is in the existing code not the new code, but yet it works without the new code....

When I run the site as anonymous I get:

Not to be overly picky on code comments, but:

An extra "that" or something?

I see this is another good use of compact mode, which I wholeheartedly approve. We don't use setting enough.

These are minor, so I'll leave at CNR and you can take care of them during the next substantive re-roll.

Subscribing and marking for later review. Administration menu module users are suffering from this.

- Renamed menu item access callback to system_admin_menu_block_access(), since the additional "page" suggested that it would be used for determining access to a page, but we are just checking admin block/category access here.

- Optimized system_admin_menu_block_access() to check for the user permissions first.

- Removed all changes related to compact mode, as those are unrelated to this issue. (please create a new issue, since the changes make sense)

- Renamed test case.

Also ran tests, tested manually with a test user (and having admin menu module installed) and everything seems to working properly.

Changes in #20 make sense, but this is an old patch, and its test doesn't comply at all with our (relatively new) Test Writers Guidelines.

^^ Missing PHPDoc above the class.

^^ We don't PHPDoc overriden functions anymore.

^^ The PHPDoc description should probably better start with test*...

testEmptyHide() is a badly choosen function name.

As an end note: it's quite fun to shoot down one of your own old patches :)

FYI: Damien moved the compact mode fixes to #352734: "Compact mode" switch doesn't work for anonymous users...

While working at #362834: Move statistics out of the administration pages and add permissions I noticed that MENU_ITEM_GROUPING has not been added to the new menu system in D7. As a result, we now have several functions that do pretty much the same. node_add_page() and system_admin_menu_block_page(). I think node_add_page() is a good candidate for a new MENU_ITEM_GROUPING. It needs some changes, but then it will do fine (See the patch in #362834).

However, that patch is not complete yet, since it displays a MENU_ITEM_GROUPING parent item even if there are no child items or if the user has no permission to access any of its child items. This is the point where chx told me to check out this issue.

1. I believe the names system_admin_menu_block() and system_admin_menu_block_page() are improper. They don't describe what the functions do in a short and simple fashion, nor do they make the functionality appear available to non-admin pages, which should be.
2. I suggest I suggest putting MENU_ITEM_GROUPING back in and let menu.inc automatically set menu_item_grouping() (as seen below) as the page callback. Menu.inc should also set special access callback that returns FALSE if there are no children or if the user has no permission to access them.
3. The access callback as described in #4 only checks if there are no child items, but it should also check if the user has permission to access those children. This could perhaps be done the easiest by creating a user_access_multiple() (issue), so multiple permissions can be checked at once.
<?php
/**
* List a menu item's children.
*
* @return string
*/
function menu_item_grouping() {
  $item = menu_get_item();
  $items = system_admin_menu_block($item);
  // Bypass the listing if only one child is available.
  if (count($items) == 1) {
    $item = array_shift($items);
    drupal_goto($item['href']);
  }
  return theme('menu_item_grouping', $items);
}

/**
* Theme a list of menu items.
*
* @param $items
*   The items as returned from system_admin_menu_block().
*
* @return string
*/
function theme_menu_item_grouping($items) {
  $output = '';

  if ($items) {
    $output = '<dl class="menu-item-grouping">';
    foreach ($items as $item) {
      $output .= '<dt>' . l($item['title'], $item['href'], $item['localized_options']) . '</dt>';
      $output .= '<dd>' . filter_xss_admin($item['description']) . '</dd>';
    }
    $output .= '</dl>';
  }
  return $output;
}
?>

The attached patch also performs access control and should hide the item if a user has no access to child items. All issues pointed out in #29 are dealt with. I haven't yet tested the patch thoroughly, but it seems to work. Will do more testing as soon as I'm done with my other work here.

To do: display a message if there are no children to list.

The code responsible for the access check:

I have split the patch in two. After #363951: Reintroduction of MENU_ITEM_GROUPING gets submitted I'll post a new patch taking care of access control here. This will make benchmarking easier.

Suscribe using Drupal 6.10.
Will drupal fix this to 6 version or will it be a solution for Drupal 7?

Subscribing.

Since #363951: Reintroduction of MENU_ITEM_GROUPING won't be committed (marked as won't fix), does this mean we're back to the patch in #20 above?

Patch looks good and works quite well. However there's still one edge case that isn't covered - if the user has the 'access administration pages' permission, but doesn't have access to _any_ of the sub menus, and if the help module is not enabled, then the 'Administer' menu item still appears in the navigation block and the message "You do not have any administrative items." appears when clicked.

It's an edge case (why would anyone give that permission to a user without giving them access to at least one sub-menu?!) and the message displayed is useful, but shouldn't that menu item not appear at all?

I am not sure that is an issue since technically there is a menu item under "Administration". When help module is disabled the Administration menu item still displays. I attempted to add the check the the root "admin" item, but it causes an issue, possibly due to excessive looping.

Not sure we want to deal with that issue or perhaps save for follup patch. Either way this patch contains admin/international alteration.

Yes. 'admin' & 'access administration pages' we need to tackle elsewhere.

Documentation added: http://drupal.org/node/224333#system_admin_menu_block_access

Guess I'll just mark fixed unless anyone says otherwise.

Automatically closed -- issue fixed for 2 weeks with no activity.

+1 for 6.x backport

I'm asking for a rollback of this until the critical performance issues it introduced on every page load are resolved. fwiw Damien suggested this in irc as the original patch author.

See #520364: system_admin_menu_block_access() makes no sense and #519046: Clean up toolbar menu code for plenty of background on why this was such a bad idea.

First I forget the patch, then I forget the status change. Sorry for the bumps :(

As far as I get it, the new proposed IA for admin/config (which I do not support) would add categorization items below admin/config, which will face the very same problem: The parent item/link (category) must only be shown if the user has access to any child item. system_admin_menu_block_access() replaced the good ol' MENU_DYNAMIC_ITEM, which is what we are talking about here.

The last submitted patch failed testing.

Re-rolled.

locale.module changed too.

@Dries, I linked to two issues from #54, both of which have a fair bit of background.

Some of the performance issues were down to bad toolbar usage of menu_tree_all_data(), but system_admin_menu_block_access() is responsible for a lot more - the issue being that any link displayed which has this as it's access callback, requires the menu system to render the entire admin tree below it just to check if it's visible or not. The toolbar, an expanded management menu, or admin_menu all cause this to happen on every authenticated user page request.

#520364: system_admin_menu_block_access() makes no sense

#519046: Clean up toolbar menu code

Attached updated webgrind screenshots too.

The last submitted patch failed testing.

Marked the caching issue postponed on this - to make that work will require piling hack upon hack.

Here's the difference that caching made though:

Re-rolled the patch.

Can't reproduce those exceptions locally, either via the UI, or via the command line. Cool that there's 1024 though.

The menu system is totally screwed up in HEAD anyway, so having one regression more or less doesn't count.

Rolled back. Let's figure out a proper solution.

ok. Recent discussion and back and forth on #591682: Categories below "Configuration and modules" do not account for all possibilities clarified:

1. This is required for Drupal 7, as long as the meta categories below admin/config/* are not removed.
2. This is required to work on a different layer: the menu system - and not only for some special-cased top-level menu items.

Regarding 1: In general though, the Toolbar will now expose a "Reports" menu item, even if the user does not have access to sub-items (or the other way around, you can't expose a report item from a contrib module without granting the permission to access all site reports), so it's not really limited to admin/config/* categories.

Regarding 2: This means that we need to re-introduce MENU_DYNAMIC_ITEM.

No, there are two issues.

1. Top level items - that can be 'solved' by separate permissions for viewing reports or changing themes etc.. Harder to do for /admin/config though. This problem affects the toolbar.

2. sub-categories under those top level items. For menu.module or admin_menu.module this means it shows the 'locale', 'people' or 'development' categories in the menu tree, even if there are no accessible links after them, when that tree is expanded. The original patch fixed this, in a way which caused something like a 10-15% (or more) slowdown in HEAD, hence why I pushed for a rollback. The toolbar isn't affected by this, because it doesn't expose those links - however it created a massive performance regression by causing access callbacks to be called on all of those links on every page - in the region of about 20%.

if we were to remove the subcategories altogether, then we would only need to deal with the top level items in terms of access, and such a thing could be done a lot more performantly, albeit likely in a special cased and hacky way - since we'd only need to determine the visibility of 5-6 links, not the 10-20 subcategories too. If we want to leave the subcategories in, hide them when empty, and not further degrade performance, that's currently an unsolved problem.

Alrighty. After a quick discussion with smk-ka, we found a really smart way to overcome the entire issue.

This patch makes the Management menu block work correctly. (and therefore also admin_menu)

After further discussion in IRC, it seems like I was totally mistaken about the purpose of MENU_DYNAMIC_ITEM in D5, and this patch still uses that constant name, but I'll change that in the next patch - probably to MENU_GROUPING_ITEM or similar. Ping me in IRC if you have a better suggestion.

I'll also look into removing all the crappy workarounds from Toolbar and System module now.

critical means Drupal doesn't work or is insecure

pwolanin and me discussed the constant name, and having to decide between MENU_GROUPING_ITEM and MENU_CONTAINER_ITEM, we went for the latter.

And, yes, this patch would basically be back-portable to D6, because, although it introduces a new menu system type and constant, it doesn't really change the regular behavior of the original MENU_NORMAL_ITEM. The only modules and implementations that should be affected by this are menu rendering modules, and from all menu rendering modules I know, only Administration menu implemented a really awkward workaround that tried to fix the problem at least for the always visible top-level menu categories in D6 (i.e. content, build, settings, etc).

I forgot to mark as needs work, but then again, this patch including very simple tests will let the testbot do it for me.

@tha_sun - the max depth param isn't really used much yet - it should only be used in rare cases for optimization. I think we can account for it readily enough by not worrying about hiding the link if we are at the max depth.

The problem lives elsewhere and is even more fundamental than the $max_depth parameter. A screenie explains best:

There is absolutely no way to figure out whether this category/container link (Structure) has any children or any inaccessible children, because menu_tree_page_data() only loads the menu tree to the level of the link (Structure), but not anything below.

Hence, there is no way to determine whether this link should be displayed or not (because it doesn't contain or provide anything for the user) -- unless we would replace menu_tree_page_data() with menu_tree_all_data(), because the latter always builds the entire tree.

This is a fundamental problem in the menu tree building of menu_tree_page_data(), because currently, the "has_children" property of links is completely pointless. The property is TRUE even if a link has no children. My last patch contained a (commented out) fix for that in _menu_tree_data().
However, even when fixing that problem, then we still have absolutely no way to figure out whether a MENU_CONTAINER_ITEM link should be displayed, because the menu tree we have at hand does not contain any further children we could check.

The problem does not necessarily exist with menu_tree_all_data(), because that function generates the full menu tree, so we would have any possible children to determine whether a parent link should be displayed.

Brainstorming with smk-ka, we played with the idea of conditionally lazy-fetching any sub-links for MENU_CONTAINER_ITEMs when needed to determine access to it (i.e. grab all links using the mlid of the link as plid). However, those conditional queries would then run on all pages where a MENU_CONTAINER_ITEM link is visible in the tree (and has no children), which could be a performance penalty.
Another idea was to additionally fetch further children right within menu_tree_page_data() for all MENU_CONTAINER_ITEMs if not already contained in the tree result (so it would be cached with the tree) and pass that data on to menu_tree_data().

#627080: [meta-issue] Additional categories admin/config Will be the issue, where we create additional categories.

There is no generic solution for this "link container" problem, except by loading all the links for each user and each page (remember that the visibility of a menu link can depend both on the user and on the page, and can even depend on any other random variable outside of the control of the menu system).

We need to reduce the complexity of the problem somehow.

I suggest we only hide empty categories that satisfy the following condition: all the links below the category are:

* categories, or
* links related with a menu router whose access callback is 'user_access'

A category that doesn't satisfy those conditions will be always displayed.

With those restrictions, we can cache the visibility of the category per role. Does this seem acceptable?

I wonder whether we don't have a very very similar logic with 'expanded' in http://api.drupal.org/api/function/menu_tree_page_data/7 already, which basically does what I outlined as last resort in #92.

subscribing

Review of #591682-41: Categories below "Configuration and modules" do not account for all possibilities

I didn't think about the option to toggle visibility during menu router (link) building, that's interesting. I don't see the need for limiting this to parent/child access callback (mis)matches, but would rather consider to introduce a fragile and ugly hack that makes a parent item invisible in case no child is accessible -- only supporting non-dynamic children.

That would at least solve the concrete use-case and problem we have now.

EDIT: I mean, for non-dynamic children, invoke the access callbacks during menu router building. If there is a dynamic item below a MENU_GROUPING_ITEM, the parent cannot be hidden.

There are typos in this patch: groupping should only have 1 P in it.

not critical - this does not prevent Drupal from working, and is not a regression

Would love to test the patch that implements the above suggestion :)

subscribe

The last submitted patch, menu_group_item_nice_little_query.patch, failed testing.

This is a critical UX bug.

subscribe.
(coming from admin_menu)

Looking at current D7 - these links don't show up in side blocks typically - especially with the overlay.

So the main UX problem now is pages like admin and admin/config that may have empty blocks?

Partial fix. Is it right? Good enough?

Are the test fails real? Did this hide a link that should stay visible?

#120: 296693-hide-empty-admin-120.patch queued for re-testing.

Well there's two bugs here. In D6, you don't see empty admin categories in /admin - neither did you in D7 until a couple of months back, I'm not sure which issue caused that regression - that should be critical due to the regression if nothing else.

The other issue is that menu links display for the categories even if there's nothing in them, that's the same behaviour as D6, the fix which was committed here some months ago made logged in user performance suck due to effectively rendering the entirety of /admin on every page view just to see if there was any links to show there. There's a patch somewhere to at least hide completely empty categories without the crazy access check which seemed like a decent compromise to me, not sure where that is.

Either way, these are two different, though related bugs, and not fixing the latter, shouldn't hold up fixing the former. If that takes two different issues so be it.

The last submitted patch, 296693-hide-empty-admin-120.patch, failed testing.

@donquixote, if you re-read this issue, you'll see that some time last year, an access callback called system_admin_menu_block_access() was added, which checked for child items of the menu item to see if the top one would be rendered. I eventually posted a patch to revert this because it was a ridiculously high performance cost - #520364: system_admin_menu_block_access() makes no sense and some other issues have more background.

However in the meantime, there's also a regression somewhere causing the empty categories to show up on admin pages themselves, which pwolanin's patch addresses (but looks like those locale tests really are broken by it).

Well. At minimum, 42% of all Drupal sites/users will care. And, depending on module decisions, various larger Drupal distro/SaaS-providers will likely care, too. In addition, the UX team cares, too.

A quick fix (hack) for those page contents makes little sense to me. Instead, we need to fix the cause - possibly by remixing/combining all previous solution attempts into a new one.

#92 (sun):

Brainstorming with smk-ka, we played with the idea of conditionally lazy-fetching any sub-links for MENU_CONTAINER_ITEMs when needed to determine access to it (i.e. grab all links using the mlid of the link as plid). However, those conditional queries would then run on all pages where a MENU_CONTAINER_ITEM link is visible in the tree (and has no children), which could be a performance penalty.

Another idea was to additionally fetch further children right within menu_tree_page_data() for all MENU_CONTAINER_ITEMs if not already contained in the tree result (so it would be cached with the tree) and pass that data on to menu_tree_data().

I think that's the way to go.
In menu_tree_page_data(), we know which items hit the $max_depth level, so we can fetch further children only for those items that have MENU_DEPENDS_ON_CHILDREN and that hit the $max_depth limit. We only do this when all the items which fail the usual access check are removed, and we only fetch as many children as necessary to check if the item has any visible children.

Right now the menu_tree_check_access() happens after menu_tree_data() and menu_tree_collect_node_links(). Maybe this has to be rearranged a bit, to get the most out of it..

Then we can remove system_admin_menu_block_page_access(), because this special type of access restriction is dealt with in a different way.
Did I miss something? I guess so..

@pwolanin - that was this patch #551080: List non-container items on /admin for a complete overview which IMO needs to be rolled back before release because it's a significant step back from D6.

@pwolanin: I think we crossed wires a bit.

The menu links block and the menu admin UI - yes this is the correct behaviour as far as core is concerned. I can see arguments for hiding those links if there's no children at all, and would be OK with this if it's done in a performant way. I'm less keen on the "hide the link if the current user doesn't have access to any of the child links" since that only affects sites with multiple levels of administrators, and so far the fixes for it have caused major issues on any site which exposes admin links somewhere other than /admin - cure worse than the illness etc.

However, there's another set of 'blocks' - those which appear on /admin and admin/config, generated by system_admin_menu_block() - and in D6, and D7 until recently, these were hidden if there were no links to render. Hiding those blocks correct behaviour IMO and should be fixed as a regression. If #551080: List non-container items on /admin for a complete overview caused this regression that's just another reason to revert it.

I don't think it matches D6 behaviour at all, because sub-items of admin/config aren't shown there, and we have two huge ugly admin pages (/admin and /admin/config) instead of one.

MENU_CONTAINER_ITEM and MENU_DEPENDS_ON_CHILDREN both sound reasonable to me.

subscribing...

Since this has the API change tag (and Release blocker as well), reraising to "critical" to raise awareness.
If this is not in fact "critical", please either move this to Drupal 8 or, if this can be done without an API change, remove the corresponding tag.

Subscribing. This is still an issue in D6 that is negatively affecting the UX on my sites. #461700: Get rid of menu groups which are empty in Administer was marked as a duplicate of this, but does not seem to be addressed, not to mention this was reported as resolved in that ticket, however this is still an issue.

Here's how I use CSS to hide unnecessary admin menu sections for my editors who don't need to see Site Building, User Management, and so on. I'm using D6 to run a college website with multiple editors across campus.

1) Create a role for your editor. Let's have some fun with it and call it "editor".

2) In your theme's page.tpl.php file, add the user's role(s) to the body tag as classes:
<body class="<?php $userclasses = implode(" ",$user->roles); echo $userclasses; ?> >"

Obviously it's helpful if you don't have any spaces in your roles' names, but you can easily get around that with a little extra PHP (wasn't an issue for me since none of my editor roles had spaces).

3) Hide the menu items with CSS attribute selectors like so:

body.editor #admin-menu a[href="/admin/build"] { display:none; }

That's it!

subscribing

...title change then ;)

subscribing