Replace local action plugin discovery with YamlDiscovery and handle routes with parameters

Really nice stuff.

• As local actions are basically menu entries, they also have route_name in there if they don't are local action plugins yet. Let's add a new check for them being plugins
• Some code still refered to the static discovery derivative
• Used a plugin for the menu link add action
• It is a bit sad that you directly jumped over #2046565: Cache the local action plugins that appear per route as we could have just extended the test coverage coming from there.

Some of the review points fixed in the patch.

1. +++ b/core/includes/menu.inc
   @@ -2307,21 +2313,9 @@ function menu_secondary_local_tasks() {
   -  $local_actions = $manager->getActionsForRoute($router_item['route_name']);
   -  foreach ($local_actions as $plugin) {
   -    $route_path = $manager->getPath($plugin);
   -    $action_router_item = menu_get_item($route_path);
   -    $links['actions'][$route_path] = array(
   -      '#theme' => 'menu_local_action',
   -      '#link' => array(
   -        'title' => $manager->getTitle($plugin),
   -        'href' => $route_path,
   -      ),
   -      '#access' => $action_router_item['access'],
   -    );
   -  }
   +  $links['actions'] += $manager->getActionsForRoute($route_name);
   

   +1 for moving the logic into the local action manager.

2. +++ b/core/lib/Drupal/Core/Menu/LocalActionManager.php
   @@ -25,6 +31,23 @@
   +    // The static title for the local task.
   ...
   +    // Default class for local task implementations.
   

   Ha, I nearly thought you really did not copied anything by hand :)

3. +++ b/core/modules/views_ui/views_ui.module
   @@ -25,11 +25,6 @@ function views_ui_menu() {
      );
    
   -  $items['admin/structure/views/add'] = array(
   -    'route_name' => 'views_ui.add',
   -    'type' => MENU_SIBLING_LOCAL_TASK,
   -  );
   

   Nice!

Comments regarding the fixes:

• Custom block types:
  The url /add got removed, though menu_get_item() always fetches also some parent. This leads the local action to appear on 403/404 pages, so the test still worked
• Many annotation based plugins haven't been converted
• The derivative class was still here.

So many new files are missing. (updating a lost of the different patches). (never touch your gitignore file)

There we go.

Ha, made a copy error.

Just a rerole.

+1 Just taking the chance to add a tag.

Just another rerole on top of #18 and one more little change.

This should be it.

Custom_block already fixed what was needed for this patch to work.

the patch should include a conversion
tim points to menu_link_add goes on menu_menu_edit

I am glad that this is already part of the patch.

... let's see how much this fixes.

Interesting, there are local actions which neither have a real href, nor a route, but just a title/text.

Yes we do:

