Problem/Motivation

For users with limited permissions (i.e. content editors), many menu items irrelevant to the role still appear in the toolbar. This can be overwhelming so empty top level menu items should have restricted access and not be shown if the user lacks the permission for it.

This is a known problem where an unnecessarily complicated interface needs to be explained/simplified to newer users.

Steps to reproduce

To reproduce, using a fresh installation:

  1. login as the site's superuser, and create a new role - name it anything you wish
  2. On the 'permissions' page for the new role, grant access to 'Use the administration pages and help', 'View the administration theme' and 'Use the administration toolbar'
  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 'Manage', you still have "Structure" - clicking it will take you to that page that states that you have no administrative items.

Proposed resolution

The current approach is to add a route parameter to several routes. Then add an access check for routes that have the new parameter. The access check loads the corresponding menu link and its children. If the user does not have access to any of the children, then deny access to the page. If any of the children have the new route parameter, then recursively check them.

Remaining tasks


Tests


Before and after screenshots. Please add them to the 'user interface changes' so they are easy to find.

Review

User interface changes

Content editor role
Before:

After:

CommentFileSizeAuthor
#284 Configuration.png18.74 KBsimon georges
#282 296693-10.1.x.patch35.54 KBtedbow
#267 Screen Shot 2023-08-16 at 6.03.01 PM.png206.78 KBhooroomoo
#267 Screen Shot 2023-08-16 at 6.03.40 PM.png276.39 KBhooroomoo
#255 296693-254.patch10 KBilya.no
#245 drupal-block-administration.png38.52 KBvacho
#244 access_calls_sorted_by_number.png108.94 KBilya.no
#244 calls_sorted_by_persentage.png169.83 KBilya.no
#243 interdiff-237-243.txt3.58 KBilya.no
#243 296693-243.patch12.15 KBilya.no
#241 after--patch--pic--296693.png7.61 KBvikashsoni
#241 before--patch--pic--296693.png19.51 KBvikashsoni
#238 before-test.png69.39 KBvacho
#238 after-test.png68.35 KBvacho
#238 before-test.png69.39 KBvacho
#237 interdiff-296693-235-237.txt1.1 KBjeroent
#237 296693-237.patch12.04 KBjeroent
#236 interdiff-296693-234-235.txt892 bytesjeroent
#236 296693-235.patch10.67 KBjeroent
#235 interdiff-296693-233-234.txt1.96 KBjeroent
#235 296693-234.patch10.19 KBjeroent
#234 interdiff-296693-231-233.txt9.65 KBjeroent
#234 296693-233.patch9.99 KBjeroent
#232 interdiff-296693-230-231.txt630 bytesjeroent
#232 296693-231.patch11.41 KBjeroent
#231 interdiff-296693-227-230.txt6.92 KBjeroent
#230 296693-230.patch11.58 KBjeroent
#227 interdiff-220-227.txt1.81 KBilya.no
#227 core-restrict-access-top-empty-admin-pages-296693-227.patch12.06 KBilya.no
#221 296693-before.png14.37 KBabhijith s
#221 296693-after.png11.06 KBabhijith s
#220 296693-220.patch11.89 KBkapilv
#219 interdiff_219.txt7.21 KBkapilv
#219 296693-219.patch6.77 KBkapilv
#218 296693-218-reroll-of-217-against-9.0.x.patch11.86 KBarshadkhan35
#217 296693-217.patch11.79 KBarshadkhan35
#216 296693-216.patch11.36 KBsaurabh-2k17
#213 interdiff_211-213.txt5.6 KBridhimaabrol24
#213 296693-213.patch11.42 KBridhimaabrol24
#211 296693-211.patch6.25 KBpaulocs
#211 interdiff-208-211.txt6.56 KBpaulocs
#208 interdiff_206-208.txt633 bytesanmolgoyal74
#208 296693-208.patch11.5 KBanmolgoyal74
#206 296693-206.patch11.51 KBpaulocs
#206 interdiff-203-206.txt647 bytespaulocs
#203 interdiff_198-203.txt2.1 KBridhimaabrol24
#203 296693-203.patch11.51 KBridhimaabrol24
#203 296693-203-test-only-patch.patch2.25 KBridhimaabrol24
#200 AfterPatch.png9.15 KBpaulocs
#200 BeforePatch.png12.47 KBpaulocs
#199 core-restrict-access-top-empty-admin-pages-296693-198.patch9.24 KBilya.no
#198 interdiff-195-198.txt1.11 KBilya.no
#195 interdiff-187-195.txt5.92 KBilya.no
#195 core-restrict-access-top-empty-admin-pages-296693-195.patch9.15 KBilya.no
#193 Screen Shot 2020-07-15 at 10.18.01 PM.png251.08 KBtanubansal
#187 core-restrict-access-top-empty-admin-pages-296693-187.patch5.58 KBilya.no
#2 296693-empty-admin-categories.patch5.25 KBdamien tournoud
#3 empty-admin-categories-no-patch.png70.59 KBwebchick
#9 296693-empty-admin-categories.patch7.19 KBboombatower
#15 296693-empty-admin-categories.patch6.69 KBdamien tournoud
#19 296693-empty-admin-categories.patch6.66 KBdeviantintegral
#20 drupal.hide-empty-admin-categories.patch6.03 KBsun
#23 drupal.hide-empty-admin-menus.patch6.01 KBsun
#29 menu_item_grouping_00.patch3.87 KBxano
#30 menu_item_grouping_01.patch5.16 KBxano
#39 296693-menu-hide.patch6.39 KBboombatower
#42 296693-menu-hide.patch7.14 KBboombatower
#55 system_block_access_revert.patch3.51 KBcatch
#62 system_block_access_revert.patch2.82 KBcatch
#64 system_block_access_revert.patch3.6 KBcatch
#66 patch.png407.28 KBcatch
#66 head.png438.94 KBcatch
#67 queries.png373.83 KBcatch
#70 revert.patch4.96 KBcatch
#73 revert.patch7.33 KBcatch
#82 drupal.empty-categories.82.patch10.85 KBsun
#83 drupal.empty-categories.83.patch13.47 KBsun
#86 drupal.empty-categories.86.patch13.5 KBsun
#88 drupal.empty-categories.88.patch17.11 KBsun
#92 drupal.empty-categories.92.patch17.35 KBsun
#92 empty-categories-page-tree-problem.png129.62 KBsun
#103 menu_group_item_nice_little_query.patch1.65 KBchx
#120 296693-hide-empty-admin-120.patch3.57 KBpwolanin

Issue fork drupal-296693

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

icouto’s picture

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.

damien tournoud’s picture

Title: Overly Persistent "User Management" Menu » Empty admin categories should be hidden
Version: 6.x-dev » 7.x-dev
Component: menu system » system.module
Status: Active » Needs review
StatusFileSize
new5.25 KB

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

webchick’s picture

Status: Needs review » Needs work
StatusFileSize
new70.59 KB

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.

boombatower’s picture

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:

function 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_page so it would only seem nature that system_admin_menu_block_page_access would be used for them.

damien tournoud’s picture

@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.

damien tournoud’s picture

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

boombatower’s picture

Assigned: Unassigned » boombatower

Working on writing the test, and a few cleanup items along with it.

icouto’s picture

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.

boombatower’s picture

Status: Needs work » Needs review
StatusFileSize
new7.19 KB

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.

boombatower’s picture

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

boombatower’s picture

Ran entire test suite with all passes.

boombatower’s picture

Still applies after dbtng.

lilou’s picture

Status: Needs review » Needs work

There is a bug with the last patch :

1. Give anonymous user these permissions :

access administration pages and administer blocks or access 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.
boombatower’s picture

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:

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/
damien tournoud’s picture

