Improve DX of local task YAML definitions

A longer post describing some of my thoughts leading to proposing this in IRC. http://www.nerdpalace.net/node/6

There are complications with the parent_id/grouping changes I'd suggested in that post that I've almost worked out but the rest is fairly easy to accomplish. I can post a patch with those changes shortly.

Tagging.

Patch.

I don't think the assumption is as hidden or dangerously magical as is implied. route_name still exists and is usable and this validates that the guess is correct and throw an exception to notify developers. It is just a short hand for the common cases.

wrong patch...

typo

#8: drupal8.plugin-system.2107531-8.patch queued for re-testing.

just a reroll.

Missed a spot. The other fail was HEAD being broken.

x

I thought we decided those where separate issues.

+++ b/core/modules/system/tests/modules/menu_test/menu_test.local_tasks.yml
@@ -1,73 +1,59 @@
-  tab_root_id: menu_test.tasks_default_tab
+  root_id: menu_test.tasks_default_tab
...
-  tab_root_id: menu_test.tasks_tasks_tab
+  root_id: menu_test.tasks_tasks_tab

most of the other changes remove the _tab postfix. should these (and a couple others) also do that?

it might be helpful to update the issue summary and explain the difference between the root and parent id. along with any other info to update the summary.

I would love to get the conversions in first.

oh, root and parent are describe in the local tasks change notice. parent is for 2nd level tasks.

+++ b/core/modules/system/tests/modules/menu_test/menu_test.local_tasks.yml
@@ -1,73 +1,59 @@
 menu_test.tasks_default_tab:
   route_name: menu_test.tasks_default
   title: 'View'
-  tab_root_id: menu_test.tasks_default_tab
+  root_id: menu_test.tasks_default_tab

taking the pattern further we can take of _tab from the plugin_id.
then if the plugin id is the same as the route_name, we can remove the route_name.

Also, if it is itself the root, we can take off the root_id.

So... with the patch here, it would become

 menu_test.tasks_default:
   title: 'View'

and it magically knows to look for a route of the same name and that it is its own root.

Before, it was clear the root_id was the plugin_id for the local task and did not have to be named the same as the route.

issue title is:
Improve DX of local task YAML definitions

So, what will our instructions be for developers?
Local tasks are provided by plugins implementing LocalTaskInterface instead of type MENU_LOCAL_TASK in hook_menu() https://drupal.org/node/2044515
says now:

user.login_tab:
  route_name: user_page
  title: 'Log in'
  tab_root_id: user_login_tab
user.new_password_tab:
  route_name: user_pass
  title: 'Request new password'
  tab_root_id: user_login_tab
user.new_password_tab:
  route_name: user_register
  title: 'Create new account'
  tab_root_id : user_login_tab
  weight: 10

adding an example:
core/modules/system/tests/modules/menu_test/menu_test.local_tasks.yml

 menu_test.tasks_default_tab:
   route_name: menu_test.tasks_default
   title: View
   tab_root_id: menu_test.tasks_default_tab
 menu_test.tasks_default_edit_tab:
   route_name: menu_test.tasks_default_edit
   title: Edit
   tab_root_id: menu_test.tasks_default_tab

Each local task is represented by a plugin. Each plugin definition (found in the $module.local_actions.yml file) contains three to five loal-task specific keys underneath the top-level key which is the unique plugin ID. Pattern for the plugin ID is: module_name.whatever_you_want_that_does_not_have_a_dot

Keys are:
route_name: The machine name of the local task route - this also determines where it's displayed
title: The title of the local action. By default, it will be passed through t() and localized.
tab_root_id: The plugin ID of the "root" tab (generally the top, leftmost one) which serves to group a set of tabs.
tab_parent_id: (optional) The plugin ID of the tab that is the parent - only relevant for 2nd level tabs
weight:(optional) The integer weight (lower weight tabs are further left, default is 0).

after would it be:

user.login:
  route_name: user.page
  title: 'Log in'
  root_id: user_login
