Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem
hook_menu_local_tasks_alter()
has an unnecessarily complex structure, caused by an unused'count'
array key.hook_menu_local_tasks_alter()
is primarily used to add, not to alter.MENU_DEFAULT_LOCAL_TASK
does not default to a weight of -10. It has to be specified manually.- Various local tasks and actions throughout core declare weights, which make it unnecessarily hard for contrib to squeeze a link in between.
- Various local tasks involve a final "Settings" task, which is supposed to come last, but core modules do not define a consistent weight for them.
Goal
- Simplify usage of menu local tasks and actions as well as MENU_DEFAULT_LOCAL_TASK.
Proposed solution
- Remove the unnecessary
'count'
array sub-key fromhook_menu_local_tasks_alter()
. - Complement
hook_menu_local_tasks_alter()
withhook_menu_local_tasks()
, so modules can add, before others try to alter. - Make type
MENU_DEFAULT_LOCAL_TASK
default to a weight of -10. - Adjust weights of local tasks to provide sufficient room for contrib to inject links in between.
- Introduce a non-formalized weight of 100 for typical "Settings" local tasks to aid
hook_menu_local_tasks()
implementations and help to ensure they come last.
Related issues
Comment | File | Size | Author |
---|---|---|---|
#39 | menu.local-tasks.38.patch | 43.92 KB | sun |
#35 | menu.local-tasks.35.patch | 43.94 KB | sun |
#29 | menu.local-tasks.29.patch | 43.92 KB | sun |
#21 | interdiff.txt | 867 bytes | tim.plunkett |
#21 | menu-1864066-21.patch | 44.61 KB | tim.plunkett |
Comments
Comment #1
sunWhoopsie, removed stray debugging code, which was totally unrelated to this patch. ;)
Comment #3
sunApparently, there are still things in the menu system that I still don't understand.
Comment #4
fubhy CreditAttribution: fubhy commented+1 for simplifying adding / altering local tasks. Also +1 on making that a proper hook instead of an alter. We could use some tests for adding / altering tasks.
Comment #5
sunHm, not sure about that... Some of the existing dynamically added local tasks are covered by tests already. Likewise, MENU_DEFAULT_LOCAL_TASKs also have test coverage already.
I think all this patch is really doing is to slightly refactor the existing code to simplify their usage. It doesn't really change the existing expectations.
In general though, we're lacking sophisticated (unit-ish) test coverage for this functionality. At the same time, I don't really know what's going to happen with
hook_menu()
for D8 in light of the new routing system (there's still zero sign of progress on that front as of now).As a result, while we could try to introduce more test coverage here, there's a ~50% chance that it might become obsolete.
(Again, I have no idea at all on the fate of hook_menu(), local tasks, etc.pp. - it might not really change, but it might also cease to exist... the sky is the limit. AFAIK, there's not even a plan or roadmap, nor a meta issue. [which is really concerning, given where we are in the release cycle...])
A compromise might be to add a very basic test that focuses on the most basic expectations to ensure that modules are able to add + alter local tasks and actions, which I guess could be done relatively quickly.
Lastly, please note that this is only a first step, as a preparation for a second step for the related issues linked in the summary, which all share the need for a tab expansion facility (which is not part of this issue/patch).
Comment #6
sunComment #8
sunFixed CommentApprovalTest.
Comment #9
BerdirPatch looks great, nice simplification and separation of that hook.
Yes, that is concerning :(
I think that the new hook could be the answer to the local task part. So that we could just remove them from hook_menu() and define in this hook instead. One thing that worries me a bit about that is performance, these hooks are invoked on every page request then and all implementations need to check if they need to add something like this:
Even if it's fast enough, the code for it is unecessary complicated.
AFAIK, these implementations are in almost all cases specific to a given route/path. So I'm wondering if we could tie them to that more explicitly. Given that we have nested local tasks and they have their own route, it might not be that easy. Suggestions?
Comment #10
sunHm. @Berdir implies a much larger meaning to the new hook in #9 than actually intended (for now).
For now, the proposed new
hook_menu_local_tasks()
is only meant to complement the existing alter hook, since the alter hook is primarily used for adding new tasks instead of altering them, which makes it hard(er) for other modules to actually alter local tasks; i.e., add vs. alter.There have been ideas and discussions in the past to actually move tabs out of
hook_menu()
entirely — however, this issue + patch is definitely NOT meant to get into that epic discussion and spiral of death. ;)All this issue wants to achieve is a simplification of the current API for its consumers and most common/typical use-cases. I admit that it could potentially be a cornerstone towards a revised tabs/tasks API, but that's not the immediate goal and scope of this issue. :)
Regarding the tab root path condition:
Please note that only the testing module implementation uses a
strpos()
condition, since it wants to dynamically add + alter local tasks on three different sub-pages.Most of the real world implementations I've seen thus far work perfectly fine by checking either the passed-in
$root_path
, or by consulting the tab root path via$router_item['tab_root']
instead, and they can compare that directly, not needing astrpos()
at all.Comment #11
sun#8: menu.local-tasks.8.patch queued for re-testing.
Comment #13
sunMerged HEAD.
Comment #14
sun#13: menu.local-tasks.13.patch queued for re-testing.
Comment #15
Berdir@sun: I didn't mean to discuss that here, was just writing down my thoughts. I completely agree that we should do that in here, just meant that it could possible provide the basics for doing that in
Looks like nobody else is looking at this. I think this is good to go and a great clean-up. So, RTBC.
Comment #16
sun#13: menu.local-tasks.13.patch queued for re-testing.
Comment #18
andypostRe-roll
Comment #19
sunBack to RTBC based on #15
Comment #20
catchThis should use the new module handler now.
Patch looks great otherwise.
Comment #21
tim.plunkettPatch context conflicted with #1862752: Implement entity access API for users.
Fixed the module_implements, and the alter 3 lines later.
Comment #22
catchComment #24
andypostIt seems that #1901546: Random failures in Drupal\system\Tests\Entity\ConfigEntityQueryTest needs fixed first
Comment #25
sun#21: menu-1864066-21.patch queued for re-testing.
Comment #26
BerdirPassed again, back to RTBC.
Comment #27
sun#21: menu-1864066-21.patch queued for re-testing.
Comment #29
sunMerged HEAD + #21.
Comment #30
sun#29 is a plain re-roll, so back to RTBC.
Comment #31
catchWhy not invokeAll() here?
Looks good otherwise.
Comment #32
sun$data is passed by reference.
Comment #33
catch#29: menu.local-tasks.29.patch queued for re-testing.
Comment #35
sunMerged HEAD.
Comment #36
Dries CreditAttribution: Dries commentedI was going to commit this patch but it looks like a re-roll is required. Asking for a re-test.
Comment #37
Dries CreditAttribution: Dries commented#35: menu.local-tasks.35.patch queued for re-testing.
Comment #39
sunMerged HEAD. (no conflicts)
Comment #41
Berdir#39: menu.local-tasks.38.patch queued for re-testing.
Comment #42
sunComment #43
Dries CreditAttribution: Dries commentedLooked at this twice now and I think it is good to go. It's a nice simplification and the added tests are welcome too.