Updated: Comment ...

Problem/Motivation

Building on
#2065571: Add YAML Plugin discovery
and
#2050919: Replace local task plugin discovery with YamlDiscovery

Local actions are currently using YAMl derivatives on top of annotation-based discovery.

however, the 90%+ use case would be satisfied by the YAMl, and I think it is more DX friendly to just let devs specify the plugin class or derivative class in the YAML in the < 10% case where they need to do something special.

Remaining tasks

User interface changes

API changes

#2091145: Adapt $module.foo pattern for local action plugin IDs

Original report by @pwolanin

CommentFileSizeAuthor
#69 interdiff.txt4.64 KBdawehner
#69 local_actions-2077473-69.patch39.76 KBdawehner
#64 local_actions-2077473-64.patch36.66 KBdawehner
#64 interdiff.txt595 bytesdawehner
#60 2077473-60.patch36.66 KBpwolanin
#60 2077473-58-60.increment.txt528 bytespwolanin
#58 2077473-58.patch36.71 KBpwolanin
#58 2077473-56-58.increment.txt2.56 KBpwolanin
#56 2077473-56.patch37.57 KBdamiankloip
#53 2077473-53.patch37.56 KBdamiankloip
#53 interdiff-2077473-53.txt618 bytesdamiankloip
#47 2077473-47.patch37.55 KBdamiankloip
#47 interdiff-2077473-47.txt3.6 KBdamiankloip
#45 local_action-2077473-45.patch37.36 KBdawehner
#45 interdiff.txt3.87 KBdawehner
#41 2077473-41.patch37.39 KBdamiankloip
#41 interdiff-2077473-41.txt1020 bytesdamiankloip
#39 local_action-2077473-39.patch37.39 KBdawehner
#39 interdiff.txt592 bytesdawehner
#37 local_actions-2077473-37.patch37.39 KBdawehner
#33 local_actions-2077473-33.patch35.02 KBdawehner
#33 interdiff.txt927 bytesdawehner
#30 local_action-2077473-30.patch34.99 KBdawehner
#30 interdiff.txt570 bytesdawehner
#28 local_action-2077473-28.patch34.92 KBdawehner
#28 interdiff.txt493 bytesdawehner
#25 local_action-2077473-25.patch34.92 KBdawehner
#23 local_action-2077473-23.patch36.63 KBdawehner
#23 interdiff.txt493 bytesdawehner
#21 local_action-2077473-21.patch36.63 KBdawehner
#21 interdiff.txt2.4 KBdawehner
#18 local_action-2077473-18.patch36.65 KBpwolanin
#18 increment.txt1.9 KBpwolanin
#17 local_action-2077473-17.patch36.6 KBdawehner
#15 local_action-2077473-15.patch36.62 KBdawehner
#15 interdiff.txt432 bytesdawehner
#13 local_actions-2077473-13.patch36.62 KBdawehner
#13 interdiff.txt397 bytesdawehner
#7 local_actions-2077473-7.patch36.62 KBdawehner
#7 interdiff.txt1.19 KBdawehner
#6 local_actions-2077473-5.patch34.8 KBdawehner
#6 interdiff.txt8.9 KBdawehner
#3 interdiff.txt5.08 KBdawehner
#3 2077473-3.patch25.91 KBdawehner
#1 2077473-1.patch23.17 KBpwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Title: Replace local action plugin discovery with YamlDiscovery » Replace local action plugin discovery with YamlDiscovery and handle routes with parameters
Status: Active » Needs review
FileSize
23.17 KB

Status: Needs review » Needs work

The last submitted patch, 2077473-1.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
25.91 KB
5.08 KB

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!

Status: Needs review » Needs work

The last submitted patch, 2077473-3.patch, failed testing.

pwolanin’s picture

@dawehner - I just lost track of #2046565: Cache the local action plugins that appear per route, though I noticed the @todo in the code. This patch reorganized that method a bit, so not sure if we should combine them some how.

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.9 KB
34.8 KB

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.
dawehner’s picture

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

Status: Needs review » Needs work

The last submitted patch, local_actions-2077473-7.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review

#7: local_actions-2077473-7.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, local_actions-2077473-7.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review

#7: local_actions-2077473-7.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, local_actions-2077473-7.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
397 bytes
36.62 KB

There we go.

Status: Needs review » Needs work

The last submitted patch, local_actions-2077473-13.patch, failed testing.

dawehner’s picture

FileSize
432 bytes
36.62 KB

Ha, made a copy error.

disasm’s picture

Status: Needs work » Needs review
dawehner’s picture

Just a rerole.

pwolanin’s picture

Some cleanup of code comments and clarifies the check in the theme function.

Status: Needs review » Needs work

The last submitted patch, local_action-2077473-18.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +MenuSystemRevamp