user.new_password:
  route_name: user.pass
  title: 'Request new password'
  root_id: user.login
user.new_password:
  route_name: user.register
  title: 'Create new account'
  root_id: user.login
  weight: 10

added example would be

 menu_test.tasks_default:
   title: View
 menu_test.tasks_default_edit:
   title: Edit
   root_id: menu_test.tasks_default

Each local task is represented by a plugin. Each plugin definition (found in the $module.local_actions.yml file) contains one to five local-task specific keys underneath the top-level key which is the unique plugin ID. Pattern for the plugin ID is: module_name.whatever_you_want_that_does_not_have_a_dot

It is recommended is to name the plugin the same as the route name:
route_name

Keys are:
route_name: The machine name of the local task route - this also determines where it's displayed. If the plugin ID is equal to the route name, the route_name is optional and if not specified, the route_name will be assumed to be the same as the plugin ID.
title: The title of the local action. By default, it will be passed through t() and localized. Strings with spaces should use single quotes.
root_id: The plugin ID of the "root" tab (generally the top, leftmost one) which serves to group a set of tabs. This is optional if it is the root.
parent_id (optional): The plugin ID of the tab that is the parent - only relevant for 2nd level tabs. Only needed if different than the root_id.
weight: (optional) The integer weight (lower weight tabs are further left, default is 0).

I'll put this is the issue summary so others can edit and correct it.

Note to self: I think class is a key missing from the list of three to five keys.

So, I'm not sure if this is 'Improving the DX' or 'shortening what they have to type'.

before and after example added

html

html again

I'm not sure this is a good idea, but since I went through menu_test and applied the pattern to bits that were left out, here it is in case it is useful.

Before, it was clear the root_id was the plugin_id for the local task and did not have to be named the same as the route.

So, I'm not sure if this is 'Improving the DX' or 'shortening what they have to type'.

So, I think these are great statements describing the confusing I've seen around this patch.

So the developer experience literally is "what I have to type" so I don't see those as completely seperate. Also something less clear from touching up or reviewing is "what I have to remember and copy around" though in the future this will be the case for re-factoring.

I found that when making a set of local tasks the biggest process after finding the routes is this dance of copying things around or repeatedly typing out certain strings. Its really frustrating and it ends up with in the worst case 3 copies of the exact same string on a definition(this is true with the _tab only you've got to remember which one gets the suffix and which don't).

Anywhere in code where we have a pattern like this where we consistently are calling a function and copying the same thing into multiple parameters we would just provide a default behavior. We document this is how we expect to generally call this so this is how each argument will behave if the argument isn't provided. Everything is clear.

This however is actually more frustrating then code though because its manually typed strings rather then what would likely be repeated variables in code so its very error prone and has more maintenance debt in that you're going to have copy the string out all over the structure. The code example used by YesCT pretty clearly demonstrate this I think.

And I think YesCT did a pretty good idea of explaining what we tell developers as well.

Didn't make it :(

So, I think the change here should be limited to at most:
- changing "tab_parent_id" to "parent_id"
- copying by default the tab_root_id from the parent for tabs not at the top level.

We could simplify it even a bit more. What about just having parent_id or parent_route_id (depends which one we want).
If parent_id == tab_parent_id, we are at the default local task case. If parent_id is a default local task, we are at level 0 but a sibling etc.

remaining tasks

I swear there is a meta issue somewhere for "all the stuff that used to be defined with hook_menu()"... where?

Also, I'd consider this a major DX issue.

Looks like we are still not close to figuring out how to improve DX of the tabs definitions or if any proposed improvement is actually an improvement or not. In practice, we now have the following structures in core:

some_route_id:
  tab_root_id: some_route_id
  route_name: some_route_id
  title: 'Some tab'

The plugin ID of the tab is not required to be the same as the route, but we adopted that as a common practice because otherwise it is free reign, people can use '_tab' after the route name, or replace dots with underscores or shorten things or whatnot and then all you end up with is a mess. I think its an improvement that we have clear guidelines for this. I'm not sure if that is even a possible use case that a route may serve different tabs at different places, so tying the two together with a best practice pattern sounds good to me.

