Make the menu shown in the administration menu tray configurable

I actually have no idea, why we would do this at all. This seems like something you want to extend to in contrib, not core.

For example, passing the menu_name would be a nice option.

Currently, toolbar_view() does the following:

  // Retrieve the admin menu from the database.
  // Add logout & user account links or login link.
  // Add a "home" link.

One might think these are arbitrary decisions. You could make these actions optional by adding a parameter for the pre_render function in toolbar_page_build(). They can then be set in a custom module or on an admin page.

#1500302: Make the toolbar menu configurable per role/per user (D7) adds a parameter for choosing the desired menu. (So I can use admin_menu for the administrator, and the more stylish Toolbar for key-users with my custom menu).

E.g., in the Commerce Kickstart Profile, you'll find a toolbar_megamenu_module, which is almost a 100% copy of toolbar.module. You can avoid that.

@benjiifisher I understand, however your reasoning should also have applied tot he D7 toolbar and frankly, we have rarely seen a request for it in the past 2 years.

@benjifisher, Bojhan,
I do not know about the (D8?) hook_toolbar_alter, and IMO #596010: Move "Administration" link into Account menu and make Toolbar output first level of 'admin' menu is the correct way to go. Then, people have the following options:
- do not change the toolbar 'menu', but change the administration menu, and it will be adapted by toolbar automatically.
- create your own menu, and show it in the Toolbar, using a new parameter in hook_page_build();
- change the toolbar menu afterwards in the #pre_render callback.

Please review the following issues:
#1500302: Make the toolbar menu configurable per role/per user (D7) contains a patch to pass the menu_name.
#1877594: Toolbar (administration menu) broken after disabling 'commerce kickstart menus' contains an example implementation for toolbar_megamenu, removing 80 lines of code in toolbar_megamenu.
The proposed solution let you programmatically change all aspects of the returned default toolbar.
Below is a very short resumé of #1877594:

function MYMODULE_page_build(&$page) {
  if (isset($page['page_top']['toolbar'])) {
    $page['page_top']['toolbar']['#pre_render'] = array('MYMODULE_pre_render');
    $page['page_top']['toolbar']['menu_name'] = 'management';  // <--- your own menu here
  }
}

function MYMODULE_pre_render($toolbar) {
  $toolbar = array_merge($toolbar, MYMODULE_toolbar_view($toolbar['menu_name']));
  return $toolbar;
}

function MYMODULE_toolbar_view($menu_name) {
  global $user;

  // Retrieve the menu from the database, using default toolbar, with our own menu.
  $build = toolbar_view($menu_name);
  // Add our custom details.
  $build['toolbar_menu']['#prefix'] .= '<h2 class="element-invisible">' . t('Administrative toolbar') . '</h2>';
  ...
  return $build;
}

I actually have no idea, why we would do this at all. This seems like something you want to extend to in contrib, not core.

+1

however your reasoning should also have applied tot he D7 toolbar and frankly, we have rarely seen a request for it in the past 2 years.

+1

So let's not introduce such a UI. That's contrib material.

What I would like to do is make the toolbar flexible enough that you do not have to do anything so drastic.

+1

