Add local action plugin deriver to use YAML discovery for static definitions

+++ b/core/lib/Drupal/Core/Menu/LocalActionManager.phpundefined
@@ -122,6 +122,7 @@ public function getActionsForRoute($route_name) {
+

Dude, what's with these weird unrelated changes. Maaaan!

+++ b/core/lib/Drupal/Core/Menu/Plugin/Derivative/StaticLocalActionDeriver.phpundefined
@@ -0,0 +1,116 @@
+   * Constructs a ViewsBlock object.

No... Just... No.

+++ b/core/lib/Drupal/Core/Menu/Plugin/Derivative/StaticLocalActionDeriver.phpundefined
@@ -0,0 +1,116 @@
+   * @param \Drupal\Core\Entity\EntityStorageControllerInterface $view_storage_controller
+   *   The entity storage controller to load views.

No, no, no...

Seems like we need a more generic framework for using yaml to define other-wise empty class definitions as derivatives of a base class?

Changing the base class now is an API change. Let's not do that.

+++ b/core/lib/Drupal/Core/Menu/Plugin/Derivative/StaticLocalActionDeriver.phpundefined
@@ -0,0 +1,117 @@
+class StaticLocalActionDeriver implements ContainerDerivativeInterface {

Ohhh I would have assumed it to use a different discovery mechanism. This one then could have been reused on local tasks as well.

Ohhh I would have assumed it to use a different discovery mechanism. This one then could have been reused on local tasks as well.

Together with #2050289: Add class to make yaml discovery reusable I think the approach of using a derivative is better. Not sure why, I just find it nice :).

Having multiple way to define such way potentially confuses people.

On the other hand doing it like it is proposed here, would really easy allow us to not change the API for now.

+++ b/core/lib/Drupal/Core/Menu/LocalActionBase.phpundefined
@@ -81,7 +81,6 @@ public function getRouteName() {
-    // Subclasses may pull in the request or specific attributes as parameters.

Why remove that damiboy? :(

+++ b/core/lib/Drupal/Core/Menu/Plugin/Derivative/StaticLocalActionDeriver.phpundefined
@@ -0,0 +1,117 @@
+   * Implements \Drupal\Component\Plugin\Derivative\DerivativeInterface::getDerivativeDefinition().

inheritdoc please, bro.

+++ b/core/lib/Drupal/Core/Menu/Plugin/Derivative/StaticLocalActionDeriver.phpundefined
@@ -0,0 +1,117 @@
+    // @todo Replace with a generic YAML discovery solution when we have one.

Maybe add issue number?

Otherwise looking good! Thanks!

I don't disagree with the better/easier DX at the moment, but I want to raise the point that having two basically equal levels will maybe confuse people.

but I want to raise the point that having two basically equal levels will maybe confuse people

You mean because you can provide a 'static' local action by providing a empty annotation class or a yaml file? Well, we can't really prevent that from happening, can we? Not without breaking the API at least.

I think this is good to go. Let's do the same for local tasks.

#2050919: Replace local task plugin discovery with YamlDiscovery

Now that I think of it, should we do local tasks and actions with a single .yml file instead of two?

If we are going this we should make sure the discovery code is generic and use it as well for local tasks

+++ b/core/lib/Drupal/Core/Menu/Plugin/Derivative/StaticLocalActionDeriver.phpundefined
@@ -0,0 +1,118 @@
+  public function getDerivativeDefinitions(array $base_plugin_definition) {
+    $parser = new Parser();
+
+    // @todo Replace with a generic YAML discovery solution when
+    // https://drupal.org/node/2050289 is complete.
+    foreach ($this->moduleHandler->getModuleList() as $module => $filename) {
+      $local_actions_file = DRUPAL_ROOT . '/' . dirname($filename) . '/' . $module . '.local_actions.yml';
+      if (file_exists($local_actions_file)) {
+        $local_actions = $parser->parse(file_get_contents($local_actions_file));
+
+        if (!empty($local_actions)) {
+          foreach ($local_actions as $name => $info) {
+            $this->derivatives[$name] = array(
+              'route_name' => isset($info['route_name']) ? $info['route_name'] : '',
+              'title' => isset($info['title']) ? $this->translationManager->translate($info['title']) : '',
+              'appears_on' => isset($info['appears_on']) ? $info['appears_on'] : array(),
+              'provider' => $module,
+            );
+
+            $this->derivatives[$name] += $base_plugin_definition;
+          }
+        }
+      }
+    }
+
+    return $this->derivatives;
+  }

Let's do this @todo now that #2050289: Add class to make yaml discovery reusable is in...

Edit: removed duplicated comment

Missing a use statement for Drupal\Component\Discovery\YamlDiscovery

+++ b/core/lib/Drupal/Core/Menu/Plugin/Derivative/StaticLocalActionDeriver.phpundefined
@@ -0,0 +1,111 @@
+            'route_name' => isset($info['route_name']) ? $info['route_name'] : '',
+            'title' => isset($info['title']) ? $this->translationManager->translate($info['title']) : '',

I think it is pointless to have a local action without a route/title

+++ b/core/lib/Drupal/Core/Menu/Plugin/Derivative/StaticLocalActionDeriver.phpundefined
@@ -0,0 +1,111 @@
+  public function __construct($base_plugin_id, ModuleHandlerInterface $module_handler, TranslatorInterface $translation_manager) {
...
+            'title' => $this->translationManager->translate($info['title']),

Sorry but it seems to be that we should typehint TranslationManager, as the method we use (translate()) is just defined on that one.

#2049709: TranslationManager::translate() should be on an interface

Just that whichever you do, there's a good chance you'll be wrong :)

Hehe!

Even I personally think it is wrong to provide two different basic discovery mechanisms for a plugin type the patch is fine. We are sort of in a state where an API break should better be avoided.

I don't think it's wrong to provide different discovery mechanism. Technically that is always possible anyways. Developers are not stupid either. You can even use static to provide a dynamic discovery derivative definition, Uh yeah... Umh. Anyways, this is totally fine. We have different discovery mechanisms all over the place (for the same things). Think about route definitions (dynamic / yaml based). That's actually exactly the same except that dynamic is event driven, not annotation driven...

So, RTBC+1 (personally thinking that this is perfectly fine) :).

+++ b/core/lib/Drupal/Core/Menu/Plugin/Derivative/StaticLocalActionDeriver.phpundefined
@@ -0,0 +1,111 @@
+        foreach ($local_actions as $name => $info) {
+          $this->derivatives[$name] = array(
+            'route_name' => $info['route_name'],
+            'title' => $this->translationManager->translate($info['title']),
+            'appears_on' => $info['appears_on'],
+            'provider' => $module,
+          );
+
+          $this->derivatives[$name] += $base_plugin_definition;
+        }

This bugs me a bit. You've got the local_action info but you're throwing anything that doesn't match to your expected parameters away, and that's not really how this works. Usually you want to do something more like $this->derivatives[$name] = $info + $base_plugin_definition;

As an example, you're completely disregarding what the yml said with regard to provider. If you look at Core's Annotation discovery we only fill out provider if it's not already populated. Likewise, plugin definitions can have other stuff hung off them for unanticipated use cases, and you've completely eliminated the possibility for that in any instance other than drupal_alter usage.

That aside, I'm very ++ to this. I wouldn't worry about the "multiple ways to do something" as that's a side effect of allowing derivatives into your plugin type in the first place. Most derivatives utilize the UI to define plugins, but this is just as valid (in fact Layout plugins do exactly this already and have for a very long time now). More power to you.

Eclipse

@@ -93,14 +93,9 @@ public function getDerivativeDefinitions(array $base_plugin_definition) {
+          $info += array('title' => '', 'provider' => $module);
+          $info['title'] = $this->translationManager->translate($info['title']);

Mhh we should tell people if either the route_name or title is missing they are doing something wrong?

re: 38

Yes that definitely solves my objections.

Eclipse

See also #2061777: Provide dynamic registration of local_action plugins

.

per dawahner - throws an exception if the definition is invalid.

@damiankloip - well, even from the yaml it could be a string instead of an array?

Should we at least cast it to array?

We should supply it if missing.

oops, I think we should supply it?

Well, either way - if you want it to be empty, you can use [] in yaml

I really like the current version, as it is way easier to read than previous versions!

+1

@damiankloip - at least at one point we were doing it to separate symfony vs. Drupal classes, though not sure if it's been standardized.

It's worth pointing out that the LocalAction Annotation class is already setting appears_on to an empty array, so you're going to get some sort of mismatch error if someone tries to pass a string in there based upon the derivative code that's adding the base plugin definition and the plugin definition from the yaml together. Just wanted to draw attention to that aspect. This code seems to have that covered as well, but no one had mentioned it. ++ing the rtbc.

Eclipse

Looks like a good change to me. Committed/pushed to 8.x, thanks!

Just being the tag fairy, don't mind me. :D

Adapted the change notice https://drupal.org/node/2007444/revisions/view/2768137/2801357

Should be revised to use #2065571: Add YAML Plugin discovery?

#2083415: [META] Write up all outstanding change notices before release

Updated issue summary.

Definitely time to close this one, the change notice at https://drupal.org/node/2007444 is looking good. Besides the revision by @dawehner last August we also have an extensive change by @pwolanin in October: https://drupal.org/node/2007444/revisions/view/2801357/2861533.