Status: Needs work » Needs review
StatusFileSize
new6.69 KB

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.

keith.smith’s picture

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.

Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

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

deviantintegral’s picture

StatusFileSize
new6.66 KB

Here is an updated patch which applies against HEAD and fixes the comments in #16. It's working as expected for me.

sun’s picture

Assigned: boombatower » Unassigned
Status: Needs work » Needs review
StatusFileSize
new6.03 KB

- 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.

sun’s picture

Title: Empty admin categories should be hidden » Empty admin categories are not hidden

This is a annoying bug - better title.

damien tournoud’s picture

Status: Needs review » Needs work

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.

   }
 }
 
+class AdminMenuBlockTestCase extends DrupalWebTestCase {

^^ Missing PHPDoc above the class.

+  /**
+   * Implementation of getInfo().
+   */
+  function getInfo() {

^^ We don't PHPDoc overriden functions anymore.

+    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'),
+    );
+  }
+
+  /**
+   * 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 :)

sun’s picture

Status: Needs work » Needs review
StatusFileSize
new6.01 KB

After a BIG support session with Damien in IRC, this should (hopefully) be properly. :) (Thanks again!)

sun’s picture

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

Status: Needs review » Needs work

The last submitted patch failed testing.

damien tournoud’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

xano’s picture

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.
  4. /**
     * 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;
    }
    
xano’s picture

Title: Empty admin categories are not hidden » MENU_ITEM_GROUPING
Component: system.module » menu system
Assigned: Unassigned » xano
Status: Needs work » Needs review
StatusFileSize
new3.87 KB

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.

  1. To do: Hide the menu item if there are no children. I must say I'm not sure if this is the way to go, since it could cause some links not to be visible anywhere, although they have been enabled at admin/build/menu.
  2. To do: Check if the user has permission to access any children, otherwise hide the item.
  3. Custom access callbacks should be used in parallel with the access callbacks from points 1 and 2.

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.

xano’s picture

StatusFileSize
new5.16 KB

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:

/**
 * 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;
}
chx’s picture

Benchmarks?

xano’s picture

Title: MENU_ITEM_GROUPING » Empty admin categories are not hidden
Assigned: xano » Unassigned
Status: Needs review » Active

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.

stella’s picture

Title: Empty admin categories are not hidden » Empty admin categories are not hidden

subscribe

mairav’s picture

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

mrfelton’s picture

Subscribing too, if a Drupal 6 backport is on the cards.

nwe_44’s picture

Subscribing.

pitxels’s picture

Subscribing and looking forward for d6 patch

stella’s picture

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?

boombatower’s picture

Status: Active » Needs review
StatusFileSize
new6.39 KB

Rework of #23 (no longer applied and outdated).

  • Make use of drupal_static().
  • Implement callback for admin/development.
  • Updated test for admin/development.
stella’s picture

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?

stella’s picture

Status: Needs review » Needs work

Actually this also change also needs to be implemented for the new top level admin menu admin/international in locale.module

boombatower’s picture

Status: Needs work » Needs review
StatusFileSize
new7.14 KB

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.

stella’s picture

I think it's good to go as is, just to summarise what this patch does:

  • For each of the top level admin menu items (admin/build, admin/user, etc), it changes the access callback to be a new function system_admin_menu_block_page and gives it the params: path (e.g. 'admin/build') and the access permission.
  • The function first checks that the user has the appropriate access, and if not returns FALSE.
  • then checks if the user has access to any of the sub-menus, and if not it returns FALSE, which prevents the user from seeing the menu item
sun’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

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.

boombatower’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

boombatower’s picture

Status: Needs work » Fixed

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

boombatower’s picture

Status: Fixed » Closed (fixed)

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

pitxels’s picture

should any fix be expected for drupal 6? In a different issue maybe?

kenorb’s picture

+1 for 6.x backport

kenorb’s picture

catch’s picture

Priority: Normal » Critical
Status: Closed (fixed) » Active

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.

catch’s picture

StatusFileSize
new3.51 KB
catch’s picture

Status: Active » Needs review

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

catch’s picture

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.

sun’s picture

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.

catch’s picture

@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.

Status: Needs review » Needs work

The last submitted patch failed testing.

emmajane’s picture

Issue tags: -Needs documentation

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!

catch’s picture

Status: Needs work » Needs review
Issue tags: +Performance
StatusFileSize
new2.82 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Title: Empty admin categories are not hidden » Revert system_admin_menu_block_access()
Status: Needs work » Needs review
StatusFileSize
new3.6 KB

locale.module changed too.

dries’s picture

catch, you mention a performance problem but you didn't provide any details. Care to shed some light on this? How bad is it?

catch’s picture

StatusFileSize
new407.28 KB
new438.94 KB

@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.

catch’s picture

StatusFileSize
new373.83 KB

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

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).

catch’s picture

Status: Needs work » Needs review
Issue tags: +API change
StatusFileSize
new4.96 KB

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

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

catch’s picture

Status: Needs work » Needs review
StatusFileSize
new7.33 KB

Re-rolled.

sun’s picture

Status: Needs review » Reviewed & tested by the community

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

catch’s picture

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.

dries’s picture

Status: Reviewed & tested by the community » Fixed

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

catch’s picture

Title: Revert system_admin_menu_block_access() » Empty admin categories are not hidden
Status: Fixed » Active

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.

sun’s picture

Issue tags: +Release blocker, +D7 API clean-up

ok. Recent discussion and back and forth on #591682: Config pages are effectively hardcoded to some subcategory of 'admin/config' 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.

David_Rothstein’s picture

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?

catch’s picture

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.

sun’s picture

performance regression by causing access callbacks to be called on all of those links on every page

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?

sun’s picture

Status: Active » Needs review
StatusFileSize
new10.85 KB

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.

sun’s picture

StatusFileSize
new13.47 KB

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.

pwolanin’s picture

Priority: Critical » Normal

critical means Drupal doesn't work or is insecure

pwolanin’s picture

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:

  • this issue is about how we display menu links, not about actual access to paths.
  • the algorithm for determining access to a link does not check any child links if a parent link is denied access
  • the above fact makes it very difficult to sensibly use an access callback to hide a link with no visible children
  • the proposed change should not break any existing D7 (or D6) code if it's done right
sun’s picture

Priority: Normal » Critical
StatusFileSize
new13.5 KB

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).

sun’s picture

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. :(

sun’s picture

StatusFileSize
new17.11 KB

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

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

Priority: Critical » Normal
Status: Needs work » Needs review

@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.

pwolanin’s picture

Status: Needs review » Needs work
sun’s picture

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

empty-categories-page-tree-problem.png

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().

Bojhan’s picture

Priority: Normal » Critical

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.

Bojhan’s picture

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

pwolanin’s picture

Doesn't the earlier patch not handle this? I'm not sure why the discussion of a new algorithm.

damien tournoud’s picture

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?

catch’s picture

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.

sun’s picture

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.

yoroy’s picture

subscribe…

dyke it’s picture

subscribing

chx’s picture

http://drupal.org/node/591682#comment-2425632 has a solution. ick but solution.

sun’s picture

Review of #591682-41: Config pages are effectively hardcoded to some subcategory of 'admin/config'

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.

chx’s picture

StatusFileSize
new1.65 KB

during menu router (link) ?! Yes. Good idea. Let me feel the links rebuild ... striking at the evil core ... this day will last forever... (i should listen to less Judas Priest but something must keep me awake at this hour).

jhodgdon’s picture

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

chx’s picture

Sure there are. It needs comments and tests too. Others can work on those...

pwolanin’s picture

Priority: Critical » Normal

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

pwolanin’s picture

catch pointed out related issues with background into in #54

Discussing this in IRC yesterday, with chx, Bojhan, and webchick, this was what I took away:

  1. This is not a release blocker
  2. Some fix to this is still a really high priority
  3. Previous fixes (as discussed in #54) that were complete had too high a performance hit.
  4. The SQL in chx' last patch ins only correct in some cases.

In terms of the menu links part of this, I suggest we proceed with a logic similar to what chx porposed, but simplify the conditions so we only hide these special links if they have zero children. This should solve the proble for user 1 or any admin with full permission, though some junior admin with partial permissions may still potentially see empty categories. I think that's easy enough and would be an improvement over the current situation, so we should do that while anyone still otivated can work on a complete fix for 7.x (or 8.x).

yoroy’s picture

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

David_Rothstein’s picture

In terms of the menu links part of this, I suggest we proceed with a logic similar to what chx porposed, but simplify the conditions so we only hide these special links if they have zero children. This should solve the proble for user 1 or any admin with full permission, though some junior admin with partial permissions may still potentially see empty categories.

I had a patch months ago (see #591682-27: Config pages are effectively hardcoded to some subcategory of 'admin/config') that did this but at the time people did not consider it a complete solution.

Mine hid the links on output, compared to @chx's which does so on menu rebuild. I think @chx's approach is probably cleaner if it works correctly. But if we need to try a different approach, there's a starting point for that too :)

Status: Needs work » Needs review
Issue tags: -Performance, -Release blocker, -API change, -D7 API clean-up

Re-test of 296693-empty-admin-categories.patch from comment #2 was requested by bakr.

Status: Needs review » Needs work
Issue tags: +Performance, +Release blocker, +API change, +D7 API clean-up

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

darthf1’s picture

subscribing

Bojhan’s picture

Priority: Normal » Critical

This is a critical UX bug.

flokli’s picture

subscribing

donquixote’s picture

subscribe.
(coming from admin_menu)

pwolanin’s picture

Priority: Critical » Normal

not critical

pwolanin’s picture

Priority: Normal » Critical

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?

pwolanin’s picture

http://api.drupal.org/api/function/theme_admin_page/7 doesn't respect $block['show'].

That's certainly part of the problem.

pwolanin’s picture

Status: Needs work » Needs review
StatusFileSize
new3.57 KB

Partial fix. Is it right? Good enough?

Status: Needs review » Needs work

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

pwolanin’s picture

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

jhodgdon’s picture

The failed tests are all in language negotiation, and don't look related to this patch? http://qa.drupal.org/pifr/test/42318
I'll request a retest.

jhodgdon’s picture

Status: Needs work » Needs review

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

sun’s picture

Status: Needs review » Needs work

I'm not sure why you are saying that this only affects the admin/ and admin/config and similar pages.

We can turn the following in a test, and it will fail:

* Create a new user with permissions

- access administration pages

- access dashboard (required, since Dashboard contains a hack to replace access to admin/ [WTF])

* Log in that user, and go to admin/

* Neither "Structure" nor "Configuration" should be displayed in the Management menu block.

* Go to admin/config... none of the sub-categories should be displayed in the Management menu block.

Note that I mean the Management menu block. Seeing the empty categories in the page content is a derivative bug, not the cause.

catch’s picture

Status: Needs work » Needs review

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.

catch’s picture

Issue tags: -Performance, -Release blocker, -API change, -D7 API clean-up

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

Status: Needs review » Needs work
Issue tags: +Performance, +Release blocker, +API change, +D7 API clean-up

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

donquixote’s picture

Looking at system_menu() in D7:

  $items['admin/structure'] = array(
    ...
    'access arguments' => array('access administration pages'),
  );

How is it surprising that it does show up for users which have this permission?
If we don't want it to show up, we need a custom access callback for admin/structure.
A menu item is not hidden only because it has no children - that would be illogical to expect.
What did I miss?

catch’s picture

@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).

pwolanin’s picture

@sun - I frankly don't care if links to empty page show up in the block - I think it's much less important, and I'd argue as to whether it's a bug or not.

sun’s picture

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.

catch’s picture

That's not a hack for the page contents - the blocks shouldn't show up if they're empty.

In fact, if you put system_admin_block_access() back into core with that current bug, you'd still get the child items, because those empty blocks get rendered, and that's what it checks - the actual rendered output of the block (or the renderable array now most likely).

So we at least need a baseline of D6 non-regressed functionality before trying to do a major rework of this (which is what it's going to take to fix it IMO unless someone manages to come up with a performant solution which solves the issue for everyone within the current limitations - which none of us have yet).

donquixote’s picture

#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’s picture

note the blocks are not built using just menu link data - some dirty hacks are in there to sometimes pull in local tasks too (why?), so I am not at all confident that we can unhack this mess cleanly for D7.

I continue to not understand why having links in the block that lead potentially to an empty page is really a big an UX/UI issue.

catch’s picture

@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’s picture

catch - sorry, let me clarify the above. I think have all links in the menu links block (and in the menu admin UI) is the correct behavior. If they are not, how can you find it to add items to it?

In Drupal 6 you can put any link under the /admin link to create a new category - I consider that a useful feature. So it actually sounds like #551080 made Drupal 7 match the Drupal 6 behavior?

catch’s picture

@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.

sun’s picture

I at least need the agreed on menu type constants from the patch in #92 to work around this very annoying UX problem in admin_menu.

catch’s picture

MENU_CONTAINER_ITEM and MENU_DEPENDS_ON_CHILDREN both sound reasonable to me.

boombatower’s picture

From a naming perspective

'type' => MENU_CONTAINER_ITEM
'type' => MENU_DEPENDS_ON_CHILDREN

The first is a much better "type" then "depends on children" which seems more like an additional descriptive attribute.

+1 for container item to solve this.

catch’s picture

Priority: Critical » Normal

Would still be nice to fix the menu link issue, at least for completely empty categories as opposed to ones with no access, but since there's never been a viable patch on this issue, and the D6 regression just got fixed in #805124: admin/config shows empty admin blocks I'm downgrading this from critical.

tstoeckler’s picture

Priority: Normal » Critical

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.

catch’s picture

Version: 7.x-dev » 8.x-dev
Priority: Critical » Major
Issue tags: -Release blocker, -D7 API clean-up

Moving this to D8, there's no regression from D6 here, there has never been a viable patch that doesn't make something else worse. It would be feasible to do the crazy access callback stuff in a contrib module if people really, really want this over and above the performance of their site in Drupal 7.

tstackhouse’s picture

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.

jhodgdon’s picture

If you read the entirety of that issue, you'll see it was reported as resolved and then later reported as reverted. If you read all 148 comments here, you'll find out that the reason it was reverted is that it caused a huge performance problem (at least I think so from comment #145 above). So we don't currently have a fix, and the issue has been moved off to Drupal 8 for consideration of a good fix.

math-hew’s picture

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!

sun’s picture

Category: bug » task

Extremely annoying, but only borderline a bug.

klonos’s picture

Title: Empty admin categories are not hidden » Hide empty admin categories

...title change then ;)

tim.plunkett’s picture

Category: task » feature

With some perspective of the other sorts of major tasks we have at this staged, this is a feature request from D7.

dawehner’s picture

Version: 8.0.x-dev » 9.x-dev
Issue summary: View changes

The usage of the toolbar as primary navigation helps a lot here already, as people either see, or don't see the "People" menu item, depending on their access. The problem
on the admin/overview pages though still exists.

I guess though that this can also be moved to 9.x

catch’s picture

Version: 9.x-dev » 8.0.x-dev
Category: Feature request » Bug report
Priority: Major » Normal

I think this is more accurate. Moving back to 8.0.x but postponing on #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.

reekris’s picture

Any update on this now that the issue in #157 is closed?

I'm seeing this issue in Drupal 8.0.4. Enabling the permission "Use the administration pages and help" will show the "Configuration" menu item with all it's child items. Clicking any of them leads to a page with "You do not have any administrative items". Removing the mentioned permission hides the menu items but also hides the "Structure" item even if the user has access to the Taxonomy child menu item.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

christianadamski’s picture

How to proceed here? Write a custom access check and add to service that does two things, check for "administration pages" permission and check for children of current menu item? Is that enough? Is that easy to do?

agoradesign’s picture

Hi there,

I've published a small module, that provides a workaround for that problem in Drupal 8: https://www.drupal.org/project/admin_links_access_filter

It's not very sophisticated, working with preprocessors - but it works for us :)

  • webchick committed 827e278 on 8.3.x
    #296693 by Damien Tournoud, boombatower, sun, and Xano: Hide parent...
  • Dries committed e87bf71 on 8.3.x
    - Patch #296693 by sun: revert system_admin_menu_block_access().
    
    

  • webchick committed 827e278 on 8.3.x
    #296693 by Damien Tournoud, boombatower, sun, and Xano: Hide parent...
  • Dries committed e87bf71 on 8.3.x
    - Patch #296693 by sun: revert system_admin_menu_block_access().
    
    

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alan d.’s picture

This is a much more obvious UX issue with the Admin Toolbar in action, of which roughly 25% of all non-dev Drupal 8 sites seem to have enabled

With a significantly reduced permissions on an administrative user, about 80-90% of all the links result in a "You do not have any administrative items.".

This also makes it time consuming actually testing permission access to see what the users can and can not see.

I'm not sure if this should be flagged with "Needs usability review" for that team to take a look. It's not fun opening doors to see a brick wall sitting behind it ;)

Referenced #2673914: Only show admin menu items with available Configuration pages according to permissions in Admin Toolbar queue that has taken a NIH approach to this issue, pushing back to core for a resolution.

  • webchick committed 827e278 on 8.4.x
    #296693 by Damien Tournoud, boombatower, sun, and Xano: Hide parent...
  • Dries committed e87bf71 on 8.4.x
    - Patch #296693 by sun: revert system_admin_menu_block_access().
    
    

  • webchick committed 827e278 on 8.4.x
    #296693 by Damien Tournoud, boombatower, sun, and Xano: Hide parent...
  • Dries committed e87bf71 on 8.4.x
    - Patch #296693 by sun: revert system_admin_menu_block_access().
    
    
danepowell’s picture

For anyone wondering about the commits for this (#162, #163, #166, #167) those are from waaaay back in 2009. No idea why they are just now being attached to this issue. This is still an issue in all 8.x branches.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dkre’s picture

Issue tags: +DrupalWTF

Thanks @agoradesign https://www.drupal.org/project/admin_links_access_filter works well.

9 Years and counting.. wow.

This is very Drupal. If it's a usability issue it has almost zero priority.

Usability issues are why Drupal is just too difficult to use commercially and scare off new users. Fixing basic usability issues like this blow out dev and training time commercially and confuse the hell out of new users.

Intiatives like #2847582 Out of The Box experience initiative are great but really miss the mark when it comes to addressing what needs to happen to make Drupal acceptable to users in 2017 - like this issue.

For Drupal 7 users I recommend https://www.drupal.org/project/admin_menu_source
It's a time consuming solution but allows you to rename and filter out toolbar link clutter so it's a viable possibility to provide clear and concise training to new admins and most importantly the content editors who are overwhelmed by the Drupal site-builder centric administration menu.

ressa’s picture

The Administration links access filter module just got merged in as sub module to Admin Toolbar module, and is included in the 8.x-1.21 release.

klonos’s picture

Anyone feel like the issue summary needs an update? I mean, D6.4?? :)

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

colan’s picture

I had to read that a few times myself, and then I looked at the issue creation date.

rooby’s picture

Title: Hide empty admin categories » Restrict access to empty top level administration pages
Issue summary: View changes
Issue tags: -Needs issue summary update

I did a quick pass of the initial post so it makes sense for D8.5

rooby’s picture

Issue summary: View changes
rooby’s picture

Issue summary: View changes
rooby’s picture

This issue is related, however it is a little different since that issue refers to a regression and this one doesn't. I feel like the issues should be merged though and dealt with in one go.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mkalbere’s picture

Here is a very ugly, but working js workaround to remove empty menus, not clean, but my customer doesn't want to wait ;-)
It fit my needs, but maybe not all cases.

Drupal.behaviors.menu_hack = {
attach: function(context, settings) {
do {
$(".toolbar-menu-administration ul:not(:has(li))").remove();
$(".toolbar-menu-administration li.menu-item--expanded").each(function() {
if ($("ul", this).length == 0) {
$(this).remove();
}
})
} while ($(".toolbar-menu-administration ul:not(:has(li))").length > 0);
}
}

mathieso’s picture

Thank you, mkalbere! Works nicely.

Tips for others. First, wrap the code in this so that $ is mapped to jQuery:

(function ($, Drupal) {
  "use strict";
  ...
}(jQuery, Drupal));

Second, for me, an empty Workflow menu item was still showing under Configuration. I don't know why. Add this ugly hack before the do loop in mkalbere's code:

      // Remove Workflow menu if MT.
      let workflowMenu = $("a.toolbar-icon-system-admin-config-workflow").parent();
      if (workflowMenu.length > 0) {
        // There is a Workflow menu.
        if (workflowMenu.find("ul").length === 0) {
          // No items in the Workflow menu.
          workflowMenu.remove();
        }
      }

Third, I wanted to give users with only the grader role access to the admin menu, but not the Drupal Help menu. These are in a single permission: "Use the administration pages and help". This code removes the Help menu item for those users.

      // Kill help menus for users who only have the grader role.
      let isJustGrader =
          drupalSettings.currentUserRoles.includes("authenticated")
          && drupalSettings.currentUserRoles.includes("grader")
          && drupalSettings.currentUserRoles.length === 2;
      if (isJustGrader) {
        $("a.toolbar-icon-help-main").parent().remove();
      }

drupalSettings.currentUserRoles was injected in hook_page_attachments():

  // Send user id and roles to JS.
  /** @var Drupal\skilling\SkillingCurrentUser $currentUser */
  $currentUser = \Drupal::service('skilling.skilling_current_user');
  $roles = $currentUser->id() ? $currentUser->getRoles() : [];
  $attachments['#attached']['drupalSettings']['currentUserRoles'] = $roles;