+1 Just taking the chance to add a tag.

dawehner’s picture

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

Status: Needs review » Needs work

The last submitted patch, local_action-2077473-21.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
493 bytes
36.63 KB

This should be it.

Status: Needs review » Needs work

The last submitted patch, local_action-2077473-23.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
34.92 KB

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

Status: Needs review » Needs work

The last submitted patch, local_action-2077473-25.patch, failed testing.

pwolanin’s picture

the patch should include a conversion

tim points to menu_link_add goes on menu_menu_edit

dawehner’s picture

Status: Needs work » Needs review
FileSize
493 bytes
34.92 KB

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.

Status: Needs review » Needs work

The last submitted patch, local_action-2077473-28.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
570 bytes
34.99 KB

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

Status: Needs review » Needs work

The last submitted patch, local_action-2077473-30.patch, failed testing.

pwolanin’s picture

Is that logic right? Are we setting an empty string by default?

dawehner’s picture

Status: Needs work » Needs review
FileSize
927 bytes
35.02 KB

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.

pwolanin’s picture

In link with the last LocalTask patch, let's rename LocalActionBase to LocalActionDefault?

dawehner’s picture

Is there a clear reason behind why that was done?

pwolanin’s picture

@dawehner - it's being used as the default. It's not being extended by any other class typically. In contrast, FormBase or ControlerBase classes are never used themselves - they are abstract.

dawehner’s picture

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

Status: Needs review » Needs work

The last submitted patch, local_actions-2077473-37.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
592 bytes
37.39 KB

doh!

Status: Needs review » Needs work

The last submitted patch, local_action-2077473-39.patch, failed testing.

damiankloip’s picture

Status: Needs review » Needs work
FileSize
1020 bytes
37.39 KB

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() }?

damiankloip’s picture

Status: Needs work » Needs review
neclimdul’s picture

Status: Needs work » Needs review
+++ 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."

damiankloip’s picture

Ha, that's what I said ;)

dawehner’s picture

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.

Status: Needs review » Needs work

The last submitted patch, local_action-2077473-45.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
3.6 KB
37.55 KB

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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

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

pwolanin’s picture

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

damiankloip’s picture

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.

dawehner’s picture

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.

pwolanin’s picture

I think NULL could still be useful for the same reason (to distinguish those plugins where it's set in the base definition vs. not set at all), even though we don't have the concept of default. In drupal_render() the weight not being set makes the item sort as 0, but it's never filled in e.g. for form arrays.

damiankloip’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
618 bytes
37.56 KB

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Whatever works, back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2077473-53.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
37.57 KB

oops, didn't pull/rebase!

neclimdul’s picture

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

as a result of #2087231: Add a PluginBase in the Core namespace with t() as a helper method, we don't need to add t() to the default class.

This is a very minor change, o still should be RTBC unless the tests fail.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2077473-58.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
528 bytes
36.66 KB

oops, I was sure I deleted that line in create()... maybe I didn't save before rolling the patch.

Status: Needs review » Needs work
Issue tags: -MenuSystemRevamp

The last submitted patch, 2077473-60.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review

#60: 2077473-60.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +MenuSystemRevamp

The last submitted patch, 2077473-60.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
595 bytes
36.66 KB

This should be it.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Ah, yes, good catch. Core not Component. back to RTBC.

pwolanin’s picture

#64: local_actions-2077473-64.patch queued for re-testing.

dawehner’s picture

Priority: Normal » Critical

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
39.76 KB
4.64 KB

Good points!

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Looks good with the added test.

pwolanin’s picture

Status: Reviewed & tested by the community » Needs review

[posted to wrong issue]

pwolanin’s picture

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

Title: Replace local action plugin discovery with YamlDiscovery and handle routes with parameters » Change notice: Replace local action plugin discovery with YamlDiscovery and handle routes with parameters
Priority: Critical » Major
Status: Reviewed & tested by the community » Active
Issue tags: +API change, +Needs change record, +Approved API change

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';
   }

-}
+}
pwolanin’s picture

Status: Active » Needs review

first pass at a change notice: https://drupal.org/node/2102583

Also updated: https://drupal.org/node/2007444

pwolanin’s picture

Title: Change notice: Replace local action plugin discovery with YamlDiscovery and handle routes with parameters » Replace local action plugin discovery with YamlDiscovery and handle routes with parameters
Status: Needs review » Fixed
Issue tags: -Needs change record

markign fixed given the lack of any feedback on the change notices.

dawehner’s picture

Issue tags: +Needs change record

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

pwolanin’s picture

Issue tags: -Needs change record

bad tag

xjm’s picture

Issue tags: +Prague Hard Problems
xjm’s picture

Issue summary: View changes

added issue summary.

Status: Fixed » Closed (fixed)

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