Problems

  • Having a single parameter for actions is not sufficient, nor is it possible to add further metadata to the single parameter (e.g. to define whether it is required)
  • We need to support actions without an entity paramter.
  • Actions should follow the same API as conditions if there are no reasons to do it different.
  • The action subsystem is coupled to entities.
  • Finally, this is required if the action system should be useful to Rules as well.

Proposed resolution

Make the action sub-system using context, such as conditions.

Remaining tasks

Patch review.

User interface changes

-

API changes

API changes in this issue are approved by the comments on ActionInterface.php.

  1. The 'type' parameter of action annotations is replaced by a proper context definition.
  2. The execute() method on ActionConfigEntityInterface is removed.
  3. The executeMultiple() method signature on ActionInterface is changed to additionally take a context name argument as first parameter.

Original report by tim.plunkett

See #1846172: Replace the actions API, it currently passes entities around directly, and uses hardcoded type = node

CommentFileSizeAuthor
#110 2011038-110.patch46.42 KBravi.shankar
#106 interdiff.2011038.105-106.txt8 KBaleevas
#106 2011038-9.1-106.patch46.7 KBaleevas
#105 interdiff.2011038.100-105.txt48.18 KBaleevas
#105 2011038-9.1-105.patch46.7 KBaleevas
#100 2011038-100.patch78.96 KBvacho
#93 use_context_system_for-2011038-93.patch62.69 KBMerryHamster
#91 2011038-81.patch70.35 KBbasil_snowman
#88 2011038-81.patch59.33 KBbasil_snowman
#86 2011038-81.patch58.66 KBbasil_snowman
#81 2011038-81.patch58.66 KBMerryHamster
#78 2011038-78.patch70.43 KBbenjy
#75 d8_actions.interdiff.txt11.84 KBfago
#75 d8_actions.patch70.46 KBfago
#59 interdiff.txt3.6 KBklausi
#58 2011038.patch69.36 KBklausi
#55 2011038.patch69.38 KBklausi
#51 interdiff.txt4.54 KBklausi
#50 2011038.patch70.49 KBklausi
#47 interdiff.txt5.74 KBklausi
#46 2011038.patch70.7 KBklausi
#44 interdiff.txt11.49 KBklausi
#42 2011038.patch72.79 KBklausi
#41 2011038.patch72.79 KBklausi
#36 2011038.patch72.31 KBklausi
#33 2011038.patch69.33 KBklausi
#30 2011038.patch62.45 KBklausi
#28 2011038.patch61.88 KBklausi
#26 2011038.patch61.88 KBklausi
#24 2011038.patch56.66 KBklausi
#21 2011038.patch53.03 KBklausi
#19 2011038.patch48.97 KBklausi
#17 2011038.patch30.68 KBklausi
#15 2011038.patch29.5 KBklausi
#11 2011038.patch24.74 KBklausi
#8 2011038.patch7.59 KBklausi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jibran’s picture

Status: Postponed » Active
fago’s picture

Priority: Normal » Major
Issue summary: View changes
Issue tags: +Rules 8.x, +Contributed project blocker, +beta target
tim.plunkett’s picture

Let's explain why this is major

tim.plunkett’s picture

Category: Task » Feature request
fago’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Added issue summary.

fago’s picture

Issue summary: View changes
Issue tags: +d8rules
fago’s picture

Issue summary: View changes
klausi’s picture

Status: Active » Needs review
FileSize
7.59 KB

klausi opened a new pull request for this issue.

klausi’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 8: 2011038.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
24.74 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Status: Needs review » Needs work

The last submitted patch, 11: 2011038.patch, failed testing.

andypost’s picture

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?

fago’s picture

+++ 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’s picture

Status: Needs work » Needs review
FileSize
29.5 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Status: Needs review » Needs work

The last submitted patch, 15: 2011038.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
30.68 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Status: Needs review » Needs work

The last submitted patch, 17: 2011038.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
48.97 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Status: Needs review » Needs work