SkillingCurrentUser is a service that wraps the standard User, adding some app-specific stuff. You don't really need it.

NB: I am not an expert Drupal developer.

martijn de wit’s picture

Version: 8.6.x-dev » 8.8.x-dev
rob230’s picture

I think the preferred solution at the moment is to install the very popular Admin Toolbar module, which contains a submodule which fixes it - thanks to agoradesign for his work on this.

Obviously it would still be nice if Drupal core didn't do it to begin with.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

  • webchick committed 827e278 on 9.1.x
    #296693 by Damien Tournoud, boombatower, sun, and Xano: Hide parent...
  • Dries committed e87bf71 on 9.1.x
    - Patch #296693 by sun: revert system_admin_menu_block_access().
    
    
ilya.no’s picture

Here is what I suggest:
1) Add new option to routes, called for example '_group_route' and set it to TRUE for necessary routes.
2) Implement new route subscriber, which is executed during 'onAlterRoutes' event.
The idea of this subscriber is following: for each route with '_group_route' set to TRUE we check, if it has accessible children by any roles with 'access administration pages' permission. If we find such links, we set requirement '_role' witch points which roles should have access to this route. If there is no such links, we add '_roles' anyway with only admin role.
Attaching patch, it doesn't have any tests so far, because I think, at first we need to check, if this idea is acceptable.

