This is a spinoff from #1008166-35: Actions should be a module in which @sun stated

As soon as the plugin system lands, actions should totally be plugins ;)

The new actions module soon will be committed and the plugin system has landed. Hence, it is time to convert the first action to use the plugin sub-system.

The purpose of this issue is to convert one action defined in the new actions module (as an example) and to perform a complete review before the others are potentially converted to use plugins as well.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Issue tags: +#pnx-sprint

Tagging

sun’s picture

Component: plugin system » action.module
Issue tags: +Plugin system
andypost’s picture

@sun how actions-plugins could relate to entities when they are related to?
What kind of manager we need to manage list of actions and their relation to entities?

Suppose this will help to progress on #1823574: [Meta] Improve the Views Bulk Operations (VBOs) that are in core

kotnik’s picture

Status: Active » Needs work
FileSize
44.04 KB

The attached patch is to get the party started, to set the initial structure for plugins, and an example how action plugins should be implemented.

This is incomplete patch, and more eyes are welcome to provide direction. Please do comment.

kotnik’s picture

Same as in #4, but somewhat polished patch.

sun’s picture

This looks like an awesome start, @kotnik!

Btw, I've seen that you marked the issue "needs work" and indeed, the patch looks more like a non-functional prototype thus far. In such cases, make sure to suffix your patch file with "do-not-test.patch"; i.e., whatever.blub-blah.blub.do-not-test.patch :)

+++ b/core/modules/action/action.module
@@ -244,12 +246,10 @@ function actions_do($action_ids, $object = NULL, $context = NULL, $a1 = NULL, $a
 function action_list($reset = FALSE) {
   $actions = &drupal_static(__FUNCTION__);
   if (!isset($actions) || $reset) {

The static variable cache can be removed here - the plugin manager retrieves definitions only once already, unless I'm mistaken.

+++ b/core/modules/action/action.module
@@ -361,22 +361,23 @@ function action_synchronize($delete_orphans = FALSE) {
+        // @todo: Store actions.
...
+        //     'callback' => $callback,

I think this can be easily kept: Just store the plugin ID instead of $callback?

+++ b/core/modules/action/lib/Drupal/action/ActionBase.php
@@ -0,0 +1,46 @@
+  public function getConfig() {
...
+      $this->configuration = $definition['settings'];
...
+    return $this->configuration;
...
+    $this->getConfig();

Shouldn't $this->configuration be set only once in the constructor? (I don't know what our current practice with regard to configurable plugins is)

+++ b/core/modules/action/lib/Drupal/action/ActionBase.php
@@ -0,0 +1,46 @@
+  public function configure($form, &$form_state) {
...
+  public function configureValidate($form, &$form_state) {}
...
+  public function configureSubmit($form, &$form_state) {}

Would it make sense to rename these to:

getForm()
validateForm()
submitForm()

?

(We should leave room for a non-form-related ::validate() method.)

+++ b/core/modules/action/lib/Drupal/action/ActionInterface.php
@@ -0,0 +1,41 @@
+  public function execute($param, array $context);

I've seen you started to convert the entity-related actions into plugins first.

That's an interesting aspect, actually. Quite a couple of those entity-specific plugins don't actually have to be entity-specific.

I wonder whether it would make sense to split all actions into entity-based and entity-unrelated actions?

That, in turn, would allow us to potentially have two separate interfaces; i.e., EntityActionInterface and GenericActionInterface (or similar).

Whereas the EntityActionInterface could take much more focused parameters in its ::execute() method; i.e.:

public function execute(EntityInterface $entity, array $context);

I'm not sure whether this idea is possible to do directly for this issue or whether it should be a follow-up — what do you think?

+++ b/core/modules/comment/comment.module
@@ -1803,114 +1803,24 @@ function comment_action_info() {
   return array(
     'comment_publish_action' => array(

Aren't the hook_action_info() implementations obsolete with this patch already? If so, let's remove them entirely.

kotnik’s picture

Title: Convert single action to plugin sub-system » Convert actions to plugin sub-system

@sun: thanks for in-depth review, I am going further with this patch.

In the mean time, updating issue title to reflect the actual work inside it.

fago’s picture

I wonder whether it would make sense to split all actions into entity-based and entity-unrelated actions?

I think the proper way for that to work is to follow the Rules-way: Support any number of parameters for actions, which are described in the action info. We have the foundation for describing the parameters in core now: the typed data API.

I'd love to go for re-done actions API that also fits the needs of Rules for that. I prepare a summary of how I see that working.

bojanz’s picture

EDIT: Nevermind.

andypost’s picture

Show message, send mail, run cron - very useful actions.

fago’s picture

finally wrote it down, see #1846172: Replace the actions API

damiankloip’s picture

Also see the above issue fago mentions, for the sandbox we have started. Which ultimately has actions as plugins anyway already.

Gábor Hojtsy’s picture

I'm afraid we might be falling between two stools. Actions are not plugins, this issue was was stopped being worked on I guess in favor of #1846172: Replace the actions API, while that has also not been progressing lately. What we have on the other hand is very custom DB storage for very custom code and actions neither using plugins, nor config entities (both of which they should). I'm afraid if we are shooting too high and not reaching those goals, then we'll just not move actions forward in any way. That would mean actions would not have any use in core, and could just as well be removed.

(The D8MI perspective is actions in core like dsm or send email should be translatable but there is no way now since they are stored in a blob field serialized, and no general config entity system is used so we can affect this in any way).

fago’s picture

#1743686: Condition Plugin System got fixed, so with the needed API in place already. What needs to be done for #1846172: Replace the actions API is basically doing a base class for actions + interface just as for conditions and converting existing actions over.

tim.plunkett’s picture

Status: Needs work » Closed (duplicate)

I made a ton of progress on #1846172: Replace the actions API (still yet to post my writeup and code...)
But to prevent any more duplicate work this coming week, I'm closing this one.