@@ -1737,12 +1737,21 @@ function theme_menu_local_action($variables) {
   $link += array(
     'href' => '',
     'localized_options' => array(),
+    'route_parameters' => array(),
   );

Let's just go with this.

Is there a clear reason behind why that was done?

All the rerole due to the route_name changed did not allow me to provide an interdiff.

This fixes especially the point made in #34

doh!

This should fix those test failures, I think it's just a route naming issue.

Plus:

1. +++ b/core/lib/Drupal/Core/Menu/LocalActionDefault.php
   @@ -0,0 +1,137 @@
   + * Provides defaults and base methods for menu local action plugins.
   

   This could prob be updated now, it's acting more of a default implementation really. EDIT: Hmm, maybe not!

2. +++ b/core/lib/Drupal/Core/Menu/LocalActionDefault.php
   @@ -0,0 +1,137 @@
   + * @todo This class needs more documentation and/or @see references.
   

   Really?

3. +++ b/core/lib/Drupal/Core/Menu/LocalActionDefault.php
   @@ -0,0 +1,137 @@
   +   * String translation object.
   

   'The string translation object'

4. +++ b/core/lib/Drupal/Core/Menu/LocalActionInterface.php
   @@ -21,6 +23,37 @@
   +   * Returns the route parameters needed to render a link for the local task.
   ...
   +   * Returns options for rendering a link to the local task.
   

   Prob make both of these "...FOR the local task"

5. +++ b/core/lib/Drupal/Core/Menu/LocalActionManager.php
   @@ -26,6 +32,28 @@
   +   * Provides some default values for all local action plugin.s
   

   plugin.s

6. +++ b/core/lib/Drupal/Core/Menu/LocalActionManager.php
   @@ -26,6 +32,28 @@
   +    'id' => '',
   

   Seems like this default should be NULL?

7. +++ b/core/lib/Drupal/Core/Menu/LocalActionManager.php
   @@ -92,44 +140,46 @@ public function getTitle(LocalActionInterface $local_action) {
   +      $route_names ? $this->routeProvider->getRoutesByNames($route_names) : array();
   

   I don't see why we need a ternary here? The logic is: if there are route names, get all the routes by names? So basically, this would be better (and maybe more readable) as if ($route_names) { $this->routeProvider->getRoutesByName() }?

+++ b/core/lib/Drupal/Core/Menu/LocalActionInterface.php
@@ -21,6 +23,37 @@
+   * Returns the route parameters needed to render a link for the local task.
...
+   * Returns options for rendering a link to the local task.

Prob make both of these "...FOR the local task"

"... for the local action."

Ha, that's what I said ;)

Seems like this default should be NULL?

I don't care, as we will throw some exception anyway in the future.

Good points, ... here are some other typos as well.

I think we still need to wrap the call to getRoutesByNames() in an if, so we don't pass nothing in there. I made a couple of other changes, as well as removeing the getPath() method from the interface (we only need route names now) and implementing getWeight() in the default class and in the manager where the local action links are built.

Someone should fill a novice task to replace '' to NULL in the LocalTaskManager.

I don't get that, 0 is a sensible default for a weight. It should always be an int IMO. id and route_name are required keys, weight isn't.

Probably weight should be NULL by default here also like for local tasks?

So damian, the reason why it is NULL on local tasks is that we can apply some logic. If it is NULL but the default local task, change it to -10, otherwise set it to 0.
If it is already 0 you cannot know whether this was intended.

For local actions it doesn't really make sense to not have a 0 as default given that there are no default local actions.

OK, I don't really mind either way, let's just change it to NULL and be on our way.

Whatever works, back to RTBC

oops, didn't pull/rebase!

This should be it.

Given that this is needed to remove the menu_router we should mark it as critical.

1. +++ b/core/modules/config/tests/config_test/config_test.module
   index 1f31d46..ec5760f 100644
   --- a/core/modules/config/tests/config_test/lib/Drupal/config_test/Plugin/Menu/LocalAction/AddConfigTestEntityLocalAction.php
   
   --- a/core/modules/config/tests/config_test/lib/Drupal/config_test/Plugin/Menu/LocalAction/AddConfigTestEntityLocalAction.php
   +++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/Plugin/Menu/LocalAction/AddConfigTestEntityLocalAction.php
   

   Shouldn't this be deleted?

2. +++ b/core/modules/filter/filter.module
   index b75c102..da45c05 100644
   --- a/core/modules/filter/lib/Drupal/filter/Plugin/Menu/LocalAction/AddFilterFormatLocalAction.php
   
   --- a/core/modules/filter/lib/Drupal/filter/Plugin/Menu/LocalAction/AddFilterFormatLocalAction.php
   +++ b/core/modules/filter/lib/Drupal/filter/Plugin/Menu/LocalAction/AddFilterFormatLocalAction.php
   

   Shouldn't this be deleted?

3. +++ b/core/modules/menu/menu.module
   index 8a01f7b..499ff3b 100644
   --- a/core/modules/shortcut/lib/Drupal/shortcut/Plugin/Menu/LocalAction/AddShortcutSetLocalAction.php
   
   --- a/core/modules/shortcut/lib/Drupal/shortcut/Plugin/Menu/LocalAction/AddShortcutSetLocalAction.php
   +++ b/core/modules/shortcut/lib/Drupal/shortcut/Plugin/Menu/LocalAction/AddShortcutSetLocalAction.php
   

   Shouldn't this be deleted?

4. +++ b/core/lib/Drupal/Core/Menu/LocalActionManager.php
   @@ -26,6 +32,30 @@
   +    'class' => 'Drupal\Core\Menu\LocalActionDefault',
   

   We need to test overriding this given that we don't appear to have any usages in core.

Good points!

Committed 0fc52f1 and pushed to 8.x. Thanks!

Fixed a couple of minor nits during commit.

diff --git a/core/lib/Drupal/Core/Menu/LocalActionDefault.php b/core/lib/Drupal/Core/Menu/LocalActionDefault.php
index bf3bda7..be6e6ab 100644
--- a/core/lib/Drupal/Core/Menu/LocalActionDefault.php
+++ b/core/lib/Drupal/Core/Menu/LocalActionDefault.php
@@ -85,7 +85,7 @@ public function getRouteParameters(Request $request) {
     $route = $this->routeProvider->getRouteByName($this->getRouteName());
     $variables = $route->compile()->getVariables();

-    // Noramlly the \Drupal\Core\ParamConverter\ParamConverterManager has
+    // Normally the \Drupal\Core\ParamConverter\ParamConverterManager has
     // processed the Request attributes, and in that case the _raw_variables
     // attribute holds the original path strings keyed to the corresponding
     // slugs in the path patterns. For example, if the route's path pattern is
diff --git a/core/modules/system/tests/modules/menu_test/lib/Drupal/menu_test/Plugin/Menu/LocalAction/TestLocalAction.php b/core/modules/system/tests/modules/menu_test/lib/Drup
index 4a99f52..3811954 100644
--- a/core/modules/system/tests/modules/menu_test/lib/Drupal/menu_test/Plugin/Menu/LocalAction/TestLocalAction.php
+++ b/core/modules/system/tests/modules/menu_test/lib/Drupal/menu_test/Plugin/Menu/LocalAction/TestLocalAction.php
@@ -21,4 +21,4 @@ public function getTitle() {
     return 'Title override';
   }

-}
+}

These changes are looking great. I do like that the central change notice is updated and the other one just link to it.

added issue summary.