ilya.no’s picture

Status: Needs work » Needs review

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

vacho’s picture

- The issue still exists int Drupal 9.

- The proposal solution that @ilya.no gets looks right. Review in deep permission for parent-children sounds good.

- However not work the patch to 9.0.x branch.
If apply the patch => rebuild routes => cache rebuild => all website not work
If you try to reinstall the website => down the installation.

This is the error that I get:
TypeError: Argument 2 passed to Symfony\\Component\\Routing\\RouteCollection::add() must be an instance of Symfony\\Component\\Routing\\Route, bool given, called in .../core/lib/Drupal/Core/Routing/RouteProvider.php on line 382

pameeela credited sic.

pameeela’s picture

Added #2748261: Granting permission to use administration and help sites adds empty menu items as related and closed that as duplicate. Also added @sic here as a contributor for creating the other issue.

tanubansal’s picture

StatusFileSize
new251.08 KB

Tested via above mentioned steps. Issue is still there on 9.1
Please provide if there is any patch available for 9.1

pameeela’s picture

Issue tags: +Bug Smash Initiative
ilya.no’s picture

Attaching patch for D9 with interdiff for previous patch. I've fixed adding roles with OR conditions, instead of previous AND. Also added code for checking menu tree for separate function, because in admin/config menu tree we have other routes with possible empty content, so now it's recursive function.