We can do that quite easily now! Basically, change

    'tray' => array(
      '#heading' => t('Administration menu'),
      '#attached' => $subtrees_attached,
      'toolbar_administration' => array(
        '#pre_render' => array(
          'toolbar_prerender_toolbar_administration_tray',
        ),

to:

    'tray' => array(
      '#heading' => t('Administration menu'),
      '#attached' => $subtrees_attached,
      'toolbar_administration' => array(
        '#menu' => 'admin',
        '#pre_render' => array(
          'toolbar_prerender_toolbar_administration_tray',
        ),

And update toolbar_prerender_toolbar_administration_tray() to look at #menu and use that as the menu to be rendered.

Trying patch as per #9

+++ b/core/modules/toolbar/toolbar.module
@@ -210,7 +211,8 @@ function toolbar_prerender_toolbar_administration_tray(array $element) {
-  $parameters->setRoot('system.admin')->excludeRoot()->setTopLevelOnly()->onlyEnabledLinks();
+  isset($element['#menu']) ? $menu_root = $element['#menu'] : $menu_root = 'system.admin';
+  $parameters->setRoot($menu_root)->excludeRoot()->setTopLevelOnly()->onlyEnabledLinks();

I would suggest maybe not creating a new variable here, but rather setting $element['#menu'] back to the default value if it has been removed and using it in place of $menu_root. Otherwise, visually the code seems to make sense.

Thanks Deciphered, suggestion implemented.

Thanks for taking this on!

1. +++ b/core/modules/toolbar/toolbar.module
   @@ -180,6 +180,7 @@ function toolbar_toolbar() {
   @@ -210,7 +211,8 @@ function toolbar_prerender_toolbar_administration_tray(array $element) {
   

   The documentation of this function should be updated to mention #menu.

2. +++ b/core/modules/toolbar/toolbar.module
   @@ -210,7 +211,8 @@ function toolbar_prerender_toolbar_administration_tray(array $element) {
   +  $element['#menu'] = isset($element['#menu']) ? $element['#menu'] : 'system.admin';
   

   Just assume #menu exists. No need to provide a fallback. This line can be deleted entirely.

@Wim,

Dan spoke to me about #2 prior to my review, I was inclined to agree, but his justification is that as someone can use hook_toolbar_alter() they could remove $element['#menu'], which would cause an error here. While it would be their fault entirely, a check here, or setting the default as he did, would prevent the error.

While it would be their fault entirely, a check here, or setting the default as he did, would prevent the error.

We like to say: We don't babysit broken code.

It'd be instantly clear that his/her code is wrong, because the "Manage" tab (with the associated admin menu toolbar tray) would then just be completely empty (I just tried it manually just to make sure that that is indeed the case).

As long as it's clear in some way to the developer that the code is broken, either through a broken UI (like we have here) or through an explicit PHP exception (which is unnecessary here), then there is no need for such defaults.

Thanks Wim and Deciphered, been great to learn some insight into how we approach this kind of scenario!

The fallback line has now been removed.

+++ b/core/modules/toolbar/toolbar.module
@@ -181,6 +181,7 @@ function toolbar_toolbar() {
         '#pre_render' => array(
+          '#menu' => 'system.admin',
           'toolbar_prerender_toolbar_administration_tray',

You now put #menu *inside* #pre_render… that won't work, since it's not a valid callback :)

You should put them on the same level.

Oh wow... sorry about that!

:) No problem. Thank you very much!

So this patch does not really look like a feature anymore. Also

The Toolbar module should create a page where the site admin can configure the toolbar. It should make use of the new configuration system and the Breakpoint module. Then the Toolbar module can implement its own hook, hook_toolbar_alter(), in order to make the changes happen.

the patch does not do this...

That's from the original proposal. Based on the prior discussion, I proposed in #9 not add such a UI, because that's really contrib material.

IS updated.

This allows contrib to use toolbar better and is not disruptive. But we should have test coverage of this functionality.

Marked #1939884: Make the administration menu in the toolbar plugabble: don't hardcode to the 'admin' menu and #2289307: Toolbar module should allow setting a different menu via configuration as duplicates of this issue.

There is already a contrib module that allows to replace admin menu in toolbar with another menu source. See https://www.drupal.org/project/navbar_extras, Navbar Sources sub-module. It does that based on a user role. I'd suggest to move Navbar Sources into an individual project and create a version for Drupal 8.

@temkin I used Admin Menu Source to get the same thing in D7 https://www.drupal.org/project/admin_menu_source. While they have no plans to port to D8, I suggest we port one of the 2 modules to D8 with support for core Toolbar module and Admin Toolbar module https://www.drupal.org/project/admin_toolbar.

Here's a re-rolled patch for 8.6.x. Unfortunately, there is a second instance of the menu name now in toolbar_get_rendered_subtrees() which I think should be inherited from hook_toolbar(), but I don't know the best way to go about this.

I haven't seen anyone mention Toolbar Menu module, which is available for Drupal 8 and provides a way to add custom menus to the toolbar:

https://www.drupal.org/project/toolbar_menu

I'm using it with Tasty Backend at the moment to swap out the default administration menu for a custom admin menu for certain user roles. It works with admin toolbar module, and it also contains permissions to access each toolbar menu element which means you can show/hide menus for different roles.

What Toolbar Menu doesn't do, is provide a permission for the default administration menu, so Tasty Backend handles that with a custom permission and an access check in a hook_toolbar_alter().

I'm wondering if the Toolbar Menu approach would solve this with an additional permission for the default administration menu?

@jeni_dc here's the feature request issue for toolbar_menu you mentioned above: https://www.drupal.org/project/toolbar_menu/issues/2949053

I'll check out Tasty Backend to see if the hook_toolbar_alter() can be used for toolbar_menu.

Needs a reroll

Rerolled patch for 9.1.x, please review.

Adding this as another possible use-case for #2464193: Provide configuration options for toolbar menu

Setting this back to "Needs Work" per comment in #36:

there is a second instance of the menu name now in toolbar_get_rendered_subtrees() which I think should be inherited from hook_toolbar(), but I don't know the best way to go about this.

    // @TODO inherit from hook_toolbar().
    '#menu' => 'admin',

It seems that this needs to be addressed in order for this to work.

Attached is a patch that includes tests.

I'm not sure why that patch fails testing... the errors are from CKEditor tests, which AFAIK are not related...

But regardless, this is still "Needs work".

The next step here is to rework toolbar_get_rendered_subtrees() so that it inherits #menu from hook_toolbar() (somehow)...

One other thing to note: the Toolbar module currently has hard-coded assumptions about the menu depth. In a default installation, there is an admin menu with a single top level system.admin menu link, which all admin menu links are children of.

Therefore the Toolbar module assumes that it should extract all links that are children of the top-level link, and display them in the tray.

This means that any custom menu used in the tray will need to have the same structure: a single top-level item, with all menu links as children of that.

I'm not sure if we want to consider making this behavior alterable as well in this patch, or if we should keep this simple for now and leave it as-is. In order for my test to pass, it creates two menu links: one parent and one child, and it tests for the presence of the child link.

It does not seem that there's a very simple and straightforward way to get the menu name from hook_toolbar_alter() to where we need it in toolbar_get_rendered_subtrees() (or ultimately preRenderGetRenderedSubtrees where it gets used), without either requiring contrib modules who want to set a different menu to set it in two separate places (ugh) or significant refactoring.

Another option to consider: do we add a toolbar.settings config?

This would be more elegant than hook_toolbar_alter() in many ways. Perhaps just something as simple as:

mymodule/config/install/toolbar.settings.yml

admin_tray:
  menu_name: my_menu

Is this worth considering? Seems like #2464193: Provide configuration options for toolbar menu would end up needing config like this as well...

Here's a quick pass at what a config based approach might look like - interdiff included.

Oops I forgot to add a dependency on Toolbar to my new test module. New patch attached.

FYI: I created follow-up patches in the Admin Toolbar module and Gin theme queues, in case this gets merged (and to give the option to include the patches in composer.json in the interim):

#3186401: Admin Toolbar support for toolbar.settings admin_tray.menu_name

#3186400: Gin support for toolbar.settings admin_tray.menu_name

I performed Novice Triage on this issue. I am leaving the Novice tag on this issue because I think that the issue summary cleanup. The issue summary mentions Beta phase from prior to Drupal 8.0.0 release. I do not think that applies anymore, and there might be further considerations for how to release this.

Also The patch in #55 needs code review and possible manual testing. I tagged for Europe2020.

Thanks for working on this! Nice to see progress on toolbar. A few concerns with the patch:

1. +++ b/core/modules/toolbar/src/Controller/ToolbarController.php
   @@ -72,8 +72,8 @@ public static function preRenderAdministrationTray(array $element) {
   +    $admin_tray_menu = \Drupal::config('toolbar.settings')->get('admin_tray.menu_name') ?: 'admin';
   
   @@ -96,8 +96,8 @@ public static function preRenderGetRenderedSubtrees(array $data) {
   +    $admin_tray_menu = \Drupal::config('toolbar.settings')->get('admin_tray.menu_name') ?: 'admin';
   

   A) We should inject the config service into this controller, not use \Drupal::config().

   B) We should have an update path that sets the default for the new setting, not hard-coding a default at the call sites.

2. +++ b/core/modules/toolbar/tests/modules/toolbar_custom_admin_tray_menu/toolbar_custom_admin_tray_menu.info.yml
   @@ -0,0 +1,7 @@
   +description: 'Support module for testing toolbar with custom admin tray menu.'
   

   "... with a custom admin..." (missing "a").

3. +++ b/core/modules/toolbar/tests/modules/toolbar_custom_admin_tray_menu/toolbar_custom_admin_tray_menu.links.menu.yml
   --- /dev/null
   +++ b/core/modules/toolbar/tests/src/Functional/ToolbarCustomAdminTrayMenuTest.php
   

   I was slightly concerned about adding a whole new test for this, but looking at the existing toolbar functional tests, none of those are a good home for this. So +1 to the new simple test class.

4. +++ b/core/modules/toolbar/tests/src/Functional/ToolbarCustomAdminTrayMenuTest.php
   @@ -0,0 +1,58 @@
   + * Tests overriding the toolbar's admin tray menu via hook_toolbar_alter().
   

   I don't see hook_toolbar_alter() involved anywhere in this test. ;) This comment could use an update.

5. +++ b/core/modules/toolbar/tests/src/Functional/ToolbarCustomAdminTrayMenuTest.php
   @@ -0,0 +1,58 @@
   +   * A user with permission to access the administrative toolbar.
   

   Minor nit: we're trying to move toolbar away from assuming "administrative". The permission is now just "access toolbar". So I'd rather the comment says "access the toolbar".

6. +++ b/core/modules/toolbar/tests/src/Functional/ToolbarCustomAdminTrayMenuTest.php
   @@ -0,0 +1,58 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   

   Why don't we use {@inheritdoc} for this?

   If we keep it manually defined, we probably want string[] not array for the type.

7. +++ b/core/modules/toolbar/tests/src/Functional/ToolbarCustomAdminTrayMenuTest.php
   @@ -0,0 +1,58 @@
   +  protected static $modules = ['toolbar', 'toolbar_custom_admin_tray_menu', 'test_page_test'];
   

   Since this is over 80 chars, would be nice to wrap this onto separate lines.

8. +++ b/core/modules/toolbar/tests/src/Functional/ToolbarCustomAdminTrayMenuTest.php
   @@ -0,0 +1,58 @@
   +  protected function setUp(): void {
   

   Doesn't this want an {@inheritdoc} comment, too?

Thanks again,
-Derek

Addressed #59.

https://www.drupal.org/project/amswap

Hi,

Creating a patch as tried to solve raised fail in #60 comment.

Please review the patch.

Thanks.

Argh, so sorry folks! I completely missed that these functions are public static. Whoops. :( No wonder the tests are failing so spectacularly. MY BAD!! 🤦‍♂️

We need to go back to patch #55 and then make the changes from #59 except 1A (which is what killed everything).

1B is still not addressed, so that needs work, too.

Sorry/thanks/sorry!
-Derek

Hi,

Creating a patch as suggested in #65 comment only excepted 1B point.

Please review the patch.

Thanks.

Can we have a dedicated function, instead of

\Drupal::config('toolbar.settings')->get('admin_tray.menu_name')

something like:

function get_admin_menu_name() {
  $admin_menu_name = \Drupal::config('toolbar.settings')->get('admin_tray.menu_name');
  \Drupal::moduleHandler()->alter('admin_menu', $admin_menu_name);
}

Then a contrib module like amswap can easily replace the admin menu based on user role, or any other condition..
If there is a better/cleaner way to override this config, I am open for suggestions.

I have created a patch for amswap with configuration override, and it seems to work #3189226: Menu swap with toolbar.settings admin_tray.menu_name
So I was wrong in my previous comment about requiring an additional alter.

@Pooja Ganjage: Thanks for moving this forward again.

@Pasqualle: Thanks for making sure this would work for contrib, and confirming we don't need any new alter points or additional API.

Back to 'Needs work' for:

1. +++ b/core/modules/toolbar/src/Controller/ToolbarController.php
   @@ -72,8 +72,8 @@ public static function preRenderAdministrationTray(array $element) {
   +    $admin_tray_menu = \Drupal::config('toolbar.settings')->get('admin_tray.menu_name') ?: 'admin';
   
   @@ -96,8 +96,8 @@ public static function preRenderGetRenderedSubtrees(array $data) {
   +    $admin_tray_menu = \Drupal::config('toolbar.settings')->get('admin_tray.menu_name') ?: 'admin';
   

   #59.1B still needs to be addressed.

2. +++ b/core/modules/toolbar/tests/modules/toolbar_custom_admin_tray_menu/toolbar_custom_admin_tray_menu.info.yml
   @@ -0,0 +1,7 @@
   +  - toolbar
   

   This should be:

     - drupal:toolbar
   

3. +++ b/core/modules/toolbar/tests/src/Functional/ToolbarCustomAdminTrayMenuTest.php
   @@ -0,0 +1,66 @@
   +    // Assert that the "Content" link not present.
   

   missing "is":

   .. link is not present.

Thanks,
-Derek

Hey, I have tried to make add a default value to the config by adding config/install YML. Also cover the points from #70.

Kindly give your feedback.

Thanks, @abhisekmazumdar! That fixes #70.2 and 70.3. However, #59.1B / #70.1 is still unresolved:

1. diff --git a/core/modules/toolbar/config/install/toolbar.settings.admin_tray.yml b/core/modules/toolbar/config/install/toolbar.settings.admin_tray.yml
   new file mode 100644
   index 0000000000..84bb52f0ba
   --- /dev/null
   +++ b/core/modules/toolbar/config/install/toolbar.settings.admin_tray.yml
   @@ -0,0 +1 @@
   +menu_name: 'admin'
   

   This file needs to be named 'toolbar.settings.yml' to match the schema definition for it, and how it's used in the rest of the patch.

   Then, the contents should be:

   admin_tray:
     menu_name: admin
   

   (see other examples later in the patch)

   See these references for more info:
   https://www.drupal.org/docs/drupal-apis/configuration-api
   https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-...
   https://www.drupal.org/docs/drupal-apis/configuration-api/simple-configu...

2. +++ b/core/modules/toolbar/config/install/toolbar.settings.admin_tray.yml
   @@ -0,0 +1 @@
   diff --git a/core/modules/toolbar/config/schema/toolbar.schema.yml b/core/modules/toolbar/config/schema/toolbar.schema.yml
   

   We also need toolbar_update_N() method in toolbar.install to save the default 'admin' for existing sites. Supplying the config/install/toolbar.settings.yml file is only useful for new installations.

Thanks again,
-Derek

@dww Thanks for the feedback, I was thinking to write a hook_update_N but was not sure about it. Your feedback helped me to understand the importance and differences between config/install and hook_update_N.

I have updated the patch. Kindly review.

Great, thanks! Almost there.

1. +++ b/core/modules/toolbar/toolbar.install
   @@ -0,0 +1,16 @@
   + * Set a default value for admin_tray.menu_name.
   

   "Sets a default value for the admin_tray.menu_name setting."

   - Core wants function comments to always start with a verb ending in 's'.
   - Helpful to be explicit this is for a config setting, not a DB column or something.

2. +++ b/core/modules/toolbar/toolbar.install
   @@ -0,0 +1,16 @@
   +function toolbar_update_8001(&$sandbox) {
   

   A) Since this isn't using the Batch API, it doesn't need to declare the $sandbox parameter.

   B) Since this change is only going into 9.2.x at this point, we should probably start this as toolbar_update_9201()

3. The toolbar_update_9201() could have a test. See, for example, core/modules/views/tests/src/Functional/Update/ViewsSettingsRenameTest.php. So we should probably add a core/modules/toolbar/tests/src/Functional/ToolbarSettingsUpdateTest.php that's very similar. Adding the "Needs upgrade path tests" tag for now. You can remove that if you upload a patch that includes the test.

Thanks again!
-Derek

Made the changes 74.1 and 74.2. The patch does not include the test.
Remaining change:
"74.3 The toolbar_update_9201() could have a test. See, for example, core/modules/views/tests/src/Functional/Update/ViewsSettingsRenameTest.php. So we should probably add a core/modules/toolbar/tests/src/Functional/ToolbarSettingsUpdateTest.php that's very similar. Adding the "Needs upgrade path tests" tag for now. You can remove that if you upload a patch that includes the test."

Worked on test mentioned in #74.3, Please have a look.

Fixed the cs issue in the new added test.

Updated tests.

As I see all the code review issues are fixed, and I have successfully used this feature in a contrib theme and module.

This issue landing could have positive impact and pave the way for #3203618: New “content creation” menu proposal

I have an idea, but I can't quite decide if it's worth doing in this issue or moving to a follow-up.

diff --git a/core/modules/toolbar/src/Controller/ToolbarController.php b/core/modules/toolbar/src/Controller/ToolbarController.php
index c94e35b307..77873e3e49 100644
--- a/core/modules/toolbar/src/Controller/ToolbarController.php
+++ b/core/modules/toolbar/src/Controller/ToolbarController.php
@@ -67,13 +67,14 @@ public function checkSubTreeAccess($hash) {
    */
   public static function preRenderAdministrationTray(array $element) {
     $menu_tree = \Drupal::service('toolbar.menu_tree');
+
     // Load the administrative menu. The first level is the "Administration"
     // link. In order to load the children of that link, start and end on the
     // second level.
     $parameters = new MenuTreeParameters();
     $parameters->setMinDepth(2)->setMaxDepth(2)->onlyEnabledLinks();
-    // @todo Make the menu configurable in https://www.drupal.org/node/1869638.
-    $tree = $menu_tree->load('admin', $parameters);
+    $admin_tray_menu = \Drupal::config('toolbar.settings')->get('admin_tray.menu_name');
+    $tree = $menu_tree->load($admin_tray_menu, $parameters);
     $manipulators = [
       ['callable' => 'menu.default_tree_manipulators:checkAccess'],
       ['callable' => 'menu.default_tree_manipulators:generateIndexAndSort'],
@@ -96,8 +97,8 @@ public static function preRenderGetRenderedSubtrees(array $data) {
     // levels, start at the second level and end at the fourth.
     $parameters = new MenuTreeParameters();
     $parameters->setMinDepth(2)->setMaxDepth(4)->onlyEnabledLinks();
-    // @todo Make the menu configurable in https://www.drupal.org/node/1869638.
-    $tree = $menu_tree->load('admin', $parameters);
+    $admin_tray_menu = \Drupal::config('toolbar.settings')->get('admin_tray.menu_name');
+    $tree = $menu_tree->load($admin_tray_menu, $parameters);
     $manipulators = [
       ['callable' => 'menu.default_tree_manipulators:checkAccess'],
       ['callable' => 'menu.default_tree_manipulators:generateIndexAndSort'],

I think it would be quite useful if we could wrap these two lines up into a utility method in the ToolbarController.

My reasoning here is thatt, I want to reuse ToolbarController in my custom module to change which menu is used in the toolbar in specific cases (in my case that's if the user has a specific role). Right now I have to override, basically everything in the ToolbarController, copying and pasting all of the code in the relevant functions from ToolbarController into my custom controller classs, simply to change one line. That's a lot of, essentially, duplicate code just to change one line.

My thinking is that, if we have a separate function in the ToolbarController class, that basically just gets the menu and returns it, then when I extend ToolbarController in my custom controller, all I need to do is override that one small function to return the right menu, leaving everything else as is.

From my perspective that's a small change, for a lot of gain, and from a maintainability and extensibility perspective, breaking things up into smaller functions is a good thing.

Doing this would also benifit other contrib modules which modify the toolbar, but basically have to do the same thing I just described in other to supply a different menu tree for rendering.

Thanks,
-Aaron

@AaronMcHale: That's a lovely suggestion. I think we should do it here as part of this issue if possible. Hopefully this isn't considered scope creep. 🤞

In addition to an @internal getAdminTrayMenuName() method, I noticed a lot of code duplicated between preRenderAdministrationTray() and preRenderGetRenderedSubtrees(). In a nod to DRY, I moved all that into a shared getAdminTrayMenuTree() method, too. Hope that's agreeable.

Whoops, I missed the fact that the two usages render different depths. Added the min and max as params for getAdminTrayMenuTree() and now all the toolbar tests pass locally. Let's see if the bot agrees. ;)

p.s. Should be obvious, but note: I had to make these new methods public static since they're called from static methods. There's no $this involved, so we can't make these protected methods.

🤦‍♂️ 3rd patch is the charm? 😉

Nope. Maybe 4? 😉 I realized that I should have read those comments more closely and left them at the call sites. The comment describe two different scenarios, so they should stay where they were originally. Hopefully this is pretty close to RTBC now...

Thanks @dww

In preRenderAdministrationTray() it looks like $menu_tree = \Drupal::service('toolbar.menu_tree'); may now no longer be used, so as far as I can tell that line can also be removed.

I'm wondering if there's a specific reason you made getAdminTrayMenuName() and getAdminTrayMenuTree()?

For instance, if we make getAdminTrayMenuTree() not static, we could add this too the top of the class:

  /**
   * @var \Drupal\toolbar\Menu\ToolbarMenuLinkTree
   */
  protected $menuTree;

  public static function create(ContainerInterface $container) {
    $instance = parent::create($container);
    $instance->menuTree = $container->get('toolbar.menu_tree');
    return $instance;
  }

In doing so, we no longer need multiple instances, and no longer need to make static calls to \Drupal::service(). Which seems like a win win to me.

Oh and also, I'm wonder if we should take the opportunity to replace the usage of the deprecated REQUEST_TIME for the line $expires->setTimestamp(REQUEST_TIME + $max_age);.

Thanks,
-Aaron

Edit:

I completely missed comment #89 and for some reason didn't notice that preRenderAdministrationTray() and preRenderGetRenderedSubtrees() are static methods.

I do wonder if there's a reason though that we can't just make everything part of the instance, it appears to be technically possible for trusted callbacks to be instances, according to the code in DoTrustedCallbackTrait::doTrustedCallback(). However, that does sound like too much of a scope creep from what this issue is aiming to do, so probably best to just leave it as is or push to a follow-up.

Thanks,
-Aaron

@AaronMcHale re: #92: Yeah, it's confusing. I made exactly the same mistake at #59. See #65. Agreed it's out of scope to fix it all here. Needs followup: #3217746: Convert static methods in ToolbarController to be instance methods 😉

Also out of scope to touch REQUEST_TIME here. There's already got to be an open issue about that. If we were introducing that here, I'd say we should Do It Right(tm), but the current patch doesn't touch that code at all.

Thanks,
-Derek

Thanks for opening that as a follow-up @dww 😉.

Also out of scope to touch REQUEST_TIME here. There's already got to be an open issue about that. If we were introducing that here, I'd say we should Do It Right(tm), but the current patch doesn't touch that code at all.

Yeah I thought that was a big piece of work somewhere, did a dig just now and found #2902895: [meta][no patch] Replace uses of REQUEST_TIME and time() with time service, looks like this child issue is covering it #3112298: Replace REQUEST_TIME in classes with direct container access, so we're all good on that front.

Just want to try the new methods that @dww added (which will hopefully do today), assuming they work as expected for my use case, then I think everything is good from my end 😉.

Alright, new methods work as expected and meet the requirements, thanks @dww.

Back to RTBC 😄

I think it might be unnecessary to override the toolbar controller just to change the menu. I have used config override to change the menu (based on user role) see #3189226: Menu swap with toolbar.settings admin_tray.menu_name.
It might be problematic if multiple contrib modules try to override the toolbar controller, but to have it as an option is really good. I like the new dedicated getAdminTrayMenuName() function.

@Pasqualle my specific use case is within a custom profile/distribution, so it's all very controlled in my case, but I agree that if you have multiple contrib modules installed that override the ToolbarController that could be problematic. That is of course an existing problem and if anything this issue probably helps to mitigate that slightly (providing more flexibility for specific implementations).

Thanks,
-Aaron

I think we should have subsystem maintainer signoff on this change since it is in some ways a significant change to how the Toolbar module works.

This is also tagged as needing manual testing and an IS update. We should either do those things, or remove the tags if they're no longer relevant. Thanks!

Some small things to fix:

1. +++ b/core/modules/toolbar/src/Controller/ToolbarController.php
   @@ -140,4 +124,47 @@ public static function trustedCallbacks() {
   +   * @internal
   ...
   +   * @internal
   

   @internal should typically come with an explanation of why it's internal (what the intended use is and isn't, or if it belongs to a class of things that are always internal, etc.).

2. +++ b/core/modules/toolbar/src/Controller/ToolbarController.php
   @@ -140,4 +124,47 @@ public static function trustedCallbacks() {
   +    return \Drupal::config('toolbar.settings')->get('admin_tray.menu_name');
   ...
   +    $menu_tree = \Drupal::service('toolbar.menu_tree');
   

   Why are we not injecting these services?

3. +++ b/core/modules/toolbar/toolbar.install
   @@ -0,0 +1,16 @@
   + * Sets a default value for the admin_tray.menu_name setting.
   

   Update hooks should start with imperative rather than indicative ("Set" rather than "Sets"), because they are shown in the UI. Reference: https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...

4. +++ b/core/modules/toolbar/toolbar.install
   @@ -0,0 +1,16 @@
   +function toolbar_update_9201() {
   +  \Drupal::service('config.factory')
   +    ->getEditable('toolbar.settings')
   +    ->set('admin_tray.menu_name', 'admin')
   +    ->save();
   +}
   

   Should be 9301, but also should maybe be a post-update instead?

Probably would also merit a CR for the addition.

So about the overarching issue of adding a config: I'm still on #2464193-14: Provide configuration options for toolbar menu essentially the toolbar isn't meant to have configuration.

The IS talks about a need for contrib/custom modules to be able to change the menu used, a way to change that with a hook is proposed but runs into an issue with the module assumptions baked into the code. Then a config option is proposed to work around that limitation with no UI for site builders to change the value (because it's for custom projects/contrib).

So we're not adding a configuration because it's a good idea to have configuration, we're adding a configuration because we need a "global" variable to work around the limitation of the module. And in doing so we're adding a "hidden" feature that's not too easy to document since there is no UI and as far as I know config values don't show up on api.drupal.org, so I'm not sure folks outside of this issue would know this exists without looking at the code or in details what their config export contains.

Personnally I would go back to #49 and fix the PHP api to allow passing/inheriting the value of the menu around the different methods/ajax calls. That way we get a code feature documented in code for developers. Adding a config value without a UI seems like a bad call.

At this time the reasoning outlined in the IS and the technical implementation doesn't match up for me, happy to be convinced otherwise.

@nod_ re #100:

My personal stake in this is from the point of view of customising the toolbar in code, changing what the "primary" menu is based on specific circumstances.

Following on from what you've said, I think there's real value in making the ToolbarController itself more extensible, a number of contrib modules (and in my case a custom module) have to essentially copy and paste a huge amount of code from ToolbarController, simply so that they can change which menu is displayed or add additional menus to the Toolbar.

That's obviously not a sustainable state to be in; For my use-case, detailed in comment #85, I proposed a solution which @dww then implemented in a patch. That solution may go far enough to solving that problem for contrib and custom modules, as well as addressing your concerns. I also like the idea you mentioned about being able to pass the menu in the Ajax call, but I think that's less flexible in some ways, because in my case the menu I'm substituting does not have an equivalent top-level "Administration" item, so in my ToolbarController (which extends the Core one), I'm also changing the depth parameters that it uses in addition to changing the menu name (that's also probably a common use-case).

So I would really encourage us to continue down these approaches, either in this issue or in a fresh one.

An additional thing to mention, is ToolbarController has a hardcoded dependency on the admin menu name, for something that is (in theory) configurable, I think it woudd be desirable to not have that hardcoded dependency.

With that in mind, and looking at the overall approach more generally, I guess the thing I'm thinking is that, is the approach taken in this issue the most useful approach for what we want to achieve. My gut feeling is actually that this approach falls short of what would be considered the most useful implemention. It strikes me that, perhaps a more useful approach, would be to allow each menu to specify whether it should be included in the Toolbar. That would mean that Toolbar itself wouldn't have a configuration screen, it would instead inject that boolean option into each menu, and probably would be more useful in more cases, rather than still being limited to only one administration menu in Core. For example, #3203618: New “content creation” menu proposal may need that kind of flexibility in order to work.

Thanks,
-Aaron

Clarifiying that this is a feature request. As of today the Toolbar module is "feature complete" meaning it does the job it's designed to do. Having the admin menu hardcoded is to ensure there is a way to navigate the numerous Drupal admin pages.

It is possible in contrib/custom to add new elements to the toolbar, including menus. I see that very frequently in client projects so it's nothing new.

As outlined in #100 the current implementation is problematic, additionally there is nothing preventing contrib from implementing this feature. This is also not a blocker for the new content creation proposal, a new toolbar item can be declared to expose this new menu in the toolbar (and I'm pretty sure that new menu shouldn't replace the admin menu, otherwise how your administrators are going to navigate the admin pages)

Given all this, as the toolbar maintainer my position is that there are 2 solutions:

1. We update the PHP to make the approach in #9 workable, giving module developers a programmatic way of swapping the menu being displayed
2. Close this feature request as won't fix

Leaving as need work if someone wants to pick this up and implement solution #1 otherwise I'll be closing this in a few weeks.

Re @nod_ in #102.

I think what you've said makes sense, however regarding:

We update the PHP to make the approach in #9 workable, giving module developers a programmatic way of swapping the menu being displayed

I think #9 is a good idea, I'd still like to see the improvements to ToolbarControllr as described in #101 and previously implemented in the patch. The solution proposed in #9, while nice, doesn't give quite as much flexibility as I personally (and possibly other contrib modules) would need. Specifically (as I described) relating to altering the min/max depth to better suit the specific menu that is being included.

Of course, another option there would be to adapt #9 to give that flexibility, which would work for me, but I still like the idea of breaking up the functions in ToolbarController a little bit because it makes it just that bit more maintainable and extensible.

Thanks,
-Aaron

I understand it doesn't handle the depth issue but it is out of scope to handle this in core. Same things as menus, you can't choose depth for core menus blocks which is one of the reason why you have the menu_block module in contrib. Same thing applies here, contrib is a viable solution it doesn't need to live in core (also will take way longer to get in core than to make a working contrib module for it).

I understand it doesn't handle the depth issue but it is out of scope to handle this in core. Same things as menus, you can't choose depth for core menus blocks which is one of the reason why you have the menu_block module in contrib. Same thing applies here, contrib is a viable solution it doesn't need to live in core (also will take way longer to get in core than to make a working contrib module for it).

Okay I'm not suggesting we build a UI for configuring that, what I'm getting at is that I don't think it's necessary to expect contrib to deal with the issue of being able to control the depth if we're making it possible to control the menu name. The admin menu is a bit odd in that all of the top level links are one level down under the "Administration" link, most menu structures wouldn't take that approach, you would have all of the top level items actually at the top level of the menu tree.

Right now, in my case, I have to essentially copy and paste ToolbarController because there's no way to pass in the menu name and the depth parameters. Now, if we make it possible to pass in the menu name, we're only solving half of that problem there, I would still need a copy of ToolbarController so that I can change the minDepth from 1 to 0. If the recommendation is to design a contrib module to handle that, the contrib module is still going to need to copy and paste everything from ToolbarController, so that when the depth is specified in the render array, it can build in logic to deal with that. So we're only pushing the problem one level down the stack, which is why contrib is not a solution here. The example of menu_block,

I think is different, because that's providing a feature complete UI and points for the user to interact. If we make this improvement, a similar module could then easily be build to provide a UI to control the depth; But right now it can't do that in a maintainable way.

Even if we just do #9 in this issue, yeah that's still an improvement, but it would be a half-baked improvement if we don't at very least provide the ToolbarController::getAdminTrayMenuName() and ToolbarController::getAdminTrayMenuTree() methods. Which, again, I would argue we should do because it breaks up what is otherwise one big function into three smaller more extensible ones.

Hope that all makes sense,
Thanks,
-Aaron

Then a config option is proposed to work around that limitation with no UI for site builders to change the value (because it's for custom projects/contrib).
...
we're adding a "hidden" feature that's not too easy to document since there is no UI

This would not be the first case, where we have a config value without a UI in module.settings.yml file. I have just checked locale.settings.yml and found multiple config values without a UI. All are documented have some description in config schema.

there is nothing preventing contrib from implementing this feature.

The problem starts when 2 contrib modules need to replace the #pre_render function to do their own things, like amswap and admin_toolbar. The modules currently can't work together. They need a common config value to solve the conflict.

As I see Drupal core should provide the common config value. That seems like the best solution here.

This would not be the first case, where we have a config value without a UI in module.settings.yml file. I have just checked locale.settings.yml and found multiple config values without a UI. All are documented have some description in config schema.

It's up to the maintainers to accept what they're ready to maintain or not, I'm personally not going to support a hidden config value in the toolbar module. If config values could be more discoverable why not, it doesn't change that a config value here feels like a workaround.

The modules currently can't work together.

For me that's a compelling argument for introducing a change.

There are a few possibilities here:

1. We fix the PHP so that #9 can be implemented cleanly from the code
2. Someone steps up to become a toolbar maintainer and agree to support this type of hidden config values/workaround, knowing that the toolbar was explicitly designed to not have any config values (so unforeseen problems may arise)

For me the situation is the same as it was in #100, proposed solution and Issue Summary doesn't match.

I agree, the issue summary needs an update, as the proposed solution is not good enough. Multiple modules using the alter hook will not work.

Just found another very good example of a hidden config, which can be managed in a contrib module.
https://www.drupal.org/project/flood_control

FWIW, I’m -1 on @nod_’s position that toolbar wasn’t intended to need config, therefore, we shouldn’t have config. I’ve already provided a (growing) list of places where toolbar needs some configuration. Toolbar was originally designed to *only* support admins, but I’ve got a bunch of real prod sites where the toolbar is enabled for almost all auth users. That’s not “intended”, but it’s a vastly better UI than nothing. With a few patches, toolbar works great for non-admins. But it’s not “feature complete” out of the box, since there are too many assumptions baked into it.

Drupal is full of examples of things designed for one use case being used for others.

“We didn’t think we needed config, so we should never add any config” is counter to how Drupal historically evolves. We’ve got the reasons why we need this. Please don’t shoot it down just because it’s not what was originally intended.

If you’re saying, basically “I refuse to support this and your only option is to take over as subsystem maintainer and I quit”, that’s a pretty harsh ultimatum. That’s definitely how the end of 108 reads.

I’m not sure hidden config is a good idea. From what I can tell, people are only proposing it as hidden because you’re opposed to any configuration at all.

I know we’re all doing the best we can with our limited time. I don’t mean to be harsh or a jerk. But I wanted to publicly register my opposition to how this is being shot down on the principle of no-config-for-toolbar and the idea that toolbar is “feature complete”.

I can see that my wording could be understood that way, so to be clearer: I am not planning on stepping down and leaving the module to the person who raises their hand, it's about adding an additional maintainer so that the maintenance burden is shared. Adding additional maintainers to support new/broader use-cases is pretty usual, which is what I'm proposing. made a more detailed response in #2464193-22: Provide configuration options for toolbar menu so we can continue this part of the discussion there.

Overall about how this issue went, I was happy with how it started, I don't remember why I didn't check back on how things went after #43, so I missed #53 and the new direction proposal until xjm pinged me in #99. The fact that I didn't look at the issue and a lot of work was done on the alternative approach to get it to RTBC is on me, I'm sorry about that. A lot of time could have been saved if I paid closer attention.

On the implementation, I'm still of the opinion that the more appropriate way of handling it is the initial direction, which would help clean up our code instead of adding another layer on top of it to sidestep the problem which is the lack of flexibility of the PHP implementation.

Fixed failed the test of patch #91.

Updated patch will resolve test failures.

Create patch for Drupal 10.1.x-dev
Please review the patch. Thank You