Beta phase evaluation
Issue category | Feature because it enables a new possibility: it's necessary for contrib/custom modules to use a different menu as the admin menu in the toolbar, possibly even using a different menu on a per-role basis. |
---|---|
Disruption | Zero disruption. |
Problem/Motivation
Any module can add an item to the toolbar. (So far, the Shortcut, Toolbar, and User modules are the only ones in core that do.) The site builder should be able to decide which ones to include, and how to order them.
Proposed resolution
The Toolbar module should not hardcode the menu to be used; it should be possible for a contrib module to alter which menu is loaded by using hook_toolbar_alter()
.
Remaining tasks
Nothing.
User interface changes
None.
API changes
None; only a render array structure change.
Comment | File | Size | Author |
---|---|---|---|
#116 | 1869638-116.patch | 10.55 KB | gaurav-mathur |
#113 | interdiff-1869638-112-113.txt | 725 bytes | yogeshmpawar |
#113 | 1869638-113.patch | 10.55 KB | yogeshmpawar |
| |||
#112 | interdiff_91-112.txt | 1.01 KB | ravi.shankar |
#112 | 1869638-112.patch | 10.55 KB | ravi.shankar |
Comments
Comment #1
benjifisherAdd issue tags.
Comment #2
Bojhan CreditAttribution: Bojhan commentedI actually have no idea, why we would do this at all. This seems like something you want to extend to in contrib, not core.
Comment #3
johnvFor example, passing the menu_name would be a nice option.
Currently, toolbar_view() does the following:
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.
Comment #4
benjifisher@Bojhan:
One reason to do it in core is to avoid having multiple modules trying to do the same thing: change the order of toolbar items using
hook_toolbar_alter()
to modify the'#weight'
properties. Whichever module has the lowest weight (or is it highest?) will "win." If it is done in core, then we can discourage contrib modules from fighting over it.As a site builder, I want to be able to control my site's toolbar. I guess not everyone feels that way.
Add the Edit module to the list of core modules that define toolbar items.
@johnv:
I marked this issue as postponed because the API is still in flux. I am happy to continue discussing it, but I still think it is too early to propose patches.
How much would we have to add to make the Commerce Kickstart Profile's extra module unnecessary?
Comment #5
Bojhan CreditAttribution: Bojhan commented@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.
Comment #6
benjifisher@Bojhan:
One difference is that the D8 toolbar is a lot better than the one for D7.
Comment #7
johnv@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:
Comment #8
benjifisher@johnv:
Basically, what your code snippet does is rip out the core toolbar and replace it with the one defined by MYMODULE. (This would also rip out any toolbar elements defined by other modules.) What I would like to do is make the toolbar flexible enough that you do not have to do anything so drastic.
Personally, I think that the Menu module should determine the menu that is shown in the toolbar, but for now that code is in the Toolbar module. If another module wants to put a different menu in the toolbar, it could do it by implementing hook_toolbar() (see below). Then, if we adopt my suggestion in this issue, the site builder can decide whether to keep the core-provided admin menu in the toolbar, replace it with the contrib-provided menu, or keep both.
I have not yet looked at the issues you mention, but I will. I do not want to resolve all of them here, but I would like to make sure that the solution we adopt here and in #1847198: Update the structure returned by hook_toolbar() is flexible enough to support whatever work is being done on those.
Here is an extract from the current proposal in #1847198 for the Toolbar module:
Yes, I left out the hard part, which is how to define the render array
$menu
. Any module that wants to do a better job will have to figure that out.Comment #9
Wim Leers+1
+1
So let's not introduce such a UI. That's contrib material.
+1
We can do that quite easily now! Basically, change
to:
And update
toolbar_prerender_toolbar_administration_tray()
to look at#menu
and use that as the menu to be rendered.Comment #10
Wim LeersComment #11
dan_lennox CreditAttribution: dan_lennox commentedTrying patch as per #9
Comment #12
Deciphered CreditAttribution: Deciphered commentedI 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.
Comment #13
dan_lennox CreditAttribution: dan_lennox commentedThanks Deciphered, suggestion implemented.
Comment #14
dan_lennox CreditAttribution: dan_lennox commentedComment #15
Wim LeersThanks for taking this on!
The documentation of this function should be updated to mention
#menu
.Just assume
#menu
exists. No need to provide a fallback. This line can be deleted entirely.Comment #16
Deciphered CreditAttribution: Deciphered commented@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.
Comment #17
Wim LeersWe like to say:
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.
Comment #18
dan_lennox CreditAttribution: dan_lennox commentedThanks Wim and Deciphered, been great to learn some insight into how we approach this kind of scenario!
The fallback line has now been removed.
Comment #20
Wim LeersYou 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.
Comment #21
dan_lennox CreditAttribution: dan_lennox commentedOh wow... sorry about that!
Comment #22
Wim Leers:) No problem. Thank you very much!
Comment #23
alexpottSo this patch does not really look like a feature anymore. Also
the patch does not do this...
Comment #24
Wim LeersThat'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.
Comment #25
alexpottThis allows contrib to use toolbar better and is not disruptive. But we should have test coverage of this functionality.
Comment #26
Wim LeersMarked #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.
Comment #27
Wim LeersComment #29
temkin CreditAttribution: temkin as a volunteer commentedThere 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.
Comment #30
tstoecklerComment #35
firewaller CreditAttribution: firewaller commented@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.
Comment #36
firewaller CreditAttribution: firewaller commentedHere'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.
Comment #37
jeni_dc CreditAttribution: jeni_dc commentedI 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?
Comment #38
firewaller CreditAttribution: firewaller commented@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.
Comment #43
nod_Needs a reroll
Comment #44
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedComment #45
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch for 9.1.x, please review.
Comment #46
dwwAdding this as another possible use-case for #2464193: Provide configuration options for toolbar menu
Comment #48
m.stentaSetting this back to "Needs Work" per comment in #36:
It seems that this needs to be addressed in order for this to work.
Comment #49
m.stentaAttached is a patch that includes tests.
Comment #51
m.stentaI'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
fromhook_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 levelsystem.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.
Comment #52
m.stentaIt 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 intoolbar_get_rendered_subtrees()
(or ultimatelypreRenderGetRenderedSubtrees
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
Is this worth considering? Seems like #2464193: Provide configuration options for toolbar menu would end up needing config like this as well...
Comment #53
m.stentaHere's a quick pass at what a config based approach might look like - interdiff included.
Comment #55
m.stentaOops I forgot to add a dependency on Toolbar to my new test module. New patch attached.
Comment #56
m.stentaComment #57
m.stentaFYI: 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
Comment #58
mradcliffeI 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.
Comment #59
dwwThanks for working on this! Nice to see progress on toolbar. A few concerns with the patch:
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.
"... with a custom admin..." (missing "a").
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.
I don't see hook_toolbar_alter() involved anywhere in this test. ;) This comment could use an update.
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".
Why don't we use {@inheritdoc} for this?
If we keep it manually defined, we probably want string[] not array for the type.
Since this is over 80 chars, would be nice to wrap this onto separate lines.
Doesn't this want an {@inheritdoc} comment, too?
Thanks again,
-Derek
Comment #60
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedAddressed #59.
Comment #61
Pasquallehttps://www.drupal.org/project/amswap
Comment #63
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedHi,
Creating a patch as tried to solve raised fail in #60 comment.
Please review the patch.
Thanks.
Comment #64
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedComment #65
dwwArgh, 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
Comment #66
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedHi,
Creating a patch as suggested in #65 comment only excepted 1B point.
Please review the patch.
Thanks.
Comment #67
Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commentedComment #68
PasqualleCan we have a dedicated function, instead of
something like:
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.
Comment #69
PasqualleI 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.
Comment #70
dww@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:
#59.1B still needs to be addressed.
This should be:
missing "is":
.. link is not present.
Thanks,
-Derek
Comment #71
abhisekmazumdarHey, 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.
Comment #72
dwwThanks, @abhisekmazumdar! That fixes #70.2 and 70.3. However, #59.1B / #70.1 is still unresolved:
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:
(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...
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
Comment #73
abhisekmazumdar@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.
Comment #74
dwwGreat, thanks! Almost there.
"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.
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()
core/modules/views/tests/src/Functional/Update/ViewsSettingsRenameTest.php
. So we should probably add acore/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
Comment #75
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedMade 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."
Comment #77
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedWorked on test mentioned in #74.3, Please have a look.
Comment #78
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedFixed the cs issue in the new added test.
Comment #80
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedUpdated tests.
Comment #81
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedComment #83
PasqualleAs I see all the code review issues are fixed, and I have successfully used this feature in a contrib theme and module.
Comment #84
AaronMcHaleThis issue landing could have positive impact and pave the way for #3203618: New “content creation” menu proposal
Comment #85
AaronMcHaleI have an idea, but I can't quite decide if it's worth doing in this issue or moving to a follow-up.
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
Comment #86
dww@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 betweenpreRenderAdministrationTray()
andpreRenderGetRenderedSubtrees()
. In a nod to DRY, I moved all that into a sharedgetAdminTrayMenuTree()
method, too. Hope that's agreeable.Comment #88
dwwWhoops, 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. ;)Comment #89
dwwp.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.Comment #90
dww🤦♂️ 3rd patch is the charm? 😉
Comment #91
dwwNope. 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...
Comment #92
AaronMcHaleThanks @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()
andgetAdminTrayMenuTree()
?For instance, if we make
getAdminTrayMenuTree()
not static, we could add this too the top of the class: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()
andpreRenderGetRenderedSubtrees()
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
Comment #93
dww@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
Comment #94
AaronMcHaleThanks for opening that as a follow-up @dww 😉.
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 😉.
Comment #95
AaronMcHaleAlright, new methods work as expected and meet the requirements, thanks @dww.
Back to RTBC 😄
Comment #96
PasqualleI 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.
Comment #97
AaronMcHale@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
Comment #98
xjmI 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:
@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.).Why are we not injecting these services?
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...
Should be 9301, but also should maybe be a post-update instead?
Comment #99
xjmProbably would also merit a CR for the addition.
Comment #100
nod_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.
Comment #101
AaronMcHale@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
Comment #102
nod_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:
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.
Comment #103
AaronMcHaleRe @nod_ in #102.
I think what you've said makes sense, however regarding:
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
Comment #104
nod_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).
Comment #105
AaronMcHaleOkay 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
Comment #107
PasqualleThis 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 documentedhave some description in config schema.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.
Comment #108
nod_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.
For me that's a compelling argument for introducing a change.
There are a few possibilities here:
For me the situation is the same as it was in #100, proposed solution and Issue Summary doesn't match.
Comment #109
PasqualleI 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
Comment #110
dwwFWIW, 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”.
Comment #111
nod_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.
Comment #112
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedFixed failed the test of patch #91.
Comment #113
yogeshmpawarUpdated patch will resolve test failures.
Comment #114
PasqualleComment #115
gaurav-mathur CreditAttribution: gaurav-mathur at Dotsquares Ltd. commentedComment #116
gaurav-mathur CreditAttribution: gaurav-mathur at Dotsquares Ltd. commentedCreate patch for Drupal 10.1.x-dev
Please review the patch. Thank You