Status: Needs review » Needs work
vacho’s picture

After applying the patch #195 and following the steps to test(role, permissions, user...). The menu structure for the new user still appears. So needs more work.

ilya.no’s picture

Status: Needs work » Needs review
StatusFileSize
new1.11 KB

I've tested again and it works ok for me. The only problem I had is contrib module Menu Admin per Menu, which adds its own custom access checker for menu 'entity.menu.collection' and return positive access result. But when I disable this module, logic works fine. Added couple improvements for tests.

ilya.no’s picture

paulocs’s picture

StatusFileSize
new12.47 KB
new9.15 KB

Hello,

Patch #199 looks good. But I think we need tests for. Am I right?

Cheers, Paulo.

ilya.no’s picture

Hello, Paulo! Thanks for the review. Yes, need to write some tests.

paulocs’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
ridhimaabrol24’s picture

Status: Needs work » Needs review
StatusFileSize
new2.25 KB
new11.51 KB
new2.1 KB

Adding a test.

The last submitted patch, 203: 296693-203-test-only-patch.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 203: 296693-203.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

paulocs’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new647 bytes
new11.51 KB

Fixing code standard.

Status: Needs review » Needs work

The last submitted patch, 206: 296693-206.patch, failed testing. View results

anmolgoyal74’s picture

Status: Needs work » Needs review
StatusFileSize
new11.5 KB
new633 bytes
paulocs’s picture

Status: Needs review » Reviewed & tested by the community

Looks good now.

quietone’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs screenshots, +Needs issue summary update

Nice top see these old issue getting some attention, thanks!

I thought this could use a before and after screenshot so I tried to make one. I was not able to make a user with access to the admin pages on 9.1.x with a minimal install. I gave the user has every permission available. Like the test, Block and Toolbar are installed.

