Branching off from #1981644: Figure out how to deal with 'title/title callback'

Local actions in Drupal 8 that are based on routes cannot handle dynamic titles or paths (e.g. an action link with the current node title or path)

The proposed solution is to convert them to plugins which have getTitle and getPath methods.

Files: 
CommentFileSizeAuthor
#28 menu-local-action-2031473-28.patch22.15 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,877 pass(es).
[ View ]
#28 interdiff.txt22.5 KBtim.plunkett
#23 local-actions-2031473-23.patch21.68 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 56,778 pass(es).
[ View ]
#23 interdiff.txt998 bytesdawehner
#22 local-actions-2031473-22.patch21.46 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,986 pass(es).
[ View ]
#22 interdiff.txt1.45 KBtim.plunkett
#20 drupal-2031473-19.patch19.97 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,121 pass(es).
[ View ]
#20 interdiff.txt3.99 KBdawehner
#15 interdiff.txt2.42 KBfubhy
#15 2031473-15.patch20.12 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 57,059 pass(es).
[ View ]
#11 interdiff.txt5.64 KBfubhy
#11 2031473-11.patch19.7 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 56,765 pass(es).
[ View ]
#8 2031473-8.patch20.3 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 56,565 pass(es).
[ View ]
#8 2031473-7-8.increment.txt1.11 KBpwolanin
#7 2031473-7.patch19.81 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 56,356 pass(es).
[ View ]
#7 2031473-4-7.increment.txt7.55 KBpwolanin
#4 2031473-4.patch20.25 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 56,356 pass(es).
[ View ]
#4 2031473-2-4.increment.txt5.04 KBpwolanin
#2 2031473-2.patch19.53 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 56,370 pass(es).
[ View ]
#2 2031473-52-2.incremental.txt7.01 KBpwolanin
#1 menu-plugins-1981644-52.patch18.36 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 56,566 pass(es).
[ View ]

Comments

pwolanin’s picture

Status:Active» Needs review
StatusFileSize
new18.36 KB
PASSED: [[SimpleTest]]: [MySQL] 56,566 pass(es).
[ View ]

Here's the patch from #52 at #1981644: Figure out how to deal with 'title/title callback', which should be the last local action patch that was posted there.

pwolanin’s picture

StatusFileSize
new7.01 KB
new19.53 KB
PASSED: [[SimpleTest]]: [MySQL] 56,370 pass(es).
[ View ]

With doxygen cleanups and minor changes suggested in the review & IRC by dawehner

dawehner’s picture

+++ b/core/modules/system/lib/Drupal/system/Annotation/MenuLocalActionPlugin.phpundefined
@@ -0,0 +1,48 @@
+   * Array of route names this appears on.

An array of route names...

+++ b/core/modules/system/lib/Drupal/system/Annotation/MenuLocalActionPlugin.phpundefined
@@ -0,0 +1,48 @@
+  public $appears_on = array();
+}

empty line

+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionBase.phpundefined
@@ -0,0 +1,91 @@
+

let's remove that, we seem to not do that anymore.