The last submitted patch, 19: 2011038.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
53.03 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 21: 2011038.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
56.66 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

andypost’s picture

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’s picture

FileSize
61.88 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Status: Needs review » Needs work

The last submitted patch, 26: 2011038.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
61.88 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Status: Needs review » Needs work

The last submitted patch, 28: 2011038.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
62.45 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

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.

fago’s picture

Status: Needs review » Needs work

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’s picture

Status: Needs work » Needs review
FileSize
69.33 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 33: 2011038.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
72.31 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

Status: Needs review » Needs work

The last submitted patch, 36: 2011038.patch, failed testing.

Status: Needs work » Needs review

klausi queued 36: 2011038.patch for re-testing.

fago’s picture

Status: Needs review » Needs work

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.

fago’s picture

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

klausi’s picture

Status: Needs work » Needs review
FileSize
72.79 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

FileSize
72.79 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

Issue summary: View changes

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.

klausi’s picture

FileSize
11.49 KB

Gnah, of course the interdiff upload got lost.

fago’s picture

Status: Needs review » Needs work

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’s picture

Status: Needs work » Needs review
FileSize
70.7 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

FileSize
5.74 KB

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.

EclipseGc’s picture

Status: Needs review » Needs work
  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

EclipseGc’s picture

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

Eclipse

klausi’s picture

Status: Needs work » Needs review
FileSize
70.49 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

FileSize
4.54 KB

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.

fago’s picture

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?

EclipseGc’s picture

@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

klausi’s picture

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’s picture

FileSize
69.38 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

That was just a reroll.

Status: Needs review » Needs work

The last submitted patch, 55: 2011038.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
69.36 KB

klausi pushed some commits to the pull request.

For an interdiff please see the list of recent commits.

klausi’s picture

FileSize
3.6 KB

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.

fago’s picture

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.

EclipseGc’s picture

  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

EclipseGc’s picture

Status: Needs review » Needs work

sorry :-(

klausi’s picture

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
EclipseGc’s picture

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

kim.pepper’s picture

klausi’s picture

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!

fago’s picture

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!

EclipseGc’s picture

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

klausi’s picture

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.

daffie’s picture

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

fago’s picture

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?

jhedstrom’s picture

Version: 8.0.x-dev » 8.1.x-dev

Feature request => 8.1.x.

fago’s picture

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.

fago’s picture

fago’s picture

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.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Related issues: +#2701829: Extension objects should not implement \Serializable
benjy’s picture

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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

MerryHamster’s picture

Rerolling patch

tim.plunkett’s picture

Status: Needs work » Needs review

Let's see what the testbot thinks.

The last submitted patch, 75: d8_actions.patch, failed testing.

The last submitted patch, 78: 2011038-78.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 81: 2011038-81.patch, failed testing.

basil_snowman’s picture

andypost’s picture

Status: Needs work » Needs review

run bot

basil_snowman’s picture

FileSize
59.33 KB

The last submitted patch, 86: 2011038-81.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 88: 2011038-81.patch, failed testing.

basil_snowman’s picture

Status: Needs work » Needs review
FileSize
70.35 KB

Status: Needs review » Needs work

The last submitted patch, 91: 2011038-81.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 93: use_context_system_for-2011038-93.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Issue tags: +Needs reroll
vacho’s picture

patch for comment #93 rerolled

vacho’s picture

Issue tags: -Needs reroll +Needs tests

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

andypost’s picture

Version: 8.9.x-dev » 9.1.x-dev
aleevas’s picture

Tried to fix failed test

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

jibran’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

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.

ravi.shankar’s picture

ravi.shankar’s picture

Added reroll of patch of #106.

Mistakenly added blank comment #109.

TR’s picture

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

Version: 9.5.x-dev » 10.1.x-dev
Parent issue: » #3252386: Use PHP attributes instead of doctrine annotations

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

quietone’s picture

Status: Needs work » Postponed

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.

dpi’s picture

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

quietone’s picture

Status: Postponed » Needs work

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.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Component: action.module » base system

Moving Non UI issue to the base system