Then I decided to review the code.

  1. +++ b/core/modules/system/src/EventSubscriber/GroupRouteSubscriber.php
    @@ -0,0 +1,145 @@
    +use Drupal\Core\Routing\RouteSubscriberBase;
    

    Can we please put the use statements in alphabetical order.

  2. +++ b/core/modules/system/src/EventSubscriber/GroupRouteSubscriber.php
    @@ -0,0 +1,145 @@
    +   * @param Symfony\Component\Routing\RouteCollection $collection
    

    Should be \Symfony

  3. +++ b/core/modules/system/src/EventSubscriber/GroupRouteSubscriber.php
    @@ -0,0 +1,145 @@
    +   * @param Drupal\user\Entity\User $test_user
    

    Should be \Drupal

  4. +++ b/core/modules/system/src/EventSubscriber/GroupRouteSubscriber.php
    @@ -0,0 +1,145 @@
    +   * @param string $test_role
    

    Seems really odd to be using $test_user and $test_role. Why not just $user and $role?

  5. +++ b/core/modules/system/src/EventSubscriber/GroupRouteSubscriber.php
    @@ -0,0 +1,145 @@
    +    // Load all menu links below route
    +    // and check if route has any accessible children by any of roles.
    

    Wrap to 80 character.

    And this need a tweak, what does 'children by any of roles' mean?

  6. +++ b/core/modules/system/src/EventSubscriber/GroupRouteSubscriber.php
    @@ -0,0 +1,145 @@
    +              // User is allowed to visit this page, but need to check,
    +              // if it's also group route with inaccessible children.
    

    Line wrap to 80 characters.

    I wonder if 'group route' should be group_route'. I don't really know.

  7. +++ b/core/modules/system/tests/src/Functional/Menu/MenuAccessTest.php
    @@ -70,4 +70,48 @@ public function testMenuBlockLinksAccessCheck() {
    +  public function testMenuAccessByUserRole() {
    

    This test checks only one role, that a user without access to the structure menu does not see the structure menu. It would be good to test that a user with access does see the structure menu, just to be sure nothing broke.

  8. +++ b/core/modules/system/tests/src/Functional/Menu/MenuAccessTest.php
    @@ -70,4 +70,48 @@ public function testMenuBlockLinksAccessCheck() {
    +    $this->drupalPostform('admin/people/roles/add', $edit, t('Save'));
    ...
    +    $this->drupalPostform('admin/people/permissions/test_role', $edit, t('Save permissions'));
    ...
    +    $this->drupalPostform('user/' . $webUser->id() . '/edit', $edit, t('Save'));
    

    This is deprecated.

  9. +++ b/core/modules/system/tests/src/Functional/Menu/MenuAccessTest.php
    @@ -70,4 +70,48 @@ public function testMenuBlockLinksAccessCheck() {
    +    $this->assertSession()->responseNotContains('class="toolbar-icon-system-admin-structure"');
    

    I ran the test locally and looked at the resulting html output. The 'structure' menu item is displayed. So this is not doing what is expected. In the html out the class of of 'structure' was 'toolbar-icon toolbar-icon-system-admin-structure'. So, that needs to be changed and then there should be a new fail and pass test as well.

paulocs’s picture

StatusFileSize
new6.56 KB
new6.25 KB

I fixed what @quietone pointed on comment #210, except item 7 and 9.
I could disable the "Structure" menu item but I don't why the tests are falling when set $this->assertSession()->responseNotContains('class="toolbar-icon toolbar-icon-system-admin-structure"'); if in my case I don't see the Structure item.

I attached a new patch with the interdiff but it still needs work.

jwilson3’s picture

Why did #211 remove GroupRouteSubscriber.php ?

ridhimaabrol24’s picture

StatusFileSize
new11.42 KB
new5.6 KB

Adding back GroupRouteSubscriber.php with the feedback implemented as mentioned in #210 except point 7. I have added back the class "toolbar-icon toolbar-icon-system-admin-structure" in the test. Hence the tests will be failing as of now. Still need to fix that.

quietone’s picture

I agree everything in #210 is addressed, except the test #7.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

saurabh-2k17’s picture

StatusFileSize
new11.36 KB

I was facing the same issue for 8.9.x branch, so I have back ported #213 patch and made it compatible.
PFB patch for 8.9.x branch.

arshadkhan35’s picture

StatusFileSize
new11.79 KB

Re-rolling #216 with a change where we are making

admin/config/people

route as group route to handle People menu.

diff --git a/core/modules/user/user.routing.yml b/core/modules/user/user.routing.yml
index dc618eed8a..c4f379d364 100644
--- a/core/modules/user/user.routing.yml
+++ b/core/modules/user/user.routing.yml
@@ -20,6 +20,8 @@ user.admin_index:
     _title: 'People'
   requirements:
     _permission: 'access administration pages'
+  options:
+    _group_route: TRUE

 entity.user.admin_form:
   path: '/admin/config/people/accounts'
arshadkhan35’s picture

re-rolling #217 against 9.0.x branch.

kapilv’s picture

Status: Needs work » Needs review
StatusFileSize
new6.77 KB
new7.21 KB

the patch does not apply 9.2.x, Hear an updated patch against 9.2.x.

kapilv’s picture

StatusFileSize
new11.89 KB
abhijith s’s picture

StatusFileSize
new11.06 KB
new14.37 KB

Applied patch #220 and it works fine.

Before patch:
before

After patch:
after

RTBC +1

Status: Needs review » Needs work

The last submitted patch, 220: 296693-220.patch, failed testing. View results

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

vacho’s picture

mmm for Drupal 9.3.x patch #220 not works.

The structure menu for a new role with the permissions 'Use the administration pages and help', 'View the administration theme' and 'Use the administration toolbar' still is showing.

BTW

after run:

$ drush cr
$ drush ev '\Drupal::service("router.builder")->rebuild();'

the website fails

54  07/May 17:54  error     php   TypeError: Argument 2 passed to Symfony\Component\Routing\RouteCollection::add() must be an instance of Symfony\Component\Routing\Route, bool given, called in /var/www/html/contribdrupal9/ 
 55  07/May 17:54  warning   php   Warning: Erroneous data format for unserializing 'Symfony\Component\Routing\Route' in Drupal\Core\Routing\RouteProvider->getRoutesByPath() (line 380 of /var/www/html/contribdrupal9/core/li 

pameeela’s picture

ilya.no’s picture

Status: Needs work » Needs review
StatusFileSize
new12.06 KB
new1.81 KB

Attaching updated patch. For some reason patch from #218 changed important line without any explanation why. I've brought initial logic back, because following code is senseless, when we have several roles:

$route->setRequirement('_role', (string) implode('', $route_accessible_roles));

We need to append them with `+` sign in order to indicate, that user of any of these roles should have access to this link.

$route->setRequirement('_role', (string) implode('+', $route_accessible_roles));

I've also added rebuilding routes command to tests and group option to config route item. Change for cron access was removed, because it seems to be out of scope, please, correct me, if I'm wrong.

Status: Needs review » Needs work
anweshasinha’s picture

I am working on this

jeroent’s picture

Status: Needs work » Needs review
StatusFileSize
new11.58 KB
jeroent’s picture

StatusFileSize
new6.92 KB
jeroent’s picture

StatusFileSize
new11.41 KB
new630 bytes

Status: Needs review » Needs work

The last submitted patch, 232: 296693-231.patch, failed testing. View results

jeroent’s picture

Status: Needs work » Needs review
StatusFileSize
new9.99 KB
new9.65 KB
jeroent’s picture

StatusFileSize
new10.19 KB
new1.96 KB

I was wondering, instead of a routeSubscriber, why not add a new access check?
With this approach, a route rebuild is not necessary since the check is cached per role.

jeroent’s picture

StatusFileSize
new10.67 KB
new892 bytes

Added some missing routes.

jeroent’s picture

StatusFileSize
new12.04 KB
new1.1 KB
vacho’s picture

StatusFileSize
new69.39 KB
new68.35 KB
new69.39 KB

After review, the code and the patch #237 I saw all right.

This screen is before the patch for a role that doesn't have any Structure menu item with grant access.
Before

This screen is after the patch.
After

Looks good now.

vacho’s picture

Status: Needs review » Reviewed & tested by the community
larowlan’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs screenshots +needs profiling
  1. +++ b/core/modules/system/src/Access/SystemAdminMenuBlockAccessCheck.php
    @@ -0,0 +1,98 @@
    +      // Check if it is a _group_route with inaccessible children.
    

    What is the significance of `_group_route` here - is that a copy/paste from group module?

  2. +++ b/core/modules/system/tests/src/Functional/Menu/MenuAccessTest.php
    @@ -70,4 +70,46 @@ public function testMenuBlockLinksAccessCheck() {
    +    $this->drupalLogin($menuAdmin);
    +    $this->drupalGet('admin/structure');
    

    it would be good to have a test here that also tested when the user didn't have access to a direct child, but did to a secondary level child.

    e.g. grant a permission to 'administer filters' and then try /admin/config - which would test admin/config/content/formats

  3. +++ b/core/tests/Drupal/Nightwatch/Tests/loginTest.js
    @@ -13,7 +13,7 @@ module.exports = {
    -        permissions: ['access site reports'],
    +        permissions: ['access site reports', 'administer site configuration'],
    

    This looks like a regression? In that test the user access /admin/reports so should only need access site reports, but now they need both?

    Can you elaborate why this change is needed?

  4. Also, added the 'needs profiling' tag, because we're adding a lot of access checking here, specifically we're iterating over every menu link that lives under a given menu link to check access. When we're building a menu tree, that's a lot of links, and we access each link under an item for each item. Previously (d6) this was rolled back for performance issues, so let's make sure we've profiled the impact of this patch before we commit it
vikashsoni’s picture

StatusFileSize
new19.51 KB
new7.61 KB

patch applied successfully
Thanks @JeroenT for the patch for ref sharing screenshot...

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

ilya.no’s picture

Status: Needs work » Needs review
StatusFileSize
new12.15 KB
new3.58 KB

Attaching patch with fixes for 1 -3 points. I haven't proceeded with profiling yet.

ilya.no’s picture

StatusFileSize
new169.83 KB
new108.94 KB

I've checked `/admin` page with blackfire on Drupal 9.3.7, site is built within skilld docker container.

Without patch:

Time 211 ms
I/O Wait 2.42 ms
CPU 209 ms
Peak Memory 18.4 MB

With patch:

Time 234 ms
I/O Wait 1.2 ms
CPU 233 ms
Peak Memory 19 MB

Result
Time +22.9 ms (+10.8%)
CPU +24.1 ms (+11.5%)
Peak Memory +583 kB (+3.17%)

Attaching some screenshots with comparison.
Please, let me know, if this is ok, or maybe I need to provide more comprehensive data.

vacho’s picture

StatusFileSize
new38.52 KB

Again, I review the code and the functionality. For me, all looks good.

Working well image wit an user with a role wit permissions:

  • - Use the administration pages and help
  • - View the administration theme
  • - Use the toolbar

without bloc administration

vacho’s picture

Status: Needs review » Reviewed & tested by the community
quietone’s picture

Status: Reviewed & tested by the community » Needs work

@ilya.no, thanks for responding to #240 and doing the profiling. It would help reviewers and committers to add a link to the profile results in the Issue Summary.

The remaining tasks in the Issue Summary are not yet complete and the patch is not passing tests, setting to NW. I left the tag for profiling because the patch is not passing tests yet.

@vacho, thanks for the confidence in the patch. However, having a working patch is not usually sufficient to mark an issue RTBC. There are several steps, or core gate that an issue must pass first.

Updating credit. @vikashsoni adding screenshots for a patch that needs work is not helping to move this forward, removing credit. Similar for @vacho and @Abhijith S where the patch was not passing tests.

edit: remove copy/paste error

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

benjifisher’s picture

Issue summary: View changes

Usability review

We discussed this issue at #3324991: Drupal Usability Meeting 2022-12-09. That issue will have a link to a recording of the meeting.

For the record, the attendees at today's usability meeting were @Antoniya, @benjifisher, @g-brodiei, @rkoller, and @simohell.

We had some concerns about the proposed resolution. The new route parameter is applied to some routes. It does not account for additional menu items that could be added to the administration menu. There is no way for a site administrator to override the route parameter.

Not every site owner will conside the current behavior a bug. Some may prefer all their users to have consistent top-level menus. Some may want users to have an indication that there is more they could access if they get additional permissions.

Instead of changing the access to these pages, we would like to give site administrators control over whether these "container" pages appear in the menu when they have no children. We suggest adding an option in the menu admin pages, next to the "Enable menu link" and "Show as expanded" options. The new option could be called "Container". The description could be something like this (first draft): "Show this link only when at least one child link is shown." Perhaps the new option should be available only when "Show as expanded" is selected.


It looks as though some profiling was done in Comment #244, so we might remove the "needs profiling" issue tag. On the other hand, I would like to see profiling with more than one scenario. For example, adding several content types and the contrib Admin Toolbar module makes the menu a lot larger.

Even without usability concerns, I do not like the approach in the current patches. The menu system goes to a lot of trouble to load all the menu items once and avoid backtracking. See the code in core/lib/Drupal/Core/Menu, especially MenuLinkTree::buildItems(). I think that is a better place to implement changes for this issue, rather than using the access system.

Adding another option to the menu admin pages would involve some work: API and data model changes. But the performance when generating the menu should not be a problem.

I am updating the issue summary: adding the current approach and the one I suggested to the "Proposed resolution" section.

catch credited smustgrave.

catch’s picture

benjifisher’s picture

@catch:

I would appreciate an opinion from a framework manager on my suggestion in #250. Unless there is a simpler way, it would involve adding a boolean to the menu_link_content entity. In the UI, it would be a checkbox option along with the expanded option.

Is it worth an API and data model change to give site administrators control over whether users can see the empty page?

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

ilya.no’s picture

Status: Needs work » Needs review
StatusFileSize
new10 KB

Hello @benjifisher. I'm attaching draft patch for your suggestions from comment #250. It almost works with 2 points:
1) Field is added, but it doesn't look, that it's fully functional in terms of content editing.
2) Route for configuration page /admin/config is hidden even if user has items in it.
So it's incomplete, but I need to know, how it's close to your thoughts.

smustgrave’s picture

Status: Needs review » Needs work

Moving to NW for issue summary update.

Did not test.

tedbow’s picture

I am reviewing this issue along with #2856666: Hide administration menu links that do not have visible child links because @ckrina pointed out in Slack that these issues solve the same problem in different ways. I also talked @lauriii about the need to choose one of these issues agreed we should close #2856666 and leave the present issue open.

I personally recommend the method in this issue because it seems like the access to the route is the better way to solve this than the menu level as #2856666 and was suggested in #250

If we solve this on the menu level than any other way a module outputted the link to these routes would still result in the user having access to the link and URL but then arriving at a page where they told they have no items. It is common when a module is going to output a link to a URL to check if the user has access to the link. The module may even provide an alternative link for users that don't have access to a first link. For instance we do this in Automatic Updates in some circumstances, if the user does not have access to the update form we will instead provide the link to the Available Updates report. Module would not able to do this type of checks if we don't handle this on route access level.

To me it makes sense as the default behavior for routes to callbacks such as \Drupal\system\Controller\SystemController::systemAdminMenuBlockPage where the sole purpose to show other admin items is that if a user has no access to the those items that user should have no access to that route.

re #250

The new route parameter is applied to some routes. It does not account for additional menu items that could be added to the administration menu.

I am not sure if this current patch is working yet for this but it seems like it should be able to take into account menu items that are added to the administration menu.

For instance if a user adds a menu item under Config -> People I think \Drupal\system\Access\SystemAdminMenuBlockAccessCheck::hasAccessToChildRoutes could make sure that the link is checked and if the user has access to that link but no other links except the one added they should still have access to /admin/config.

On the other hand I don't think we should care if a user adds another top level menu item, say /admin/custom, to the admin menu and then adds links menu below it. If the user has no access to any of the menu items below it there is no programmatic way to know if /admin/custom is a page that outputs a links besides looking for known controller callbacks that core provides.

If we wanted to change this issue slightly we could change to route subscriber that would add the `_access_admin_menu_block_page` requirement to routes where the _controller: \Drupal\system\Controller\SystemController::systemAdminMenuBlockPage and the few other known core callback that need this behavior. It might make it easier for contrib.

Even the Menu based approach used in #2856666 does not solve the problem of user added links as it relies on settings a transitive: true property on menu items.

There is no way for a site administrator to override the route parameter.

Not every site owner will conside the current behavior a bug. Some may prefer all their users to have consistent top-level menus.

I talked this over with Lauri. While it is true that not every admin would see the current behavior as a bug that is true with any user facing interface bug. If we could not fix UX bugs because some people would prefer that current behavior it would be very hard to fix any UX bugs.

This issue has 193 followers. I looked through all the comments before the UX review in #250 I didn't see any comments saying they prefer the current behavior(there were a lot so I could missed one), although of course some one will for the reason stated in the UX review we can't have solution that absolutely everyone will be happy with.

But I think the need for this is summed up well in #1 back in 2008 🙀

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.

Regarding adding an option re #250

Instead of changing the access to these pages, we would like to give site administrators control over whether these "container" pages appear in the menu when they have no children. We suggest adding an option in the menu admin pages, next to the "Enable menu link" and "Show as expanded" options. The new option could be called "Container". The description could be something like this (first draft): "Show this link only when at least one child link is shown." Perhaps the new option should be available only when "Show as expanded" is selected.

I think this goes back to the fact that this is anissue opened in 2008 with 193 followers I couldn't find anyone in the comments saying "Please don't change this I think the current behavior works great."

To me configuring menu's is complicated enough and we should not make it even more complicated. A contrib or custom module could alter the routes and remove the _access_admin_menu_block_page requirement and add their own which would have different a behavior even adding a UI option like suggested but having 2 ways for this behave doesn't seem like something that is needed in core. Looking at the list of contribors and commenters on this issue these are a lot developers that probably work with a lot of clients if there was a real world need for top level categories to be the same for all users regardless of whether each user has access to those items it would have come up before.

As far as can tell the reason this issue is not already done is because of performance implications not differing opinions on whether users need to these empty admin pages.

I will comment on #2856666: Hide administration menu links that do not have visible child links to state I think we should close that issue with a link to this comment

tim.plunkett’s picture

+1. Copying credit from the other issue.

omkar.podey’s picture

@ted.bow noticed that in SystemAdminMenuBlockAccessCheck we do $tree = $this->menuLinkTree->load(NULL, $parameters);, So instead of NULL should we use 'system.admin' as there might be 2 different parents containing children with same route names and this could cause issues as unintentional access would be provided to one of the parent.

catch’s picture

Now that we have render and access caching, cache contexts etc. the performance considerations are likely to be different here.

Will probably still not be great on cold starts but should be able to cache the access OK and ideally use the cache metadata of the child routes to determine the cache ability of the parent routes (so e.g. it depends on permissions in 99% of cases).

tedbow’s picture

Issue tags: +Needs tests

Adding a "Needs tests" I put a comment in the MR about what extra test coverage we need

hooroomoo made their first commit to this issue’s fork.

hooroomoo’s picture

Issue tags: -Needs tests

Test cases were added so removing tag

hooroomoo’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
StatusFileSize
new276.39 KB
new206.78 KB

Updated issue summary so removing tag

hooroomoo’s picture

Issue summary: View changes
tim.plunkett’s picture

After spending the afternoon digging into #3381929: Restrict access to empty top level administration pages for overview controller, I think the last commit I pushed belongs in this issue and negates the need for the follow-up.

tedbow’s picture

Assigned: Unassigned » tedbow
Status: Needs review » Needs work

The last fail was expected. See my MR comment

I assigning to myself to update test cases

lauriii’s picture

Issue tags: +Admin UX
tedbow’s picture

Ok found #3359511: [regression] missing menu active trail since Drupal 9.5.9 which I think this why we get the problem of seeing "You do not have any administrative items." when there are actually menu items under the current item but since \Drupal\system\Controller\SystemController::systemAdminMenuBlockPage does not take any parameters in our case it is kind of forced problem test case.

but I think this means we don't need make a follow-up

tedbow’s picture

Assigned: tedbow » Unassigned
Status: Needs work » Needs review

Ok. I have expanded the test coverage to include specifically a sub-menu item that uses \Drupal\system\Controller\SystemController::systemAdminMenuBlockPage but does not have items at all under it. This is similar to /admin/config/workflow, but does dependent on that menu item never changing.

I also expanded the comments.

hooroomoo’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me

catch’s picture

Status: Reviewed & tested by the community » Needs review

It would be good to check that this really is cached, for example:

put dump('foo') in the new access callback, hit an admin page a couple of times and/or other pages with the toolbar, make sure the access check doesn't keep running over and over.

Don't think we need profiling of the uncached case, it's as optimized as it can be looking for any link with access and returning early.

tedbow’s picture

Assigned: Unassigned » tedbow

re #275. I don't think it is being cached. Will work on this

lauriii’s picture

I did check this earlier and the request itself is not cached because of \Drupal\dynamic_page_cache\PageCache\ResponsePolicy\DenyAdminRoutes::check prevents caching admin pages. This results in a single call to \Drupal\system\Access\SystemAdminMenuBlockAccessCheck::access when accessing pages rendered by \Drupal\system\Controller\SystemController::systemAdminMenuBlockPage. The links in toolbar itself, and submenus in /admin/config are cached.

tedbow’s picture

Assigned: tedbow » Unassigned
Issue tags: -needs profiling

I confirm #277 I saw the single call to \Drupal\system\Access\SystemAdminMenuBlockAccessCheck::access after the first load for a user

removed "needs profiling" tag as per @catch in #275

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

re-RTBC'ed since the caching was checked

  • catch committed bcaa369c on 11.x
    Issue #296693 by tedbow, omkar.podey, sun, ilya.no, JeroenT, tim....
catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for double-checking the caching, that sounds good.

I didn't think when I asked for this to be rolled back in 2009 that I'd be committing it from the same issue fourteen years later.

When the original patch went in it caused around a 20% performance regression in Drupal 7 for any user with the toolbar. For every link in the toolbar, it would render the system admin menu block in its entirety, on every page on the site, on every request.

Since then we have:

1. Toolbar caching
2. Render caching, for example of regular menu blocks.

The access check added here is different from the original one that went in, instead of rendering the actual block, it just checks access on child links one at a time until it finds an accessible one then returns early, this was also first suggested back in 2009 in #77.

I was trying to think of the earliest point this could have been fixed in the way it has been, possibly after #1814932: Caching strategy for D8 toolbar, although render caching of menu blocks generically wasn't added until Drupal 8, so somewhere between 3-5 years after the original rollback.

Either way good to close it now.

Committed bcaa369 and pushed to 11.x. Thanks!

tedbow’s picture

StatusFileSize
new35.54 KB

@catch thanks for giving the context. Quite a journey🙌

Assuming this will not get ported back to 10.1.x here is patch

Status: Fixed » Closed (fixed)

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

simon georges’s picture

StatusFileSize
new18.74 KB

As "Configuration" is not using the same Controller (but \Drupal\system\Controller\SystemController::overview), people still see the "Configuration" menu. Adding a $route->setRequirement('_access_admin_menu_block_page', 'TRUE'); using a custom RouteSubscriber seems to work, but I am wondering if this behavior is expected or if you did not take that into account for a reason.

Edit: correction, adding that will block access to "Configuration" page to everyone (admin included) :-(

aaronmchale’s picture

@Simon Georges I haven't tested that myself, but would recommend opening a follow-up issue where it can be properly investigated and fixed, thanks.

tim.plunkett’s picture

jelle_s’s picture

giuseppe87’s picture

For who's landing on this issue from admin_toolbar_links_access_filter description:

from what I saw, the module still works to hide the "Configuration" link or similar cases of
#284/ #3381929: Restrict access to empty top level administration pages for overview controller

It could be useful in sites like mine, where #3413508 patch is necessary and #3381929 one conflicts with it.

leo liao’s picture

#284 @Simon Georges
I'm creating an issue for this. https://www.drupal.org/project/drupal/issues/296693

abelass’s picture

Is there a path for 10.2 available?

martijn de wit’s picture

This is already in 10.2 @abelass

See: https://www.drupal.org/about/core/blog/new-drupal-core-branching-scheme-... Why it is : "catch committed bcaa369c on 11.x"

abelass’s picture

Ok, perfect. Thanks

chri5tia’s picture

Patch for 10.1 changes are rolled into 10.2, checked.

Skipped patch 'core/modules/content_moderation/tests/src/Functional/ModeratedContentLocalTaskTest.php'.
Skipped patch 'core/modules/language/tests/src/Functional/LanguageBreadcrumbTest.php'.
Skipped patch 'core/modules/system/src/Access/SystemAdminMenuBlockAccessCheck.php'.
Skipped patch 'core/modules/system/src/Controller/SystemController.php'.
Skipped patch 'core/modules/system/src/EventSubscriber/AccessRouteAlterSubscriber.php'.
Skipped patch 'core/modules/system/system.routing.yml'.
Skipped patch 'core/modules/system/system.services.yml'.
Skipped patch 'core/modules/system/tests/http.php'.
Skipped patch 'core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml'.
Skipped patch 'core/modules/system/tests/modules/menu_test/menu_test.permissions.yml'.
Skipped patch 'core/modules/system/tests/modules/menu_test/menu_test.routing.yml'.
Skipped patch 'core/modules/system/tests/src/Functional/Menu/MenuAccessTest.php'.
Skipped patch 'core/modules/system/tests/src/Functional/SecurityAdvisories/SecurityAdvisoryTest.php'.
Skipped patch 'core/modules/system/tests/src/Functional/Session/SessionHttpsTest.php'.
Skipped patch 'core/modules/toolbar/tests/src/Functional/ToolbarAdminMenuTest.php'.
Skipped patch 'core/modules/toolbar/tests/src/Functional/ToolbarMenuTranslationTest.php'.
Skipped patch 'core/modules/toolbar/tests/src/Nightwatch/Tests/toolbarTest.js'.
Skipped patch 'core/modules/update/tests/src/Functional/UpdateSemverCoreTest.php'.
Skipped patch 'core/tests/Drupal/Nightwatch/Tests/loginTest.js'.

Does anyone know why this issue is linked to "/Admin Toolbar Links Access Filter" being a deprecated module in status report? Since the patch is already rolled into Drupal 10.2, as far as I can tell, I'm not sure how to fix this or this issue is the one that should actually be linked for this.

liam morland’s picture

Is it the case that admin_toolbar_links_access_filter module is no longer needed as-of Drupal 11.0? Or can it be removed earlier?

giorgosk’s picture

For people still wondering about Admin Toolbar Links Access Filter it is no longer needed based on the maintainer's Proposed resolution here https://www.drupal.org/project/admin_toolbar/issues/3485771