As for then leaving them out if they are not different from each other and ending up with:

some_route_id:
  title: 'Some tab'

I don't have a strong feeling either way, but it is true that people do understand classes where not all properties of the object are on the constructor (and have defaults, sometimes computed). People also understand functions that have optional arguments, etc. I think its a good question how default values here would be different from those.

I think if we document / suggest the naming parity rule for the tab plugin ID and the route name, then repeated equal values will keep being common. If we don't document / suggest that, then the plugin IDs will be an inconsistent mess, and people need to wonder how to name them and find some other rule / suggestion anyway. That sounds like more work. So if we assume our best practice setup will have the same ID 2-3 times on it, the question remains if that is best for developers to understand and repeat their understanding of the inner workings of the system each time they write a tab definition.

@pwolanin: would be good to have a list of simplifications that all parties support :) You just mentioned one here. Hope there is more, because taking the root from the parent sounds like not a major or beta blocker change(?)

The base_route looks better then the tab_root_id to me. Is this the extent of changes proposed or just the first step?

Probably not. base_route isn't set in many cases now so what are we trying to store in the base routes instead? Should it be:

$base_routes[$task_info['route_name']] = $task_info['parent_id'];

I don't really understand the issue enough to say any more about it.

+++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
@@ -179,47 +179,54 @@ public function getLocalTasksForRoute($route_name) {
+            if(!empty($task_info['base_route'])) {
+              $base_routes[$task_info['base_route']] = $task_info['base_route'];
+            }
...
           if ($route_name == $task_info['route_name']) {
-            $tab_root_ids[$task_info['tab_root_id']] = $task_info['tab_root_id'];

I wonder whether we can introduce to auomatically add the base_route, if possible.

Is parent_id supposed to be the base_route if base_route is not set?

@pwolanin: that is what I thought. If I am following this, that additional code should go right near the patch in #46, correct?

I have noticed that in cases where this conditional is used:

foreach ($definitions as $plugin_id => $task_info) {}

$plugin_id sometimes has parameters attached after a colon. So, you shouldn't try to test if the $route_name passed to the method is equal to $plugin_id. It is better to do this:

$route_name == $task_info['id']

This will certainly still fail, but I'd like to see how.

This patch is wonderful. It really improves the DX of local tasks while keeping the logic the same, now it just have to pass.

Sadly

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Plugin/Derivative/FieldUiLocalTask.php
@@ -79,7 +79,7 @@ public function getDerivativeDefinitions(array $base_plugin_definition) {
-          'tab_root_id' => "field_ui.fields:overview_$entity_type",
+          'base_route' => "field_ui.overview_$entity_type",

This is wonderful <3 < 3

+++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskDefaultTest.php
@@ -193,33 +193,26 @@ public function providerTestGetWeight() {
-      // Ensure that a default tab of a derivative gets the default value.
-      array(
-        array(
-          'tab_root_id' => 'local_task_derivative_default:example_id',
-          'id' => 'local_task_derivative_default'
-        ),
-        'local_task_derivative_default:example_id',
-        -10,
-      ),

What is the reason for this hunk?

I went down and tried to remove that removal as well as add a new unit test for childs, so we know that the parent_id works,
though I have not managed to get it green.

Extended the test coverage to 100% in the getLocalTasksForRoute method.

Critical beta blocker.

Wow, this patch is absolutely just full of win. Great job. Also nice job on the issue summary, it was super easy to tell what was going on.

Committed and pushed to 8.x. Thanks! We need to update the existing change notices and documentation under https://drupal.org/node/2122071 now.

nice!

In the change record, we wrote:
"Note in particular that by defining a base_route, it's simpler to discover how to associate a tab, rather than having to find a plugin ID."

I'm not sure why we said that. Don't we look up the route id in the same place that we would have looked up the plugin id?