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

  1. Remove the unnecessary 'count' array sub-key from hook_menu_local_tasks_alter().
  2. Complement hook_menu_local_tasks_alter() with hook_menu_local_tasks(), so modules can add, before others try to alter.
  3. Make type MENU_DEFAULT_LOCAL_TASK default to a weight of -10.
  4. Adjust weights of local tasks to provide sufficient room for contrib to inject links in between.
  5. 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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Whoopsie, removed stray debugging code, which was totally unrelated to this patch. ;)

Status: Needs review » Needs work

The last submitted patch, drupal8.menu-local-tasks.1.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
452 bytes
33.74 KB

Apparently, there are still things in the menu system that I still don't understand.

fubhy’s picture

Issue tags: +Needs tests

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

sun’s picture

We could use some tests for adding / altering tasks.

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

sun’s picture

FileSize
14.62 KB
44.4 KB
  1. Added tests.
  2. Changed menu_local_tasks() structure to be keyed by link href to improve DX of hooks.
  3. Fixed tabs markup container is rendered even if there are no tabs.

Status: Needs review » Needs work

The last submitted patch, menu.local-tasks.6.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
460 bytes
44.43 KB

Fixed CommentApprovalTest.

Berdir’s picture

Issue tags: -Needs tests

Patch looks great, nice simplification and separation of that hook.

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

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:

+++ b/core/modules/system/tests/modules/menu_test/menu_test.moduleundefined
@@ -367,6 +396,56 @@ function menu_test_menu() {
+  if (strpos($router_item['tab_root'], 'menu-test/tasks/') === 0) {

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?

sun’s picture

Hm. @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 a strpos() at all.

sun’s picture

Issue tags: -API clean-up

#8: menu.local-tasks.8.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +API clean-up

The last submitted patch, menu.local-tasks.8.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
44.41 KB

Merged HEAD.

sun’s picture

#13: menu.local-tasks.13.patch queued for re-testing.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

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

sun’s picture

Issue tags: -API clean-up

#13: menu.local-tasks.13.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +API clean-up

The last submitted patch, menu.local-tasks.13.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
44.38 KB

Re-roll

sun’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC based on #15

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/menu.incundefined
@@ -2061,7 +2064,12 @@ function menu_local_tasks($level = 0) {
+    foreach (module_implements('menu_local_tasks') as $module) {
+      $function = $module . '_menu_local_tasks';
+      $function($data, $router_item, $root_path);

This should use the new module handler now.

Patch looks great otherwise.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
44.61 KB
867 bytes

Patch context conflicted with #1862752: Implement entity access API for users.
Fixed the module_implements, and the alter 3 lines later.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, menu-1864066-21.patch, failed testing.

andypost’s picture

sun’s picture

Status: Needs work » Needs review

#21: menu-1864066-21.patch queued for re-testing.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Passed again, back to RTBC.

sun’s picture

Issue tags: -API clean-up

#21: menu-1864066-21.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +API clean-up

The last submitted patch, menu-1864066-21.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
43.92 KB

Merged HEAD + #21.

sun’s picture

Status: Needs review » Reviewed & tested by the community

#29 is a plain re-roll, so back to RTBC.

catch’s picture

+++ b/core/includes/menu.incundefined
@@ -2037,8 +2040,14 @@ function menu_local_tasks($level = 0) {
+    $module_handler = drupal_container()->get('module_handler');
+    foreach ($module_handler->getImplementations('menu_local_tasks') as $module) {
+      $function = $module . '_menu_local_tasks';
+      $function($data, $router_item, $root_path);

Why not invokeAll() here?

Looks good otherwise.

sun’s picture

$data is passed by reference.

catch’s picture

Issue tags: -API clean-up

#29: menu.local-tasks.29.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +API clean-up

The last submitted patch, menu.local-tasks.29.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
43.94 KB

Merged HEAD.

Dries’s picture

I was going to commit this patch but it looks like a re-roll is required. Asking for a re-test.

Dries’s picture

Issue tags: -API clean-up

#35: menu.local-tasks.35.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +API clean-up

The last submitted patch, menu.local-tasks.35.patch, failed testing.

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
43.92 KB

Merged HEAD. (no conflicts)

Status: Reviewed & tested by the community » Needs work
Issue tags: -API clean-up

The last submitted patch, menu.local-tasks.38.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +API clean-up

#39: menu.local-tasks.38.patch queued for re-testing.

sun’s picture

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

Status: Reviewed & tested by the community » Fixed

Looked at this twice now and I think it is good to go. It's a nice simplification and the added tests are welcome too.

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