Empty admin categories are not hidden
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | menu system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | API change, D7 API clean-up, Performance, Release blocker |
Under Drupal 6(.4), even if a role has *no* User-Management related permissions, the "User Management" menu does not hide - it still shows, with no items under it. Selecting the menu takes the user to a page that simply states: "You do not have any administrative items."
It seems to me, that the menu simply should not be present.
To reproduce, using a brand-new D6.4 installation:
1) login as the site's superuser, and create a new role - name it anything you wish
2) In the 'permissions' page for the new role, make sure the role has *no* user management permissions - under "user module", deselect everything.
3) create a new user for the site, and give this user the newly created role
4) log out as the superuser, and login as the new user, with the new role
5) notice that under 'Administer', you still have "User Management" - clicking it will take you to that silly page that states that you have no administrative items...
This is a *very* annoying bug. I do *not* wish the "User Management" menu to be displayed for certain roles - ie., 'site editors', whose function is simply to add and edit content to the site.
#1
The same problem is also reproducible for the "Site Building" menu - ie., even if the role has no permissions for any of the menu items in the menu, the menu itself is still displayed. I suspect the same thing might happen also for the "Site Configuration" menu.
This *is* a serious problem to me, as one of the main tasks I have when setting up a Drupal site is to 'dumb down' the admin interface for newbie users. Not being able to easily hide these menus means that the interface ends up being overly complicated for low-level admin users - like site editors, who have no business managing users, or playing around with the site configuration.
Currently, the only way to overcome this bug is to split the Administer menu into several menus (one for each role needed), and use the BLOCK permissions for each menu block individually. In my case, for instance, I ended up having to split the menu into "RegisteredUser", "SiteEditor" and "SiteAdmin" menus. This is not a proper solution, it is a time-consuming, inelegant hack. I end up with 3 or 4 separate menus, containing totally related functions that had to be split in order for me to be able to control role access to them - when they really should be under just a single menu.
I do not need several different menus. I need just ONE menu, that properly restricts its items' visibility according to the user's permissions.
I hope this will be fixed for D7.
#2
That issue has been identified several months ago. Here is a first patch for it.
#3
Confirmed this problem. Would be nice to back-port fix to 6.x as well.
Steps to reproduce:
- Create a role called "editor" and an "editor" user assigned to that role.
- Give the role both "administer nodes" and "access administration pages" permissions.
- Log in as "editor" and go to "Administration"
You'll see entries for Site building, etc. in the navigation menu even though those result in "Access denied" when you try to go there. See attached screenshot.
Let's get tests with this patch though to make sure this bug doesn't recur.
#4
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_accessdoesn'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:
<?phpfunction system_admin_menu_block_page_access($path, $string) {
if (user_access($string)) {
$count = db_result(db_query("SELECT COUNT(mlid)
FROM {menu_links}
WHERE plid = (
SELECT mlid
FROM {menu_links}
WHERE link_path
LIKE '%s'
)", $path));
return ($count > 0);
}
return FALSE;
}
?>
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_pageso it would only seem nature thatsystem_admin_menu_block_page_accesswould be used for them.#5
@boomatower: Ok for the name change, it makes sense and I don't really care :)
The objective of using system_admin_menu_block() was to limit code duplication and database queries as most as possible. Your solution was not enough, because we also need to check for access of each menu item, not just count them.
I turned that issue around over and over, and came to the conclusion there is no better way.
#6
I forgot to add: "... but of course, I only want to be proven wrong!"
#7
Working on writing the test, and a few cleanup items along with it.
#8
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.
#9
Adds test and changes access callback name per #4 and #5.
@icouto: Depends on what this is considered. If this is considered a bug/or necessary feature then it most likely will be. It would be useful as SimpleTest 6.x-2.x would use this and I'm sure there are others, not to mention default use-cases as per #3.
The test needs assertLink and assertNoLink: #297894: Add assertLink and assertNoLink to SimpleTest
which requires: #297869: Add xpath method to Simpletest and refactor existing tests
Will need those to get in first, but we can still review this patch. (although you will need to apply those.)
After patches applies test passes.
#10
After applying the above patches and then applying this one tests pass.
#11
Ran entire test suite with all passes.
#12
Still applies after dbtng.
#13
There is a bug with the last patch :
1. Give anonymous user these permissions :
access administration pagesandadminister blocksoraccess site reports.2. Clear cache
3. Logout
4. Go to admin page >> Access denied, and no menu link
5. Refresh >> Notice error :
warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'system_admin_menu_block_page_access' was given in D:\Serveur\www\drupal\7.x\includes\menu.inc on line 502.
warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'system_admin_menu_block_page_access' was given in D:\Serveur\www\drupal\7.x\includes\menu.inc on line 502.
#14
Hmmmm...two interesting things:
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:
error PHP Fatal error: require_once(): Failed opening required './includes/database/mysql/query.inc' (include_path='.:/usr/share/php5:/usr/share/php5/PEAR') in .../drupal-7/includes/bootstrap.inc on line 1399, referer: http://drupal-7.dev.loc/#15
There are three bugs here:
* Drupal don't use absolute paths for loading include files, and the current path can change, especially during the execution of shutdown functions. This is solved in #259623: Broken autoloader: convert includes/requires to use absolute paths. Please try that patch.
* Access control on /admin don't work properly, probably because system_admin_menu_block_page_access() recursively calls itself. For now, I disabled that access control on /admin.
* Hide descriptions/Show descriptions toggles didn't work for anonymous users (it was relying on a $user parameter). I changed this to use session for anonymous users.
#16
Not to be overly picky on code comments, but:
+ * Ensure that menu items that without "visible" children are hidden.An extra "that" or something?
+ * The path of the menu item to ensure has children.Something's not quite right there.
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.
#17
The last submitted patch failed testing.
#18
Subscribing and marking for later review. Administration menu module users are suffering from this.
#19
Here is an updated patch which applies against HEAD and fixes the comments in #16. It's working as expected for me.
#20
- 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.
#21
This is a annoying bug - better title.
#22
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.
<?php
}
}
+class AdminMenuBlockTestCase extends DrupalWebTestCase {
?>
^^ Missing PHPDoc above the class.
<?php+ /**
+ * Implementation of getInfo().
+ */
+ function getInfo() {
?>
^^ We don't PHPDoc overriden functions anymore.
<?php+ return array(
+ 'name' => t('Admin menu categories'),
+ 'description' => t('Confirm that administrative categories, which do not contain any visible child items, are not displayed.'),
+ 'group' => t('System'),
+ );
+ }
?>
<?php+
+ /**
+ * Ensure that administrative menu items without visible children are hidden.
+ */
+ public function testEmptyHide() {
?>
^^ 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 :)
#23
After a BIG support session with Damien in IRC, this should (hopefully) be properly. :) (Thanks again!)
#24
FYI: Damien moved the compact mode fixes to #352734: "Compact mode" switch doesn't work for anonymous users...
#25
The last submitted patch failed testing.
#26
#27
The last submitted patch failed testing.
#28
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.
<?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;
}
?>
#29
The attached patch reintroduces MENU_ITEM_GROUPING as a menu item type. Such menu items automatically get a page callback that lists all the item's children, or redirects the user if there is only one child.
See this patch in action at /node/add. See anything different? No? Exactly!
By the way: node_add_page() and similar obsolete functions have not yet been removed to keep this initial patch simple.
#30
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:
<?php
/**
* Determine if a user should have access to a menu item.
*
* Users should only have access if all of the following cases are true:
* - The user has access to at least one of this item's children.
* - The custom access callback (if any) returns TRUE.
*
* @param $path string
* The path of the menu item to check access to.
* @param $callback string
* A custom access callback to check as well.
* @param $arguments array
* Arguments to pass on to the access callback.
*
* @return boolean
*/
function menu_item_grouping_access($path, $callback, $arguments) {
if (is_string($callback)) {
$custom_access = call_user_func_array($callback, $arguments);
}
$item = array('path' => $path);
$items = system_admin_menu_block($item);
foreach ($items as $child) {
_menu_check_access($child, array());
if ($child['access']) {
if ($custom_access) {
return TRUE;
}
else {
return FALSE;
}
}
}
return FALSE;
}
?>
#31
Benchmarks?
#32
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.
#33
subscribe
#34
Suscribe using Drupal 6.10.
Will drupal fix this to 6 version or will it be a solution for Drupal 7?
#35
Subscribing too, if a Drupal 6 backport is on the cards.
#36
Subscribing.
#37
Subscribing and looking forward for d6 patch
#38
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?
#39
Rework of #23 (no longer applied and outdated).
drupal_static().admin/development.admin/development.#40
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?
#41
Actually this also change also needs to be implemented for the new top level admin menu
admin/internationalin locale.module#42
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.
#43
I think it's good to go as is, just to summarise what this patch does:
system_admin_menu_block_pageand gives it the params: path (e.g. 'admin/build') and the access permission.#44
Yes. 'admin' & 'access administration pages' we need to tackle elsewhere.
#45
Kick ass! I'm so happy to finally see this fixed! :D
Committed to HEAD! Marking "needs work" until it's documented in the 6.x => 7.x upgrade page.
Ultimately this is up to Gábor, of course, but given what was required to fix this in 7.x, I'm pretty sure we can't backport this to 6.x. It's an API change that requires module developers to make changes to their menu items' access callbacks. In addition to this being a mean thing to ask people 1.5 years after D6's initial release, it will result in hackish "if function_exists" workarounds in order for this to work for people not using the latest versions of Drupal 6. Granted, it'd only affect the handful of modules that define their own top-level admin blocks like OG, Panels, and Project. But still. Stable means stable.
#46
Documentation added: http://drupal.org/node/224333#system_admin_menu_block_access
#47
The last submitted patch failed testing.
#48
Guess I'll just mark fixed unless anyone says otherwise.
#49
Followup patch: #475348: Move admin/development menu category into system.module
#50
Automatically closed -- issue fixed for 2 weeks with no activity.
#51
should any fix be expected for drupal 6? In a different issue maybe?
#52
+1 for 6.x backport
#53
Drupal 6.x branch issue: #461700: Get rid of menu groups which are empty in Administer
#54
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.
#55
#56
First I forget the patch, then I forget the status change. Sorry for the bumps :(
#57
Possible alternative to the current implementation -have a convention of 'access structure administration page' 'access configuration and modules administration page' for these top level links - then you can give the specific top level links to users as necessary and while you might need to update them manually it'll be a normal access check instead of access checking and rendering /admin on every page load. Could probably replace 'access administration pages' with this since we don't have a single centralised admin page any more. Most sites are going to be using either the core toolbar or admin_menu.module - both of which suffer from this (apart from admin_menu implementing client side caching), and both of which reduce hits on /admin anyway.
#58
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.
#59
@sun, yes that's right. Except the new IA in combination with the core toolbar means by default you'd never be displaying expanded links to the categories, just the top level link, so the original issue here is slightly mollified if we add a blanket on/off permission for those top level links.
The original issue (and the performance implications in HEAD) are then primarily going to affect core menu module and admin_menu. i know admin_menu has client-side caching, but menu module isn't and won't.
#60
The last submitted patch failed testing.
#61
Documentation update:
Documentation was added in #46, but the issue is still marked as, "Needs documentation." I've given the documentation at http://drupal.org/update/modules/6/7#system_admin_menu_block_access a quick edit to improve clarity. For now I'm going to remove the "needs documentation" tag. If/when the issue does get rolled back, please re-mark as "needs documentation." Thanks!
#62
Re-rolled.
#63
The last submitted patch failed testing.
#64
locale.module changed too.
#65
catch, you mention a performance problem but you didn't provide any details. Care to shed some light on this? How bad is it?
#66
@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.
#67
OK so currently the admin/config link just uses 'access administration pages' - but for that to show up even when empty, it really ought to use system_admin_menu_block_access(). So to show just how bad things get, here's a screenshot of the front page with devel query log output turned on - note the 5x2 queries for system_admin_menu_block() - that's 10 out of 43.
#68
The last submitted patch failed testing.
#69
Posted #557806: Cache the toolbar per-user which would fix the immediate issue in HEAD without showing dead-end links, and has additional stats on performance issues (approx. 20% of page execution time, and 30% of database queries on a newly installed Drupal).
#70
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:
Before patch:
Executed 43 queries in 26.31 milliseconds. Page execution time was 93.31 ms.
Executed 43 queries in 29.88 milliseconds. Page execution time was 97.41 ms.
Executed 43 queries in 24.89 milliseconds. Page execution time was 84.37 ms.
After patch:
Executed 30 queries in 16.55 milliseconds. Page execution time was 71.57 ms.
Executed 30 queries in 16.96 milliseconds. Page execution time was 77.55 ms.
Executed 30 queries in 18.57 milliseconds. Page execution time was 75.16 ms.
That's just under 1/3rd of queries, 2/5 of query execution time, and around 15-20% of page execution time on a light non-admin page taken by toolbar menu rendering.
Re-rolled the patch.
#71
The last submitted patch failed testing.
#72
Can't reproduce those exceptions locally, either via the UI, or via the command line. Cool that there's 1024 though.
#73
Re-rolled.
#74
The menu system is totally screwed up in HEAD anyway, so having one regression more or less doesn't count.
#75
Well yeah, it's basically a choice of which kind of regression we want - links to empty categories, or a 25% increase in database queries and quite a bit of extra time spent in PHP.
Note we /can/ fix performacne this in toolbar.module with caching, but it's going to take a lot of special-casing and workarounds, and won't work for menu.module blocks which may contain admin links, so I'd like to see this committed or won't fixed before putting any time into that.
#76
Rolled back. Let's figure out a proper solution.
#77
Let's start again then:
One idea I had, but need to look at feasibility - system_admin_menu_block_access() rendered the entire block of links with all their menu items to check access, it seems like we could bail out before that. Something like this:
Fetch the child links (still one or two queries per set of links).
If any of the child links have the same access callback and access arguments return TRUE (is this information available easily though?).
If not, return as soon as any of the access callbacks has returned TRUE to save checking all of them. I don't think either of these would be a lot cheaper though.
#78
ok. Recent discussion and back and forth on #591682: Categories below "Configuration and modules" clarified:
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.
#79
Subscribe.
Yeah, I don't understand how removing the meta categories below admin/config/* fixes this problem at all... The biggest part of this problem for D7 seems to have to do with the top level of the management menu (toolbar)... all of whose items are nicely displayed at the top of your screen, even if you don't have access to any of the items underneath some of them, right?
#80
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.
#81
Frankly, I don't really understand why this happened on every page. I can understand that the rendering function menu_tree_output() is invoked on every page, but I don't really grok why we need to rebuild the menu tree and therefore invoke menu_tree_check_access() on every page?
#82
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.
#83
We are not going to fix those awkward system_admin_menu* functions in here. Those are total hacks, entirely disregarding menu tree functionality. Those can be fixed in #618530: System module's "listing" pages (and blocks) should use menu_build_tree(), but not here.
This patch makes all functionality that properly uses menu tree data functions correctly output those grouping items and meta categories - which includes the "Management" menu block.
Hence, if you want to test this patch, then change your admin theme to Garland, so you see the Management menu block on all admin/* pages.
#84
critical means Drupal doesn't work or is insecure
#85
ok, sun very patiently explained this issue to me in IRC, and now I understand why it's a bug and needs to be approached at this level of the API:
#86
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).
#87
And. Yeah. This patch is, again, an insane attempt to fix the nightmares of the menu system.
The patch fixes the actual issue, but it reveals a total flaw in a part of the menu system unrelated to this patch. Somone introduced a $max_depth parameter to menu tree data building functions without thinking about the consequence that with this forced tree limitation, the entire 'has_children' property is fucked up. As of now, no implementation is able to figure out whether a link really has no children or whether it just has been limited by the original caller. :(
#88
I forgot to mark as needs work, but then again, this patch including very simple tests will let the testbot do it for me.
#89
The last submitted patch failed testing.
#90
@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.
#91
#92
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().
#93
Can we please keep this at highly-highly critical, we will need this to have a good IA. Because the UX-Team already pointed out, right after we ended the streak of creating all the initial categories that we need Drupal to provide more good defaults. Not to only to keep consistency but primarily to provide direction, sorry - but IA is critical.
#94
#627080: [meta-issue] Additional categories admin/config Will be the issue, where we create additional categories.
#95
Doesn't the earlier patch not handle this? I'm not sure why the discussion of a new algorithm.
#96
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?
#97
I can't off the top of my head think of any pages under /admin which don't have user_access() as a menu callback, so that ought to satisfy enough cases to make it viable.
#98
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.
#99
subscribe…