+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionBase.phpundefined
@@ -0,0 +1,91 @@
+abstract class MenuLocalActionBase extends ContainerFactoryPluginBase implements MenuLocalActionInterface {
+  /**

empty lines between the two.

+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionBase.phpundefined
@@ -0,0 +1,91 @@
+  public function __construct(TranslatorInterface $string_translation,  UrlGeneratorInterface $generator, array $configuration, $plugin_id, array $plugin_definition) {

Needs some docs

+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionBase.phpundefined
@@ -0,0 +1,91 @@
+    // Subclasses may set a request into the generator or
+    // use any desired method to generate the path.

Let's try to fill up until 80 chars.

+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionBase.phpundefined
@@ -0,0 +1,91 @@
+  }
+}

empty line between of them.

+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionInterface.phpundefined
@@ -0,0 +1,39 @@
+   * Implmenetations of this interface must also implement two controller

Typo in "Implmenetations"

+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionInterface.phpundefined
@@ -0,0 +1,39 @@
+   * public function getTitle();
...
+   * public function getPath();

What about making a new comment block, and document out. So it looks like "*/ \n // public function getTitle"

+++ b/core/modules/system/lib/Drupal/system/Plugin/Type/MenuLocalActionManager.phpundefined
@@ -0,0 +1,128 @@
+ * Menu local actions are links that lead to actions like "add new".
+ * The plugin format allows them (if needed) to dynamically generate
+ * a title or the path they link to. The annotation on the plugin provides
+ * the default title and other key information.

Let's try to fill up the 80 chars.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Type/MenuLocalActionManager.phpundefined
@@ -0,0 +1,128 @@
+   * @param Drupal\system\Plugin\MenuLocalActionInterface $local_action
...
+   * @param Drupal\system\Plugin\MenuLocalActionInterface $local_action

Missing starting "\"

+++ b/core/modules/system/lib/Drupal/system/Plugin/Type/MenuLocalActionManager.phpundefined
@@ -0,0 +1,128 @@
+   * @throws BadMethodCallException

+1 for document that kind of stuff

+++ b/core/modules/system/lib/Drupal/system/Plugin/Type/MenuLocalActionManager.phpundefined
@@ -0,0 +1,128 @@
+    throw new \BadMethodCallException('Menu local action instances must implement getTitle()');

+1 for throw an exception. it is just a better DX.

pwolanin’s picture

StatusFileSize
new5.04 KB
new20.25 KB
PASSED: [[SimpleTest]]: [MySQL] 56,356 pass(es).
[ View ]

Thanks @dawehner. Here's a revised one with doc fixes.

tim.plunkett’s picture

Discussion ongoing about the interface and the thrown exception, but here's a nitpick.

+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionInterface.phpundefined
@@ -0,0 +1,40 @@
+   * Get the route name from the settings.

Gets

+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionInterface.phpundefined
@@ -0,0 +1,40 @@
+   * Implmentations of this interface must also implement two controller

Typo, but I think we agreed to remove this comment?

+++ b/core/modules/system/lib/Drupal/system/Plugin/Type/MenuLocalActionManager.phpundefined
@@ -0,0 +1,128 @@
+   * Get the title for a local action.
...
+   * Get the Drupal path for a local action.
...
+   * Find all local actions that appear on a named route.

Gets, Finds, etc

+++ b/core/modules/system/lib/Drupal/system/Plugin/Type/MenuLocalActionManager.phpundefined
@@ -0,0 +1,128 @@
+   * @param \Drupal\system\Plugin\MenuLocalActionInterface $local_action
...
+   * @param \Drupal\system\Plugin\MenuLocalActionInterface $local_action
...
+   * @param string $route_name

Needs a sentence

+++ b/core/modules/system/tests/modules/menu_test/lib/Drupal/menu_test/Plugin/Menu/MenuTestLocalAction.phpundefined
@@ -0,0 +1,23 @@
+<?php
+/**

+++ b/core/modules/views_ui/lib/Drupal/views_ui/Plugin/Menu/AddViewLocalAction.phpundefined
@@ -0,0 +1,24 @@
+<?php
+/**

Missing a blank line, here and a couple places

+++ b/core/modules/system/tests/modules/menu_test/lib/Drupal/menu_test/Plugin/Menu/MenuTestLocalAction.phpundefined
@@ -0,0 +1,23 @@
+ * @MenuLocalActionPlugin(

Can we just call this @MenuLocalAction?

+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionInterface.phpundefined
@@ -21,19 +21,20 @@
+   * Implmentations of this interface must also implement two controller

Typo

pwolanin’s picture

StatusFileSize
new7.55 KB
new19.81 KB
PASSED: [[SimpleTest]]: [MySQL] 56,356 pass(es).
[ View ]

Fixes per Tim's suggestions

pwolanin’s picture

StatusFileSize
new1.11 KB
new20.3 KB
PASSED: [[SimpleTest]]: [MySQL] 56,565 pass(es).
[ View ]

Adds methods in the interface - apparently adding optional params works with both the interface and the ControllerResolver.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

This is the best possible outcome I think we'll see. This will suit all contrib needs.

#famouslastwords

YesCT’s picture

Issue tags:+RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

fubhy’s picture

StatusFileSize
new19.7 KB
PASSED: [[SimpleTest]]: [MySQL] 56,765 pass(es).
[ View ]
new5.64 KB

Just some small fixes and some remarks (@see below).

+++ b/core/modules/system/lib/Drupal/system/Annotation/MenuLocalAction.phpundefined
@@ -0,0 +1,49 @@
+class MenuLocalAction extends Plugin {

I'd even go as far as just naming that "LocalAction". Seems verbose enough for me, especially since the annotation discovery is in /Menu for this. I did not touch that though.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Type/MenuLocalActionManager.phpundefined
@@ -0,0 +1,131 @@
+  /**
+   * Gets the title for a local action.
+   *
+   * @param \Drupal\system\Plugin\MenuLocalActionInterface $local_action
+   *   An object to get the title from.
+   *
+   * @return string
+   *   The title (already localized).
+   *
+   * @throws BadMethodCallException
+   */
+  public function getTitle(MenuLocalActionInterface $local_action) {
+    $controller = array($local_action, 'getTitle');
+    if (is_callable($controller)) {
+      $arguments = $this->controllerResolver->getArguments($this->request, $controller);
+      return call_user_func_array($controller, $arguments);
+    }
+    throw new \BadMethodCallException('Menu local action instances must implement getTitle()');
+  }
+
+  /**
+   * Gets the Drupal path for a local action.
+   *
+   * @param \Drupal\system\Plugin\MenuLocalActionInterface $local_action
+   *   An object to get the path from.
+   *
+   * @return string
+   *   The path.
+   *
+   * @throws BadMethodCallException
+   */
+  public function getPath(MenuLocalActionInterface $local_action) {
+    $controller = array($local_action, 'getPath');
+    if (is_callable($controller)) {
+      $arguments = $this->controllerResolver->getArguments($this->request, $controller);
+      return call_user_func_array($controller, $arguments);
+    }
+    throw new \BadMethodCallException('Menu local action instances must implement getPath()');

Either we have an interface, or check for is_callable(). Doing both does not make sense. I would vote for not having an interface as I find it rather weird to pretend that the arguments of these methods are optional. They are not (depending the local action plugin). And yes, they are different. So we got the controller pattern right there -> no interface (imo).

+++ b/core/modules/system/lib/Drupal/system/Plugin/Type/MenuLocalActionManager.phpundefined
@@ -0,0 +1,131 @@
+    if (!isset($this->instances[$route_name])) {
+      $this->instances[$route_name] = array();
+      // @todo - optimize this lookup by compiling or caching.
+      foreach ($this->getDefinitions() as $plugin_id => $action_info) {
+        if (in_array($route_name, $action_info['appears_on'])) {
+          $plugin = $this->createInstance($plugin_id);
+          $this->instances[$route_name][$plugin_id] = $plugin;
+        }
+      }
+    }

Yeah. We should try to compile this into each route so that every route knows which local actions appear on it.

dawehner’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/modules/system/lib/Drupal/system/Plugin/MenuLocalActionInterface.phpundefined
@@ -20,20 +20,4 @@
-  /**
-   * Returns the localized title to be shown for this action.
-   *
-   * Subclasses may add optional arguments like NodeInterface $node = NULL that
-   * will be supplied by the ControllerResolver.
-   */
-  public function getTitle();
-
-  /**
-   * Returns an internal Drupal path to use when linking to the action.
-   *
-   * Subclasses may add optional arguments like NodeInterface $node = NULL that
-   * will be supplied by the ControllerResolver.
-   */
-  public function getPath();

/me sighs ... Thanks for removing a solution we already found.

fubhy’s picture

Status:Needs work» Needs review

It's not a solution. We are treating these methods as controllers and we are calling them through controller resolver. We don't have interfaces for other controllers. Why do we want one for this? And if we do want one for this then why are we checking for is_callable() in the plugin manager? Having both doesn't make sense. And having an interface that does not have any arguments on a method and then explicitly saying "oh, but you should definitely add your own arguments there, but make them optional" is totally weird. It's a lie to say that those arguments are optional or have a default value. If your plugin depends on them, it depends on them.

dawehner’s picture

+++ b/core/modules/system/lib/Drupal/system/Plugin/Type/MenuLocalActionManager.phpundefined
@@ -0,0 +1,130 @@
+   * @throws BadMethodCallException

Should have a "\"

fubhy’s picture

StatusFileSize
new20.12 KB
PASSED: [[SimpleTest]]: [MySQL] 57,059 pass(es).
[ View ]
new2.42 KB

One last thing before I shut up: Why are we putting this in system module instead of Drupal\Core? I feel it really belongs in Drupal\Core instead.

tim.plunkett’s picture

Please drop the exceptions and go back to interfaces. We already spent several hours hashing this out...

fubhy’s picture

Why would we want to introduce a new pattern here? It's a total lie to have the implements go with $foo = NULL even though they actually require $foo. It's impossible to provide interfaces for controller-like methods as they are dynamic. Period. The interface is a contract. If the implementations can't live up to that promise then that's code-smell. Hence, we can't have an interface implementation for these methods. It makes no sense.

tim.plunkett’s picture

We already do this in FormInterface in dozens, eventually hundreds of places.

fubhy’s picture

Okay, we agreed on IRC that we will do the interface/no interface thing in a follow-up. Daniel is going to come back with a re-roll (I gotta run in a few, office closes).

dawehner’s picture

StatusFileSize
new3.99 KB
new19.97 KB
PASSED: [[SimpleTest]]: [MySQL] 57,121 pass(es).
[ View ]

This just reverts the interface bits.

fubhy’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC then

tim.plunkett’s picture

StatusFileSize
new1.45 KB
new21.46 KB
PASSED: [[SimpleTest]]: [MySQL] 56,986 pass(es).
[ View ]

#1978952: Convert shortcut_set_add to a Controller went in, so rerolling for that one.

My only worry here is that everywhere else, each plugin type gets its own subdirectory. But both this and #2004334: Separate Tabs (MENU_LOCAL_TASK) from hook_menu() use \Drupal\$provider\Plugin\Menu, not \Drupal\$provider\Plugin\MenuLocalAction and \Drupal\$provider\Plugin\MenuLocalTask.

Is that a problem? Have we tried combining the two patches?

dawehner’s picture

StatusFileSize
new998 bytes
new21.68 KB
PASSED: [[SimpleTest]]: [MySQL] 56,778 pass(es).
[ View ]

Thanks for the reroll.

To avoid confusion we should really switch to a different directory (I went with MenuLocalAction for now)

fubhy’s picture

Now that we are moving it anyways, can't we just move it to Drupal\Core?

pwolanin’s picture

Given that we have Drupal\system\Annotation\MenuLocalAction, I think MenuLocalAction is sensible as the directory name.

tstoeckler’s picture

We could also do Menu\LocalAction and Menu\LocalTask

fubhy’s picture

Or just leave the "Menu" part out entirely. ;) Bikeshed inc... But please let's not push this back further. Only thing I want to know is whether it's okay to move this to Drupal\Core after committing this version here that has been RTBCed already.

tim.plunkett’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new22.5 KB
new22.15 KB
PASSED: [[SimpleTest]]: [MySQL] 56,877 pass(es).
[ View ]
new22.5 KB
new22.15 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

After discussion with Eclipse, fubhy, and alexpott in IRC, we decided to do the namespace move now.

I also, per our discussion, added an alter.

tim.plunkett’s picture

ugh, the second set is rebased on HEAD. I meant to delete the first set. I'll cancel that test...

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Good try with this interdiff ...

+++ b/core/lib/Drupal/Core/Menu/LocalActionManager.phpundefined
@@ -104,9 +108,8 @@ public function getPath(MenuLocalActionInterface $local_action) {
    *
-   * @return array
-   *   Objects implementing MenuLocalActionInterface that appear on the route
-   *   path.
+   * @return \Drupal\Core\Menu\LocalActionInterface[]
+   *   An array of LocalActionInterface objects that appear on the route path.

+1

pwolanin’s picture

I'm happy with the namespace change here.

Dries’s picture

Status:Reviewed & tested by the community» Fixed

Despite the empty classes, this makes sense. Committed to 8.x. Thanks!

xjm’s picture

Title:Convert menu local actions to plugins so that we can generate dynamic titles and paths» [Change notice] Convert menu local actions to plugins so that we can generate dynamic titles and paths
Priority:Normal» Critical
Status:Fixed» Active
Issue tags:+Needs change record

Needs a change notice.

catch’s picture

+++ b/core/lib/Drupal/Core/Menu/LocalActionManager.phpundefined
@@ -0,0 +1,128 @@
+      // @todo - optimize this lookup by compiling or caching.
+      foreach ($this->getDefinitions() as $plugin_id => $action_info) {
+        if (in_array($route_name, $action_info['appears_on'])) {
+          $plugin = $this->createInstance($plugin_id);
+          $this->instances[$route_name][$plugin_id] = $plugin;
+        }
+      }
+    }
+    return $this->instances[$route_name];

Not sure this should've gone in with this particular @todo. Was this profiled? Is there a follow-up issue to add that compiling caching?

tim.plunkett’s picture

It used to say // @todo Consider storing the results of hook_local_actions() in a static.
I think this was just an updated version of that pre-existing @todo.

catch’s picture

That's swapping a hook invocation to a cache get though, is a problem for any info hook -> plugin conversion though I guess.

pwolanin’s picture

Since the final version is extending DefaultPluginManager, getDefinitions() is already cached - I think that changed in #1903346: Establish a new DefaultPluginManager to encapsulate best practices

So we probably just need to remove that @todo

pwolanin’s picture

Actually - instead of that @todo, we need a follow-up issue to flag routes that have a local action (or local task) appear when we do the router rebuild so that we have an easy way to skip calling this code when it's a no-op.

fubhy’s picture

I don't think flagging the routes is going to work as that basically requires re-running the route builder entirely every time you configure a local action/task (for example in views). I don't think we want that. Clearing the cached task/action definitions is much simpler. And with cached definitions we won't run into performance problems either. Yes, let's build set's on a per-route basis so we don't have to build up the list on run-time, but let's not write that into the route definitions.

pwolanin’s picture

yes, maybe that's what we need - to cache via the manager the actions/tabs that appear per route to avoid having to iterate to find them. I agree flagging the routes might not be the right option.

catch’s picture

Caching per route sounds good to me.

pwolanin’s picture

Title:[Change notice] Convert menu local actions to plugins so that we can generate dynamic titles and paths» Convert menu local actions to plugins so that we can generate dynamic titles and paths
Status:Active» Fixed
Issue tags:-Needs change record

updated the change notice at https://drupal.org/node/2007444

pwolanin’s picture

Priority:Critical» Normal
pwolanin’s picture

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