Use Context system for actions

Added issue summary.

klausi opened a new pull request for this issue.

Starting a WIP patch here. Trying to convert EmailAction to context values.

I could not find the test cases for this plugin, so let's see where the testbot fails.

I'm removing configuration forms in this patch, because action parameters should use the context system. Not sure how this will look in the UI, but I guess the caller should be responsible for building a UI from the context definitions. Or should we keep the configuration form and populate the context values from the config?

We are already using context aware action plugins in Rules by abusing the core action system as it fits our needs. At some point this will break (as the plugins are not compatible) and we will have to fork the core action system as Rules action system, but for now we can try to move the ideas from Rules into core.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Code looks good step forward, only views bulk ops are needs some fixes

+++ b/core/lib/Drupal/Core/Annotation/Action.php
@@ -51,12 +51,17 @@ class Action extends Plugin {
+   * The name of the module providing the action.
...
+  public $module;

provider is used all over plugins, maybe extension is better...

do profiles are able to expose action plugins?

+++ b/core/modules/action/src/Plugin/Action/EmailAction.php
@@ -113,63 +128,4 @@ public function execute($entity = NULL) {
-  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
-    $form['recipient'] = array(

I do not think we can just do away with that?

Also we'll need some test coverage for a contextual action.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Now including an ActionTestBase class using PHPUnit to also test the contents of the action plugin annotation and the plugin instantiation. Also inject ContextHandler properly from the service container.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

just a few nitpicks

1. +++ b/core/lib/Drupal/Core/Action/ActionBase.php
   @@ -18,14 +18,18 @@
   +      // @todo this assumes that the first context is the entity context of the
   

   needs issue #

2. +++ b/core/lib/Drupal/Core/Annotation/Action.php
   @@ -51,12 +51,17 @@ class Action extends Plugin {
   +   * The name of the module providing the action.
   ...
   +  public $module;
   

   should be provider, suppose install profiles and probably themes could provide action plugns

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Tests passing again and I also converted the remaining action plugins in action module, which seem to have no test coverage. I also addressed the points mentioned by andypost and we also have test coverage for contextual actions now as pointed out by fago.

There are a couple of @todos in the code left what we should do with context vs. configuration. There are now a few actions that can get their parameters either from context or from configuration values; but I think this patch is already large enough, so we should not address that also in this issue.

Thanks. Great to seeing the improved test-base from Rules being added also. Here some remarks:

1. +++ b/core/lib/Drupal/Core/Action/ActionBase.php
   @@ -18,14 +18,20 @@
      public function executeMultiple(array $entities) {
   

   Could we just add second parameter taking the context name for which the action should be multiple-executed?

   executeMultiple('user', $users)

   Then action implementations need an easy way to provide optimized methods for certain context parameters. But yeah, let's figure out details in a follow-up here.

2. +++ b/core/lib/Drupal/Core/Action/ActionInterface.php
   @@ -13,13 +13,19 @@
   + *   context definition is a typed data definition describing the context. Check
   + *   the typed data definition docs for details.
   

   Not fully true as context definitions use a slightly different format. Isn't enough to point to context definitions for docs?

3. +++ b/core/lib/Drupal/Core/Annotation/Action.php
   @@ -51,12 +51,10 @@ class Action extends Plugin {
   +  public $action = array();
   

   This seems wrong. it's $contexts, right?

4. +++ b/core/modules/action/src/ActionListBuilder.php
   @@ -79,7 +79,12 @@ public function load() {
   +      $value = (string) $value->getLabel()->get();
   

   What's the get() after getLabel() for?

5. +++ b/core/modules/action/src/Plugin/Action/EmailAction.php
   @@ -84,13 +102,23 @@ public static function create(ContainerInterface $container, array $configuratio
   +    $recipient = $this->getContextValue('recipient');
   +    if (!$recipient) {
   +      $recipient = $this->configuration['recipient'];
   

   Ouch. I think we'll have to clean this up before commit, i.e. only use contextual values. We can keep the configuration form, but use the configuration to pre-fill context in those cases maybe? I'd do move all configuration processing to a single method that's invoked in the beginning of execute() and writes configuration values into the context - so the rest can be clean-ed up then.

6. +++ b/core/modules/action/src/Plugin/Action/EmailAction.php
   @@ -84,13 +102,23 @@ public static function create(ContainerInterface $container, array $configuratio
   +    $recipient = $this->token->replace($recipient, $this->configuration);
   

   This should go in the configuration processing code also then.

7. +++ b/core/modules/action/src/Plugin/Action/MessageAction.php
   @@ -20,7 +20,17 @@
   + *     "entity" = @ContextDefinition("entity",
   + *       label = @Translation("Entity"),
   

   I do no think this action needs an entity, does it?

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

I think I addressed almost all comments from fago.

We need the entity context for those actions because they rely on an entity for token replacements.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

hm, an interdiff would be nice as there is no way to review intermediate commits with dreditor / posting comments to d.o. Anyway, here a review:

 * @param array $objects
* An array of entities. 	* An array of entities.
	+ * @param string $context_name
	+ * The name of the context that should be filled with the objects.
*/ 	*/
- public function executeMultiple(array $objects); 	+ public function executeMultiple(array $objects, $context_name);

Generically, this makes no sense. There may be no reference of entity, also objects should not be used either (what's that?). I'd suggest using the context name as first parameter also, as this is the logical ordering. Suggestion:

/**
 * Executes the action for multiple context values.
 * 
 * This method can be used for executing the action multiple times,
 * while allowing the action implementation to perform optimizations.
 * If the implementation provides no optimizations, the fallback behaviour
 * is to execute the action once per value.
 * 
 * @param $context_name
 *   The name of the context with multiple values.
 * @param mixed[] $context_values
 *    An array of context values, for which the action will be executed once per value.
 */
public function executeMultiple($context_name, array $context_values)

;

1. +++ b/core/modules/action/src/Plugin/Action/EmailAction.php
   @@ -21,7 +21,25 @@
     *   label = @Translation("Send email"),
   ...
   + *   context = {
   + *     "entity" = @ContextDefinition("entity",
   + *       label = @Translation("Entity"),
   + *       description = @Translation("The contextual entity that can be used for token replacements."),
   + *       required = false
   

   Ouch, having that still hurts. But well, no other way to do it until we've dedicated token replacement / data processing support.

2. +++ b/core/modules/action/src/Plugin/Action/EmailAction.php
   @@ -114,6 +134,37 @@ public function execute($entity = NULL) {
   +  protected function mapConfigToContext() {
   

   as it's called configuration else where here, it should be configuration here also imho.

3. +++ b/core/modules/action/src/Plugin/Action/GotoAction.php
   @@ -22,7 +22,12 @@
   + *     "url" = @ContextDefinition("string",
   

   Should be of type "uri" ?

4. +++ b/core/modules/system/src/ActionConfigEntityInterface.php
   @@ -22,17 +22,21 @@
   +  public function execute(array $entities, $context_name);
   

   Should use the same signature as on the action interface, but moreover - do we really need it here? One can just use the plugin instead?

5. +++ b/core/tests/Drupal/Tests/Core/Action/ActionTestBase.php
   @@ -0,0 +1,207 @@
   +        $namespaces = array('Drupal\\' . $name => DRUPAL_ROOT . '/core/modules/' . $name . '/src');
   

   Too much indentation.

Also we should update the summary with all concrete API changes.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Implemented remarks from fago, interdiff attached.

We cannot use "url" as data type for the GotoAction because an internal path such as "node/1" might not register as valid URL, so I left it simply as string.

Gnah, of course the interdiff upload got lost.

Thanks, this looks already good. I did another close review:

1. +++ b/core/lib/Drupal/Core/Action/ConfigurableActionBase.php
   @@ -60,4 +61,29 @@ public function calculateDependencies() {
   +          $value = $this->getContextValue($context_name);
   +        }
   +        catch (ContextException $e) {}
   

   The exception is only thrown for required contexts what means we have to check for non-NULL values else. But maybe best we could just add a parameter to getContextValue() to determine whether it it should throw exceptions? $fail_on_error ?

2. +++ b/core/modules/node/src/Plugin/Action/UnpublishByKeywordNode.php
   @@ -17,16 +17,31 @@
   + *     "keywords" = @ContextDefinition("string",
   + *       label = @Translation("Keywords"),
   + *       description = @Translation("The content will be unpublished if it contains any of the phrases above. Use a case-sensitive, comma-separated list of phrases. Example: funny, bungee jumping, ""Company, Inc.""")
   

   Should be multiple strings instead.

3. +++ b/core/modules/node/src/Plugin/Action/UnpublishByKeywordNode.php
   @@ -17,16 +17,31 @@
   +    $keywords = Tags::explode($keywords);
   

   Configuration value processing should be done on form submit as before.

@summary:

The execute() method on ActionConfigEntityInterface is removed.

This sounds quite arbitarary, I guess some short reasoning would be good to have there as well ($entity parameter gone?).

Also this needs a change record draft.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Fixed fago's remarks, interdiff attached.

EclipseGc mentioned in IRC that there’s a method on ContextHandler for this configuration to context mapping, so we could look into that as well.

1. +++ b/core/lib/Drupal/Core/Action/ConfigurableActionBase.php
   @@ -60,4 +61,29 @@ public function calculateDependencies() {
   +  protected function mapConfigurationToContext() {
   +    foreach ($this->getContextDefinitions() as $context_name => $definition) {
   +      // If there is a value in the configuration we populate the context with
   +      // it.
   +      if (isset($this->configuration[$context_name])) {
   +        $value = NULL;
   +        // If a required context is not set we have to catch ContextExceptions.
   +        try {
   +          $value = $this->getContextValue($context_name);
   +        }
   +        catch (ContextException $e) {}
   +        // Only populate the context if it has not been set explicitly before.
   +        if ($value === NULL) {
   +          $this->setContextValue($context_name, $this->configuration[$context_name]);
   +        }
   +      }
   +    }
   +  }
   

   The ContextHandler class has a helper for this. You shouldn't need this method.

2. +++ b/core/lib/Drupal/Core/Annotation/ContextDefinition.php
   @@ -101,6 +101,14 @@ public function __construct(array $values) {
   +    // Make sure that we don't pass Translation objects to the
   +    // ContextDefinition, it expects plain strings.
   +    foreach (array('label', 'description') as $translatable) {
   +      if ($values[$translatable] instanceof Translation) {
   +        $values[$translatable] = (string) $values[$translatable]->get();
   +      }
   +    }
   

   I ran into the same issue with #2339151: Conditions / context system does not allow for multiple configurable contexts, eg. language types. I fixed it in the Translation Annotation since it's that's class' job to make it's return usable.

3. +++ b/core/modules/action/src/Plugin/Action/EmailAction.php
   @@ -21,7 +21,25 @@
   + *     "recipient" = @ContextDefinition("string",
   

   don't we have an email address data type?

4. +++ b/core/modules/action/src/Plugin/Action/GotoAction.php
   @@ -22,7 +22,12 @@
   + *     "url" = @ContextDefinition("string",
   

   I think we also have a url data type.

5. +++ b/core/modules/action/src/Plugin/Action/MessageAction.php
   @@ -49,12 +59,31 @@ public static function create(ContainerInterface $container, array $configuratio
   +    $entity = $this->getContextValue('entity');
   +    if (empty($this->configuration['node']) && $entity) {
   

   This seems really weird. Why are we setting the node key to $entity when we clearly expect generic entities?

6. +++ b/core/modules/action/src/Plugin/Action/MessageAction.php
   @@ -49,12 +59,31 @@ public static function create(ContainerInterface $container, array $configuratio
   +    // First check if a message context value is set, otherwise use the value
   +    // from the configuration.
   +    $message = $this->getContextValue('message');
   +    if (!$message) {
   +      $message = $this->configuration['message'];
   +      $message = $this->token->replace(Xss::filterAdmin($message), $this->configuration);
   +      $this->setContextValue('message', $message);
   +    }
   

   Doing all of this in the protected mapConfigurationToContext method will have to change as you move to using the ContextHandler.

7. +++ b/core/modules/node/src/Plugin/Action/AssignOwnerNode.php
   @@ -60,10 +68,20 @@ public static function create(ContainerInterface $container, array $configuratio
   +    $user = $this->getContextValue('user');
   

   I think this needs a try/catch cause getContextValue() throws an exception if context is not set and is required. Worth pointing out you could set that the "user" context is not required in the annotation. I think Fago mentioned something similar with regard to a parameter to NOT throw the exception. If you don't want the exception, then this is by definition an optional context, and the ContextDefinition annotation and class support, that, please use that instead of adding some switch parameters to the generic ContextAware Plugin classes.

8. +++ b/core/modules/node/src/Plugin/Action/UnpublishByKeywordNode.php
   @@ -17,16 +17,31 @@
   + *     "keywords" = @ContextDefinition("string",
   + *       label = @Translation("Keywords"),
   + *       description = @Translation("The content will be unpublished if it contains any of the phrases above. Use a case-sensitive, comma-separated list of phrases. Example: funny, bungee jumping, ""Company, Inc.""")
   + *     )
   

   I've seen this a couple times now in a couple of different permutations and didn't immediately pick up on it. This looks like it's probably Fago's "configuration context" thing that he wants. It's fine that he wants this, and I support doing it, but we should probably come up with a separate key and not conflate these and regular execution contexts together.

In general, the changes in this patch make a ton of sense, but it still needs a little work. This is a review of 42 since 46 was published while I was doing this. Looking at the interdiff, I don't see anything that invalidates any of my criticisms.

Eclipse

Oh, also I would expect you don't have enough test coverage for 7 if things aren't blowing up.

Eclipse

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Thanks for the review!

1. ContextHandler::applyContextMapping() does not make sense here since we would have to build up artificial context objects just to invoke this method. That would just bloat the code for this easy task?

2. In #2339151: Conditions / context system does not allow for multiple configurable contexts, eg. language types the Translation Annotation is not modified, you also fixed it in the ContextDefinition Annotation. Copied the code from there.

3. Right, since the token replacement is now only done on the configuration, we can make this an actual email data type. Fixed.

4. We cannot use the url data type for the GotoAction because it can be an internal path like "node/1", which is not a valid URL. Leaving as string.

5. Yep, using entity when we actually only mean nodes seems wrong. Changed it to node.

6. Not using ContextHandler, so nothing to change here.

7. The user context is required, because we have to have a user to be set as owner of the node. We can't do without user, right? So we just expect that it is always there. If there are exceptions it is the callers fault. We are not adding any switch parameters to the ContextAwarePluginBase either here.

8. There is no difference between configuration context and runtime context. All parameters the plugin needs for the execution should be in context objects. So we should not invent any new keys here, since everything is just context to the plugin.

Adding test coverage for the AssignOwnerNode action plugin is out of scope for this issue, we should open a new one if there is work remaining after this.

This looks like it's probably Fago's "configuration context" thing that he wants

.
Yep, we need it for Rules.

It's fine that he wants this, and I support doing it, but we should probably come up with a separate key and not conflate these and regular execution contexts together.

I agree that there needs some information on what's the default intention - i.e. should it be configured or passed on the fly. Maybe we can look into adding some additional key to the context definition or so in a follow up?

@Fago

I'd prefer it if were were to just introduce a separate key in the plugin definition for this instead of adding it to the context definition since run-time context vs configuration isn't a distinction the ContextDefinition objects should care about. This is all about how you're using it.

@Klausi

1.) There's an expected configuration key of context_mapping that maps the name of the context from the context array to be used. Gathering the contexts array is the job of the system which will be executing the actions. Passing the contexts array that is gathered by that system plus the plugin & it's configuration to the ContextHandler is the appropriate way forward here, unless I misunderstand what you're doing very badly.

2.) My bad, misspoke.

4.) :-(

5.) I read that is being able to be any entity. I was suggesting you change the array key to 'entity'. If this actually only uses nodes, then ++ to your change.

6.) See 1

7.) No, you clearly do an "if ($user)" check after that and if it's not there you fallback to a configuration value. That is absolutely an "optional" context.

8.) See my response to fago.

Eclipse

Gathering the contexts array is the job of the system which will be executing the actions.

I agree, but we are far from that in core, that's why the action plugins have to supply themselves with context that is stored in their configuration for now.

A complete implementation for the BulkForm example use case would require us to pull out stored context configuration from the plugin, turn it into a context array, pass that on to ContextHandler, create a context mapping. I think this is out of scope for this issue. We can't do everything at once. Let's continue with incremental steps since you said yourself that this is a large improvement already. Ready for RTBC?

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

That was just a reroll.

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

DRUPAL_ROOT does not exist anymore in PHPUnit tests, switched to $this->root which also required a slight change of order when registering modules/namespaces. Same code as used in Rules 8.x-3.x.

I'd prefer it if were were to just introduce a separate key in the plugin definition for this instead of adding it to the context definition since run-time context vs configuration isn't a distinction the ContextDefinition objects should care about. This is all about how you're using it.

Imo, context vs configuration isn't a distinction the plugin should have to differentiate upon - the plugin should rely on getting it's context set from outside and be able to run from there. So all we need is a way for having defaults for generating forms for pre-configuring context values on configuration time or mapping contexts, i.e. the configuration form. But, it doesn't really make much sense to me to provide another API for getting configuration values vs context values when it's all about getting a value the plugin needs for execution, i.e. it's context (mapped + pre-configured).

But anyway, I experimented with separate context + configuration definition buckets and figured that it's end up being problematic with ordering. Given there are two different buckets for configuration and context we loose the native ordering, e.g. on the configuration form. And yes, Rules has use-cases for optional (by default contextually mapped) arguments being less important and at the end of the form, what means we'd have to introduce some weights-fu for getting back a common ordering. Thus, having it in bucket just makes our lives a lot easier here.
Related to the ordering, but not directly related to this issue, I still think about experimenting with base classes providing better DX and auto-completion, e.g. by supporting methods like executeWithContext(EntityInterface $entity, array $some_array) (arbitrary example) - for which we need the common ordering as well.

1. +++ b/core/lib/Drupal/Core/Action/ActionBase.php
   @@ -18,14 +18,15 @@
   +  public function executeMultiple($context_name, array $context_values) {
   +    foreach ($context_values as $context_value) {
   +      $this->setContextValue($context_name, $context_value);
   +      $this->execute();
        }
   

   This seems woefully insufficient for the way Actions handle contexts in both config and traditional contexts, furthermore, I don't think this will properly execute for an action that genuinely needs multiple contexts. The various permutations simply aren't happening here.

2. +++ b/core/lib/Drupal/Core/Action/ConfigurableActionBase.php
   @@ -60,4 +61,31 @@ public function calculateDependencies() {
   +  protected function mapConfigurationToContext() {
   +    foreach ($this->getContextDefinitions() as $context_name => $definition) {
   +      // If there is a value in the configuration we populate the context with
   +      // it.
   +      if (isset($this->configuration[$context_name])) {
   +        $value = NULL;
   +        // If a required context is not set we have to catch ContextExceptions.
   +        // @todo Use a isSet() method on the context object instead of catching
   +        // exceptions. See https://www.drupal.org/node/2367121
   +        try {
   +          $value = $this->getContextValue($context_name);
   +        }
   +        catch (ContextException $e) {}
   +        // Only populate the context if it has not been set explicitly before.
   +        if ($value === NULL) {
   +          $this->setContextValue($context_name, $this->configuration[$context_name]);
   +        }
   +      }
   +    }
   +  }
   

   Totally unnecessary. The ContextHandler class has methods for doing this sort of stuff already. Please us that.

3. +++ b/core/modules/action/src/Plugin/Action/EmailAction.php
   @@ -114,6 +134,37 @@ public function execute($entity = NULL) {
   +   * Maps configuration values to the context values if they are not set.
   +   *
   +   * @todo The configuration options should be removed here and the action
   +   * should solely depend on context.
   +   */
   +  protected function mapConfigurationToContext() {
   +    $entity = $this->getContextValue('entity');
   +    if (empty($this->configuration['node']) && $entity) {
   +      $this->configuration['node'] = $entity;
   +    }
   +
   ...
   +    // from the configuration.
   +    $recipient = $this->getContextValue('recipient');
   +    if (!$recipient) {
   +      $recipient = $this->configuration['recipient'];
   +      $recipient = $this->token->replace($recipient, $this->configuration);
   +      $this->setContextValue('recipient', $recipient);
   +    }
   +
   +    $subject = $this->getContextValue('subject');
   +    if (!$subject) {
   +      $this->setContextValue('subject', $this->configuration['subject']);
   +    }
   +    $message = $this->getContextValue('message');
   +    if (!$message) {
   +      $this->setContextValue('message', $this->configuration['message']);
   +    }
   +  }
   +
   +  /**
   

   This is a classic "optional context". You don't need this custom logic in the mapper, just use the ContextHandler and you can set sane fallbacks in the plugin itself.

4. +++ b/core/modules/node/src/Plugin/Action/AssignOwnerNode.php
   @@ -19,7 +19,15 @@
   + *   context = {
   + *     "node" = @ContextDefinition("entity:node",
   + *       label = @Translation("Node")
   + *     ),
   + *     "user" = @ContextDefinition("entity:user",
   + *       label = @Translation("User"),
   + *       description = @Translation("The user to which you would like to assign ownership.")
   + *     )
   + *   }
   

   Perfect example of what I was saying about executeMultiple earlier. If you have a list of nodes and users, you have no way to actually set this for all of them reliably. You could set all nodes to a particular user, or assign a different owner to the same node over and over. Other options are limited.

   Also, looking at your execute() logic, user should be an optional context.

I didn't get to give this all the attention it deserved, but there's a lot still wrong with this patch. We should not have competing Contextual Plugin approaches in core. Conditions does much of this same stuff very well, and #2377757: Expose Block Context mapping in the UI is the next patch I'm working on for blocks. Let's try to have a unified approach here.

Eclipse

sorry :-(

2 actionable items identified by Eclipse in IRC:

• remove mapConfigurationToContext() and try to use ContextHanlder
• make context optional if there is a fallback in the configuration

+++ b/core/modules/action/src/Plugin/Action/MessageAction.php
@@ -49,12 +59,31 @@ public static function create(ContainerInterface $container, array $configuratio
+    $node = $this->getContextValue('node');
+    if (empty($this->configuration['node']) && $node) {
+      $this->configuration['node'] = $node;
+    }

Doesn't this prevent you from being able to effectively executeMultiple() and change the node you want to extract tokens from?

Tried to look at this again today and how we could use ContextHandler in the BulkForm use case, but it all does not make sense to me. I'm sorry, I have to give up at this point. Feel free to pick this up and walk it to completion!

If there is no good way to use the context mapping in vbo, why bother with it? Main goal must be to have context-aware action plugins such that they are re-usable and can be a base for Rules. Having better VBO integration shouldn't block this!

I'll try to give this some time in the next couple days.

Now that #2172017: Bulk operations does not respect entity access has landed it becomes more and more clear that core action plugins are quite different than Rules action plugins. The function signature of the access() method with $object does not make sense for Rules.

Core focuses on one "object" the action is about (usually an entity), while Rules actions can have multiple objects as context that are equally important. Core actions have the concept of configuration with them, while in Rules an action never has configuration. In Rules everything the action plugin receives from the outside is handled with the Context API, which is populated when the action plugin is executed. No configuration is stored with the action plugin, all mapping configuration is stored outside in Rules config entities.

So IMO it is time to go separate ways with action plugins now and invent @RulesAction plugins in contrib. This will be a bit annoying for other contrib authors since they will have to implement @Action and @RulesAction plugins for their modules, but I think we cannot avoid that with the different goals that we have.

@klausi: Is there some way we can change @Action plugins so that there is no need for a seperate @RulesAction plugin.

Now that #2172017: Bulk operations does not respect entity access went in, we'll need to fix that to work context based as well. We'll need the same for conditions though, so maybe should work on adding it to conditions first and contextualize the API that and then pick up the same solution here?

Feature request => 8.1.x.

I took a closer look at this also and tried to come up with a good solution leveraging the core context handler. However, this and the multiple execution feature of actions makes it really hard to do so - I've not been able to come up with a good way of doing this without mixing up the preparing execution environment step and multiple execution. However, the preparation is the job of the caller, the multiple execution the job of the plugin...

I'm not sure how this multiple execution feature can work without coupling the plugin with knowledge of how its context gets set (That said, there is already getContextMapping() on ContextAwarePluginInterface what seems wrong to me also).

That said, it looks like this has to be postponed indeed and Rules will have to do its own context aware actions API :-/ Maybe we are able to figure a solution here once the Rules context configuration API has matured and it's easier to see the big picture of how context & configuration work together.

For record or later, here is my WIP patch based upon klausi's latest one. It's WIP and has some left-over merge issues, but should show where I was headed.

Straight re-roll, I think there is still quite a bit todo here, I'm not sure why the work dropped off, maybe we were waiting on something else to happen in core first?
@fago, if you want to discuss, ping me on IRC, happy to help with this.

Rerolling patch

run bot

Fixed bugs in patch from #91 commit

patch for comment #93 rerolled

Trying to re-rorll the latest patch

Tried to fix failed test

The patch needs a reroll as it is still using drupal_set_message.

1. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/MessageAction.php
   @@ -78,9 +88,30 @@ public static function create(ContainerInterface $container, array $configuratio
   -  public function execute($entity = NULL) {
   ...
   +  public function execute() {
   

   This is a public method we can't straight away change this without deprecation warning.

2. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/MessageAction.php
   @@ -78,9 +88,30 @@ public static function create(ContainerInterface $container, array $configuratio
   +   * @todo The configuration options should be removed here and the action
   +   * should solely depend on context.
   

   All these @todos at least need an issue to properly deprecate config option.

Added reroll of patch of #106.

Mistakenly added blank comment #109.

Also, see change record: Plugins now use the 'context_definitions' key to define their contexts

Meantime core enabling PHP attributes, so actions should get new contexts #3252386: Use PHP attributes instead of doctrine annotations

Action module is approved for removal. See #3266458: [Policy] Deprecate Action (UI) module in D10 and move to contrib in D11

This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

It will be moved to the contributed Action project when the Drupal 11 branch is open.

@quietone is this issue affected? its just actions UI due for removal?

Oops, you are right. I was on 'automatic'. Feel free to change the status on any others I did incorrectly. I think I'll take a break and then come back to this task.

Moving Non UI issue to the base system

Reviving this as I believe Action plugins may be a great pathway to unlock core and contrib en masse as AI tools. However, to do so, actions would need to be able to communicate required contexts.

Much has changed since the last patch, so I tried to work through and map changes accordingly.

I did change the approach for:
Drupal\Core\Action\ActionBase::executeMultiple with the hopes of better backward compatibility.

Original:

/**
   * {@inheritdoc}
   */
  public function executeMultiple(array $entities) {
    foreach ($entities as $entity) {
      $this->execute($entity);
    }
  }

From most recent patch:

  public function executeMultiple($context_name, array $context_values) {
    foreach ($context_values as $context_value) {
      $this->prepareExecutionContext([$context_name => $context_value]);
      $this->execute();
     }

New:

/**
   * {@inheritdoc}
   */
  public function executeMultiple(array $contexts) {
    $context_names = array_keys($this->getContextDefinitions());
    foreach ($contexts as $context_values) {
      if (!empty($context_names) && $context_values instanceof EntityInterface) {
        // Assume a single context of type entity exists.
        $context_values = [$context_names[0] => EntityContext::fromEntity($context_values)];
      }
      // Assume the plugin hasn't been upgraded.
      else {
        $this->execute($context_values);
        continue;
      }
      $this->prepareExecutionContext($context_values);
      $this->execute();
    }
  }

Based on this change, I was looking for feedback of how:
Drupal\user\Plugin\Action\CancelUser and Drupal\user_batch_action_test\Plugin\Action\BatchUserAction used execute and executeMultiple

I also wasn't sure if I needed to add constraints as well?

Looks like it still needs work around access calls.

As a reminder, Rules *still* has context-aware actions working, along the lines being proposed here, with extensive test cases.

This issue was opened because the development of Rules in the Drupal 7->8 transition identified functionality that was needed for Rules and would be useful to core as well. Back then @fago and @klausi were actively involved in developing Rules and were leveraging that work to contribute to the still-evolving core Drupal 8, and this issue was part of that effort.

The patches in here contain a lot of (old) code developed for Rules. This issue seems to have lost traction for inclusion into core when @fago stopped pushing this issue back in 2015 and put the needed functionality directly into Rules rather than wait for core to change.

As prophesized in #9, RulesAction plugins are now a fork of the core Action plugins, made context-aware. If anyone is interested, the commit that did this in the Rules repository was 55fe6db1, and it is similar to what was offered in #75 above. It's a shallow fork, done only because subclassing to get the needed functionality was not possible.

Rules implemented the functionality discussed here long ago. It works. And that functionality (and tests) have been maintained, improved, and ported to Drupal 9, 10, and 11 and is currently being tested weekly on GitLab CI. Rules and Rules-related modules supply hundreds of working examples of these plugins, with many examples and lots of documentation.

I would suggest to anyone who has a need / use case for context-aware actions that you try the Rules implementation as an archetype of how this can be implemented. There's a huge amount of commonality between what Rules has already done and what is being discussed here, precisely because this issue was opened to enable functionality needed by Rules.

I would be happy to help move this Rules functionality into an MR for core, but it would help to first have some feedback on which features should be in core, which should remain in Rules, and what is missing from Rules that core would need.

The newly-changed "Remaining tasks" in the issue summary are not described anywhere, and are not really remaining tasks but rather feature requirements?

I think the first step to make progress here needs to be to open up an MR with a concrete set of changes that can be tested and discussed - there is currently only an old patch file that is horribly out-of-date - for example it doesn't even account for the core change from "context" to "context_definitions" made back in 2019, or the change from Annotations to Attributes made in core last year.

But to do that we need a well-written issue summary stating exactly what the goal of this issue is. The "Problems" section and "Proposed resolution" section still seem relevant (and are still addressed by what was done in Rules), but the "Tasks" and "API changes" sections need clarity.

As I said in #126, I'm willing to offer an MR extracted from Rules, but I would like a little clarity here on what people are looking for from Core.

I removed the remaining tasks for now, it was previously set to 'Review Patch', but that definitely was not sufficient.

In the fork I created, the things I was still sorting through:

• execute / executeMultiple solution and infinite loops
• Node actions extending entity agnostic Drupal\Core\Field\FieldUpdateActionBase, need means to know name of entity context
• Backwards compatibility with Action plugins from contributed modules.

I appreciate the comments and I'll look at the Rules approach to it.

The improved context information would actually be very helpful for ECA as well, not only because the significant number of action plugins that come with it would be of real value to other ecosystems then as well.