Convert menu local actions to plugins so that we can generate dynamic titles and paths

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

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

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

#famouslastwords

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

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.

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

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.

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

Should have a "\"

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.

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

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.

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

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

This just reverts the interface bits.

Back to RTBC then

#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?

Thanks for the reroll.

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

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

We could also do Menu\LocalAction and Menu\LocalTask

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.

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.

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

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

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

Needs a change notice.

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

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.

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

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.

Caching per route sounds good to me.