Goal

Replace the current actions API by a better one, which provides reasonable UX and fulfills the needs for Rules. Having two incompatible action APIs is bad for Drupal and its ecosystem - so this needs a fix.

Next, blocks&layouts needs a conditions API: #1743686: Condition Plugin System - we want to have conditions and actions with a similar DX and UX. Also, we want a conditions API that Rules can build upon to avoid having two incompatible APIs again.

Proposed resolution

1. Add a new actions API (done!)
2. Add a conditions API that works the same way (done!)
3. Convert core to leverage the new API (remove old action usages and that node operation stuff) (done!)
4. Remove action.module
4. Rename the action.module to action_ui.module

What's wrong with actions.module?

  • The current way of configuring actions is very unintuitive. It does not make sense for users to switch to another interface for configuring an action, then go back to whereever to use it. Instead, the primary action configuration should happen on the fly in a multi-step form or so. (Yes, there is a use-case for having pre-configured actions, but that's far not the most common one.)
  • Actions need multiple parameters as input. Having just one which is identified solely by an "action type" does not suffice, nor does a magic but undefined $context array for further input help. I need to be able to know when I can execute an action, for which I need to have a complete description of the required inputs.
  • Then, another thing the current action system does is tracking the number of recursive calls and caring about recursion. I don't think that's something the action system should take care of. It should provide me with an action that I can execute - whether this creates a recursive loop is up to the calling code; usually (think VBO) it won't. Recursions may occur in the regular trigger/rules use-case, but then it's caused by the triggering logic so the check should be there.

So in the end, I see nothing valuable in the existing actions API left. Let's better re-design it from scratch with the tools we already have: Plugins + TypedData definitions for describing inputs (=action parameters).

The new API

Focus

Imo, the focus for core should be on providing an action API that can be configured on the fly - i.e. I select the action, optionally get a configuration form, press submit - it's executed. There is no storage or separate screen for pre-configuration actions required.
Based upon that basic API an UI + storage can be added to build a screen for pre-configuring actions. But I do not even think we need a screen for "pre-configured actions in core?

Defined parameters

We already haved typed data definitions in core, which allow use to describe the needed data similar to what Rules does already in Drupal 7. By doing, so we exactly know what the required action parameters are, so we can determine action plugins that are executable given a certain input (e.g. a node).
Moreover, to fulfill the needs of Rules we need to have that kind of data definition for any input - i.e. any string input needs to be defined as parameter. Thus, action parameter configuration is totally decoupled from forms, forms are just the UI on top of the API. That makes it easy to re-use an action from code also.

Relationship to Conditions

Code-wise a condition is very similar to an action, so we can share most of the code. The difference between actions and conditions really is that conditions impose more restrictions upon the actual implementation: It must return a boolean, and it may not alter the input variables (conditions are not expected to issue changes). As conditions impose restrictions upon actions, we can extend conditions from actions, i.e. have ConditionInterface extends ActionInterface. Likewise we can probably extend some of the required classes or use them directly, while for developers the condition API appears to be a separate one that just works the same way as actions. E.g.
-> have \Drupal\Core\Action\ActionManager
-> have \Drupal\Core\Condition\ConditionManager

So the condition core component would have a dependency on the action component.

Using the new API

As said, actions would be based upon the plugin system, so they are instantiated via a respective plugin manager service. For configuring action methods should be used, e.g. like this:

  drupal_container()->get('action.manager')
    ->createInstance('node_set_title')
    ->setArgument('node', $node)
    ->setArgument('text', 'new title')
    ->execute();

More things required by Rules

Unfortunately there is even more we need to have to make the API usable by Rules also. It mostly boils down to features needed for configuring parameters: The ability to select the argument from an available data input (e.g. to configure 'node'), optionally via a data selector ('node:referenced-node') and the ability to have DataProcessors which process the argument value before it is passed on to the action. The core use-case here would be the Tokens, i.e. have tokens replaced before the inputted text is passed on to the action. Rules can add more data processor plugins then.
To keep things reasonable simple in core I think we should make sure that the API for this is in place, but only provide a simple and focussed UI for it in core; e.g. we do not need to have Rules' data selector (for selection node:author:referenced-node:title) in core, a simple select for choosing the "Node" to go with suffices. Rules, then should be able to override this for its own usage by a more sophisticated UI - this should be rather easy by using dependency injection, e.g. we just need to allow Rules to inject it's own ParameterFormManager later on.

User interface changes

The action pre-configuration screen would go away. The remaining 1% use-case of pre-configuring actions instead of configuring them on the fly could be handled by Rules.
Relatively unchanged.

API changes

The actions API would completely change.

Code

In patch.

Followups

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Issue tags: +actions, +rules, +Blocks-Layouts

adding tags

sun’s picture

fago’s picture

Here is some API brainstorming I did together with EclipseGC at BADcamp: http://pastebin.com/WafrnPrF

@entity-operations:
I discussed that with fubhy today and the relationship to actions came to my mind also. I think it's different though, as entiy operations is more about the available operations you can list or link to when listing an entity. So maybe some actions could be exposed as entity operations but its main purpose is quite different I think.

rszrama’s picture

Would love to see this happen and am happy to pitch in on the sandbox once it's up.

sun’s picture

Can you clarify where exactly you see the difference between an action and an entity operation?

I think that would be a tremendous help for everyone. Thanks in advance.

fago’s picture

Unlike actions, entity operations come with a menu callback that is ready to use for linking to it. Then I think for entity operations it would be best to require them to return a renderable array only (or just have it at the callback), so e.g. view or different view-modes can be an operation also. Example: A commerce order might have "view order" and "view invoice" operations. However, there is no "action" functionality involved there.
Then something like the "edit" operation also differs as it would get the regular entity-form for an entity operation, but an entity update action would have a different looking form that e.g. allows you to populate entity references like node author by referring to existing context variables (e.g. the current user).

sun’s picture

mmm, yeah, I rather thought of entity operations à la "delete", but also "clone", "enable", "disable", "publish", "unpublish", "make sticky", "promote to frontpage" though... some entity operations may be configurable, and so are actions... in the end, I see quite some overlap ;)

But OK, let's move forward here independently for now, and only later on double-check how much these two things are duplicating each other. :)

Grayside’s picture

Forcing something to use context does not seem counter to each action providing a form. Sounds like operations are a particular route to a thing, where that thing might be an action with constrained or predetermined values.

fubhy’s picture

We discussed the overlap between actions and operations today on IRC and came to the conclusion that actions CAN be exposed as operations. We would do that through derivative discovery by picking up all actions that want to be picked up as operations.

EclipseGc’s picture

Thought I'd post my own code example of where this might generally go here: https://gist.github.com/4067136

damiankloip’s picture

Assigned: Unassigned » damiankloip

Ok, I've been speaking to EclipseGc and fubhy; I'm going to work on creating an initial patch for this.

fago’s picture

Great!

Adding another thought - I think we should leverage #1845546: Implement validation for the TypedData API for validating parameter configuration once its in. With that we can just put 'constraints' into the parameter-description and leverage it to easily validate parameter values independently from forms. So when you manually execute an action, we can fire up the same validation as during action configuration via forms.

damiankloip’s picture

Assigned: damiankloip » Unassigned

I've created a sandbox for this now: http://drupal.org/sandbox/damiankloip/1853786

It's currently just a rough start, but you have to start somewhere!

xjm’s picture

Issue tags: +VDC
andypost’s picture

Any progress on this?

joachim’s picture

While I initially didn't grok at all the crossover between operations and actions, I've been actively developing this D7 module: http://drupal.org/project/entity_operations, and sure enough, I've found that there is huge benefit from making some operations be actions: they get exposed to VBO, Services, and more.

It would be great to get people with more D8 experience than I have to look at it.

fago’s picture

yes - we started working on #1743686: Condition Plugin System. It comes with a parent executable core component which actions and conditions can re-use. Once we have conditions done, we can simply use the same code as a base for actions. We'd have to allow actions to provide variables/context and obviously migrate the current uses.

podarok’s picture

Priority: Normal » Major

#1743686-74: Condition Plugin System already in core
making this major

zhangtaihao’s picture

Even though it's not strictly for this discussion to fully resolve, IMO the definition of automation events has always been a thorny issue. Now that Trigger module has been removed from core, there is nothing in particular remotely governing the nature of events triggering actions. Given #1509164: Use Symfony EventDispatcher for hook system is postponed to D9, we need to officially answer the question as to whose responsibility it will be to "standardize" automation events.

Should I create this as a separate task, or does someone here already have the answer?

Gábor Hojtsy’s picture

I'm afraid we might be falling between two stools. Actions are not plugins (#1788104: Convert actions to plugin sub-system was stopped being worked on I guess in favor of this), while this 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).

zhangtaihao’s picture

I suppose I might take that to mean automation events are out (from core).

On the other hand, Rules in D7 already stores configurations as entities and sports an i18n extension for translating parameter settings (e.g. message for dsm). Apart from the inevitable hard work involved, I don't see why Rules for D8 can't do the same.

If I understood fago right, this issue is similar to the condition plugin system, i.e. to standardize an API for defining/using actions. From the D6 days, Rules implemented its own action system (presumably) to handle multiple typed parameters, among other features. That Rules became more popular than core Trigger for automation meant other modules (e.g. VBO) had to support Rules executables in their own (sometimes convoluted) ways. Defining this API would go a long way towards making maintainers for a number of modules agree on the structure and dynamics of an action.

The fact that we've landed on plugins is no accident (since that's what it was there for). The multilingual aspect (apart from @Translation annotations) will be up to Rules. The removal of Trigger simplifies this consideration.

fago’s picture

I suppose I might take that to mean automation events are out (from core).

Yes they are. I see no problem with that, Rules will introduce an "event" plugin system as in D7.

@Gabor: I answered it over at #1788104-14: Convert actions to plugin sub-system.

Gábor Hojtsy’s picture

Erm, acton module *is in core*. Action configuration with human facing text as admin input is in core. It is not out of core. it is in.

zhangtaihao’s picture

Automation events triggering those actions (with conditions) will be defined in Rules.

As for the core Actions module and the text configuration, I presume a ConfigEntityInterface-based actions API refactoring is in order (for entity-related needs, e.g. translation) in a separate issue. I'm not sure it's realistic to remove it at any point, considering Views is in core and the core Actions module does provide some useful extensions for bulk operations (unless the community supports an upgrade path directly from core Actions to Rules, which will then need #D8CX).

This issue's main intent is to unify the interface underlying the advanced actions managed in the Actions admin UI and other possible actions, e.g. managed in Rules. However, if Actions is indeed to become just an API, then most of the remaining Actions code can be split between System and Views. If not, then Actions will provide an overview of available actions provided by all, and then a UI for managing custom, configurable "actions" (e.g. dsm).

In the latter case, a detailed investigation will be required regarding how to treat an action as core-customizable. Should the case be "all actions can be customized", then entity contexts will have to be considered (e.g. for node actions). With typed data in core, it should at least theoretically be less impossible to define the criteria under which an action can be customized in the core UI.

andypost’s picture

So actions should be plugins a-la conditions now?

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

I need to sync up with @damiankloip about this, but I've thought about this more from the "please kill hook_user_operations and hook_node_operations" side of things.

In the meantime, assigning to myself.

tim.plunkett’s picture

I will have an update tomorrow. Closed #1788104: Convert actions to plugin sub-system in the meantime.

tim.plunkett’s picture

Status: Active » Needs work
FileSize
170.54 KB

This depends on #1851086: Replace admin/people with a View and #1895160: Convert admin/content to a View, keep a non-views fallback with no bulk operations.
It will not apply until they are committed.

It introduces a new plugin type called Operation.
This replaces hook_user_operation() and hook_node_operation(), as well hook_action_info().

However, since many actions actually need to be configured, this patch also introduces a new ConfigEntity, Action

An Action has a 1:1 relationship with an Operation. It actually directly mirrors the new Block Entity/Plugin architecture, and works quite well here.

This has a lot of @todos and other cleanups that can take place in the next couple days while those views conversions are finalized.

Purposefully leaving needs work for now.

tim.plunkett’s picture

tim.plunkett’s picture

As I summarized before:

An Action is an instance of an Operation.

An Operation is a potential candidate for an Action.

tim.plunkett’s picture

Ideally this would use \Drupal\Core\Executable\ExecutableInterface.

However, ExecutableInterface is completely wrapped up in Conditions and is unusable on its own. See #1920822: Decouple ExecutableInterface from Conditions and FormInterface.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
258.33 KB

Okay, got it passing. Here it is with both conversions included.

The upgrade path is blocked on #1998204: config_install_default_config() is not safe to use in hook_update_N().

aspilicious’s picture

Hmm yeah we need action plugins. Certainly after reading this patch :)

+++ b/core/lib/Drupal/Core/Annotation/Operation.phpundefined
@@ -0,0 +1,69 @@
+ * @return
+ *   An array of operations. Each operation is an associative array that may
+ *   contain the following key-value pairs:
+ *   - label: (required) The label for the operation, displayed in the dropdown
+ *     menu.

It's strange and a bit confusing to add @return docs to a class..

+++ b/core/lib/Drupal/Core/Operation/OperationBase.phpundefined
@@ -0,0 +1,58 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function execute(array $entities) {
+    foreach ($entities as $entity) {
+      $this->executeSingle($entity);
+    }
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function executeSingle($object) {
+    $this->execute(array($object));
+  }

This looks kinda strange for a base implementation... Why are you calling $this->execute() inside executeSingle. Doesn't that trigger an infinite loop?

+++ b/core/lib/Drupal/Core/Operation/OperationManager.phpundefined
@@ -0,0 +1,52 @@
+    $this->discovery = new AnnotatedClassDiscovery('Operation', $namespaces, array(), 'Drupal\Core\Annotation\Operation');
+    $this->discovery = new AlterDecorator($this->discovery, 'operation_info');

Hmm don't we need to cache this? (and wasn't there a patch somewhere that enabled caching by default?)

+++ b/core/modules/action/action.moduleundefined
@@ -130,568 +113,9 @@ function actions_do($action_ids, $object = NULL, $context = NULL, $a1 = NULL, $a
+  $actions = entity_load_multiple('action', (array) $action_ids);
+  foreach ($actions as $action) {
+    $action->execute(array($object));

Why not use executeSingle here?

+++ b/core/modules/action/lib/Drupal/action/ActionListController.phpundefined
@@ -0,0 +1,98 @@
+    $build['action_table'] = parent::render();
+    if (!$this->hasConfigurableActions) {
+      unset($build['action_table']['#header']['operations']);
+    }
+    $build['action_admin_manage_form'] = drupal_get_form(new ActionAdminManageForm(\Drupal::service('plugin.manager.operation')));
+    return $build;

Maybe a newbisch question. But this is part of a entityListingForm thingie. Why does it first build a table by callen parent::render and adds another table on the bottom? EDIT never mind, adding an action is on the same page, if I understand the flow.

+++ b/core/modules/action/lib/Drupal/action/Plugin/Operation/GotoAction.phpundefined
@@ -0,0 +1,69 @@
+  public function validate(array &$form, array &$form_state) {

If we have a ConfigurationOperationBase we don't need to insert an empty validate function every single time. Not sure what core policy says about this...

+++ b/core/modules/action/lib/Drupal/action/Plugin/Operation/MessageAction.phpundefined
@@ -0,0 +1,97 @@
+  public function validate(array &$form, array &$form_state) {

See :D

+++ b/core/modules/node/lib/Drupal/node/NodeBCDecorator.phpundefined
@@ -0,0 +1,16 @@
+class NodeBCDecorator extends EntityBCDecorator implements NodeInterface {

What do we need this here?

+++ b/core/modules/node/lib/Drupal/node/Plugin/Core/Entity/Node.phpundefined
@@ -233,4 +234,15 @@ public function getRevisionId() {
+  /**
+   * {@inheritdoc}
+   */
+  public function getBCEntity() {
+    if (!isset($this->bcEntity)) {
+      $this->getPropertyDefinitions();
+      $this->bcEntity = new NodeBCDecorator($this, $this->fieldDefinitions);
+    }
+    return $this->bcEntity;

Same question...

+++ b/core/modules/node/lib/Drupal/node/Plugin/Operation/DemoteNode.phpundefined
@@ -0,0 +1,32 @@
+    node_mass_update($entities, array('promote' => NODE_NOT_PROMOTED));

Out of scope for this issue and probably D9, but shouldn't we put this inside some controller holding some utility functions?

+++ b/core/modules/node/lib/Drupal/node/Plugin/Operation/UnpublishByKeywordNode.phpundefined
@@ -0,0 +1,75 @@
+        $node->save();
+        break;

a break inside a foreach, is that how we should handle these kinda loops? (just a quesstion :) )

+++ b/core/modules/node/lib/Drupal/node/Plugin/Operation/UnstickyNode.phpundefined
@@ -0,0 +1,32 @@
+class UnstickyNode extends OperationBase {

Hmm now I know why you're doing that strange loop thingie in OperationBase... Else you need to implement executeSingle here to call execute. But in the end I still think creating classes with infinite loops in it isn't a good idea...

tim.plunkett’s picture

Pretending each dreditor docblock is numbered:

1) Yep, copy/paste. Will fix.
2) Yes. This is a bit strange. We'll see if I can find an elegant way to solve this.
3) It indeeds needs caching. See #1903346: Establish a new DefaultPluginManager to encapsulate best practices for the default caching.
4) Uhhhhh idk I will :)
5) :)
6) yep, I already considered adding a ConfigurationOperationBase, probably still will
8) That's part of the admin/content patch
9) See 8
10) resolved in the sandbox
11) Already in HEAD
12) See 2

tim.plunkett’s picture

Ideally those two views issues will go in first. But in case they continue to be stalled, here is the above patch decoupled from them.

This addresses 4 and 6 from #34.

dawehner’s picture

Just some small comments.

+++ b/core/modules/action/lib/Drupal/action/ActionFormControllerBase.phpundefined
@@ -0,0 +1,126 @@
+class ActionFormControllerBase extends EntityFormController {

Feels like a good usecase for abstract.

+++ b/core/modules/action/lib/Drupal/action/Form/ActionAdminManageForm.phpundefined
@@ -22,12 +47,16 @@ public function getFormID() {
+    foreach ($this->manager->getDefinitions() as $id => $definition) {
+      if (is_subclass_of($definition['class'], '\Drupal\Core\Operation\ConfigurableOperationInterface')) {
+        $key = Crypt::hashBase64($id);
+        $actions[$key] = $definition['label'] . '...';
+      }
+    }

Why do we need all this crazyness ... Maybe a comment would help.

+++ b/core/modules/action/lib/Drupal/action/Plugin/Operation/GotoAction.phpundefined
@@ -0,0 +1,69 @@
+class GotoAction extends OperationBase implements ConfigurableOperationInterface {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function executeSingle($object) {
+    drupal_goto($this->configuration['url']);

Urgs, this will be really hard to rip out later for RedirectResponse ...

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleContentTest.phpundefined
@@ -209,6 +209,7 @@ function testContentTypeDirLang() {
+    module_enable(array('views'));

It seems to be that you not 100% truncated the patch but yeah, that's not a big deal at all.

+++ b/core/modules/node/lib/Drupal/node/Plugin/Operation/AssignOwnerNode.phpundefined
@@ -0,0 +1,100 @@
+    $count = db_query("SELECT COUNT(*) FROM {users}")->fetchField();
...
+    if (is_numeric($this->configuration['owner_uid'])) {
+      $owner_name = db_query("SELECT name FROM {users} WHERE uid = :uid", array(':uid' => $this->configuration['owner_uid']))->fetchField();
...
+      $result = db_query("SELECT uid, name FROM {users} WHERE uid > 0 ORDER BY name");

Just wondering whether we want to use the ContainerFactory and inject stuff in there.

+++ b/core/modules/node/lib/Drupal/node/Plugin/Operation/UnpublishByKeywordNode.phpundefined
@@ -0,0 +1,75 @@
+class UnpublishByKeywordNode extends OperationBase implements ConfigurableOperationInterface {
...
+      $elements = node_view(clone $node);

UnpublishByKeyword(Node|Comment) could be abstracted to an action based on entity + entity_view (maybe this is something for a follow up).

tim.plunkett’s picture

FileSize
187.92 KB

I had a long talk with fago, who disliked the name Operation. So that is all Action now.

I need to respond to @dawehner still.

tim.plunkett’s picture

FileSize
187.92 KB

missing semicolons

tim.plunkett’s picture

FileSize
187.9 KB

Silly mistakes. This should pass. I would appreciate help with docs.

bojanz’s picture

tim.plunkett, you are my hero. That is all.

I did a brief glance at the patch.
1) Why are we doing:

+    if (empty($this->configuration['node'])) {
+      $this->configuration['node'] = $entity;
+    }

Which part of code is still assuming nodes here?

2) Am I right to assume that this API no longer has global actions (previously type = 'system', in VBO type = 'entity')? Declaring an action available to all entity types has had a bit of a bumpy UX, so I'd love to see if we could use derivatives to provide one for each entity type.

For instance, we need a "delete entity" action that would work on any entity type, so that we don't need to duplicate code for every entity type we define (it is an action that is always needed).
Of course, this can be a followup.

3) I see that we are defining a confirmation page for deletions only.
VBO tried to tackle this generically, by introducing a setting for requiring confirmation per action.
We might not want to do that in D8, but there's still an open question of which action deserves a confirmation page, which action doesn't,
and whether we can remove the requirement to hand code it each time.

4) Am I right to assume that the API would still allow for actions that are configured on the fly? (like the issue summary claims)
An example would be sending an email, where after selecting the entities (users, for instance), an email is inputted and then sent off
to each one. We don't want to save a configured action entity, because the configuration of the action is discarded right after.
(So, we'd stick the action entity in a tempstore?)

5)

+class SaveComment extends ActionBase {
+  public function execute($comment) {
+    $comment->save();
+    Cache::invalidateTags(array('content' => TRUE));
+  }
+}

Sounds like we want a postExecute() method that would run after all of the passed entities have been processed (for invalidating tags or doing any other cleanup).

tim.plunkett’s picture

Thanks @bojanz, I appreciate it.

I think all 5 of those points are excellent, and still very possible, but very much fodder for follow-ups.

1) That's taken directly from http://api.drupal.org/api/drupal/core%21modules%21action%21action.module...
2) Yes, I'm doing a 1:1 port right now, just to cut down on complexity. I absolutely want to use derivatives to unify stuff like PublishComment and PublishNode. And we still have three system actions: action_message_action, action_goto_action, and action_send_email_action (now MessageAction, GotoAction, and EmailAction).
3) I've already considered adding a form for all actions, but that would be a new UI and interaction pattern, and would need a bunch of new tests. Follow-up!
4) Yes, that should work just fine. entity_create() without a save.
5) +1, follow-up.

fago’s picture

For instance, we need a "delete entity" action that would work on any entity type, so that we don't need to duplicate code for every entity type we define (it is an action that is always needed).

Yeah, I think we should do derivates for all the important entity-generic actions. Not having them is bad for UX (what we are seeing with Rules), so I really think we should have that. We can have a generic action working with any entity as well.

4) Yes, that should work just fine. entity_create() without a save.

Couldn't I just use the plugin, instanstiate it, configure it and execute it also? I think we should be able to that. Having to create an entity if you do not want to store anything feels wrong to me.

tim.plunkett’s picture

Ah! Yeah you could absolutely use the plugin directly.

So, I opened #2001190: Use derivatives for action plugins and #2001196: Add a postExecute() step to action plugins.

What are the next steps before RTBC?

fago’s picture

Great work! I did a first quick review. I've not been able to through everything yet, but it should be a start:

+++ b/core/includes/bootstrap.inc
@@ -3035,19 +3035,7 @@ function drupal_classloader_register($name, $path) {
+ * @todo We need a new example.

yes

+++ b/core/lib/Drupal/Core/Action/ActionBase.php
@@ -0,0 +1,59 @@
+  public function executeMultiple(array $entities) {
+    foreach ($entities as $entity) {
+      $this->execute($entity);

this and execute pointing back looks like a loop?

+++ b/core/lib/Drupal/Core/Action/ActionInterface.php
@@ -0,0 +1,44 @@
+ * @todo Extend \Drupal\Core\Executable\ExecutableInterface after

It might not be ideal yet, but still why not immediately extend from it and fix what needs to be fixed in follow-ups? Not using it is just confusing and seems wrong.

+++ b/core/lib/Drupal/Core/Action/ActionInterface.php
@@ -0,0 +1,44 @@
+ * @see \Drupal\Core\Annotation\Operation

Left-over?

+++ b/core/lib/Drupal/Core/Action/ActionInterface.php
@@ -0,0 +1,44 @@
+  public function execute($object);

$object should be a context? If I have an action that works with an entity, it should be in the context.

Not totally sure how that would work with multiple executing though. Maybe we could just define the multiple stuff to be required that way in the context?

+++ b/core/lib/Drupal/Core/Action/ConfigurableActionInterface.php
@@ -0,0 +1,53 @@
+interface ConfigurableActionInterface extends ActionInterface {

+1 to separating that but that's an issue for Conditions as well. Let's treat it as one and just file it as follow-up to split out form interface?

+++ b/core/lib/Drupal/Core/Annotation/Action.php
@@ -0,0 +1,56 @@
+   * A URL to redirect to after processing the action.
+   *
+   * @var string (optional)
+   */

That seems wrong to me. The code making use of the action should provide the default redirect, like going back to the node admin screen, not the action imho. Is there a use-case for which we need that?

+++ b/core/lib/Drupal/Core/Annotation/Action.php
@@ -0,0 +1,56 @@
+   * @todo Replace with \Drupal\Core\Plugin\Context\Context.

Yes, imho that's critical and should be done for the initial patch.

fago’s picture

Status: Needs review » Needs work
    $this->factory = new DefaultFactory($this->discovery);
+    $this->factory = new ContainerFactory($this);

Seems to be unrelated?

+ * Implements hook_user_role_insert().
+ */
+function user_user_role_insert(RoleInterface $role) {
+  // Ignore the authenticated and anonymous roles.
+  if (in_array($role->id(), array(DRUPAL_AUTHENTICATED_RID, DRUPAL_ANONYMOUS_RI
+    retur

ouch. Roles should be a configuration parameter, so there is no reason for auto-generating an action per role. I doubt auto-generating actions is a good pattern. Instead, one should select the action "Remove user role" (or add) and configure it on-the-fly. That clutters the UI less and is more user-friendly as well I think. (You could even select multiple roles at once..) If that's complicated to do right now, it's something we might want to do in a follow-up? Or if we really want to keep the separate entries, it should implement a derivative but not auto-generate configs.

tim.plunkett’s picture

I got to talk through #47 with fago, and I'll address it shortly. In addition, I'm including #1920822: Decouple ExecutableInterface from Conditions and FormInterface with the hopes it will be a quick commit.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
28.76 KB
187.84 KB

Okay, this:
moves to ExecutableInterface
removes the loop in ActionBase
renames from Operation to Action in old code
rename 'redirect' to 'confirm_form_path' with @todos
removes old changes to node.admin.inc that were from the views issue

tim.plunkett’s picture

FileSize
4.25 KB
192.41 KB

A couple blockers went in, so this should be pretty much done.

Except the upgrade path...

tim.plunkett’s picture

FileSize
185.41 KB
tim.plunkett’s picture

Okay, here's the upgrade path and test.

dawehner’s picture

1.

+++ b/core/lib/Drupal/Core/Action/ConfigurableActionBase.phpundefined
@@ -0,0 +1,24 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function validate(array &$form, array &$form_state) {
+  }

The idea of having just the validate method is that all configurable actions certainly need a submit/form method but no validate method?

2.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -185,6 +185,7 @@ public function getFormController($entity_type, $operation) {
+        $this->controllers['form'][$operation][$entity_type]->setOperation($operation);

What about adding a @todo to add the operation in the constructor arguments?

3.

+++ b/core/modules/action/lib/Drupal/action/ActionFormControllerBase.phpundefined
@@ -0,0 +1,126 @@
+      '#weight' => -10,

is there any reason for a weight here?

4.

+++ b/core/modules/action/lib/Drupal/action/ActionFormControllerBase.phpundefined
@@ -0,0 +1,126 @@
+   * Determines if the action already exists.
...
+  public function exists($id) {

Missing docs.

5.

+++ b/core/modules/action/lib/Drupal/action/ActionFormControllerBase.phpundefined
@@ -0,0 +1,126 @@
+    return (bool) entity_load('action', $id);

Let's inject the entity manager/storage controller here.

6.

+++ b/core/modules/action/lib/Drupal/action/ActionListController.phpundefined
@@ -0,0 +1,98 @@
+    $row['label'] = $entity->label();

Just to be sure: I can

aler('123');

without any problems.

7.

+++ b/core/modules/action/lib/Drupal/action/ActionListController.phpundefined
@@ -0,0 +1,98 @@
+        'title' => t('configure'),
...
+        'title' => t('delete'),

Don't we use upper-case words now? @see views edit interface.

8.

+++ b/core/modules/action/lib/Drupal/action/ActionListController.phpundefined
@@ -0,0 +1,98 @@
+    $build['action_admin_manage_form'] = drupal_get_form(new ActionAdminManageForm(\Drupal::service('plugin.manager.action')));

Can't we inject the action plugin manager already?

9.

+++ b/core/modules/action/lib/Drupal/action/Form/ActionAdminManageForm.phpundefined
@@ -7,12 +7,37 @@
+  /**
...
+  protected $manager;
...
+  /**
+   * @param \Drupal\Core\Action\ActionManager $manager

Some missing docs.

10.

+++ b/core/modules/action/lib/Drupal/action/Plugin/Action/EmailAction.phpundefined
@@ -0,0 +1,137 @@
+  /**
+   * @var \Drupal\Core\Utility\Token
+   */
+  protected $token;
+
+  /**
+   * Constructs a EmailAction object.
+   */
+  public function __construct(array $configuration, $plugin_id, array $plugin_definition, Token $token) {

Missing docs

11.

+++ b/core/modules/action/lib/Drupal/action/Plugin/Action/EmailAction.phpundefined
@@ -0,0 +1,137 @@
+    $recipient_account = user_load_by_mail($recipient);

Let's inject the user storage controller or just use an EQ?

12.

+++ b/core/modules/action/lib/Drupal/action/Plugin/Action/MessageAction.phpundefined
@@ -0,0 +1,91 @@
+    $message = $this->token->replace(filter_xss_admin($this->configuration['message']), $this->configuration);

Just as note: it should be Xss::filterAdmin now

13.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTestBase.phpundefined
@@ -283,27 +283,27 @@ function commentContactInfoAvailable() {
+      $this->assertRaw(format_plural(1, 'Deleted 1 comment.', 'Deleted @count comments.'), format_string('Action "@action" was performed on comment.', array('@action' => $action)));
...
+      $this->assertText(t('The update has been performed.'), format_string('Action "@action" was performed on comment.', array('@action' => $action)));

format_string can be replaced by String::format

14.

+++ b/core/modules/node/lib/Drupal/node/Plugin/Action/AssignOwnerNode.phpundefined
@@ -0,0 +1,99 @@
+    $count = db_query("SELECT COUNT(*) FROM {users}")->fetchField();
...
+      $owner_name = db_query("SELECT name FROM {users} WHERE uid = :uid", array(':uid' => $this->configuration['owner_uid']))->fetchField();
...
+      $result = db_query("SELECT uid, name FROM {users} WHERE uid > 0 ORDER BY name");
+      foreach ($result as $data) {
...
+    $exists = (bool) db_query_range('SELECT 1 FROM {users} WHERE name = :name', 0, 1, array(':name' => $form_state['values']['owner_name']))->fetchField();
...
+    $this->configuration['owner_uid'] = db_query('SELECT uid from {users} WHERE name = :name', array(':name' => $form_state['values']['owner_name']))->fetchField();

Is it worth to inject the db connection all over the place here?

15.

+++ b/core/modules/node/lib/Drupal/node/Plugin/Action/DeleteNode.phpundefined
@@ -0,0 +1,63 @@
+  /**
+   * @var \Drupal\user\TempStore
+   */
+  protected $tempStore;
+
+  /**
+   * Constructs a DeleteNode object.

Needs some docs

16.

+++ b/core/modules/node/lib/Drupal/node/Plugin/Action/PromoteNode.phpundefined
@@ -0,0 +1,34 @@
+    $entity->status = NODE_PUBLISHED;
+    $entity->promote = NODE_PROMOTED;

it feels odd to publish and promote the node at the same time. At least in rules I would not expect that behavior.

17.

+++ b/core/modules/node/lib/Drupal/node/Plugin/Action/StickyNode.phpundefined
@@ -0,0 +1,36 @@
+ *   type = {
+ *     "node"
+ *   }

Oh type can be singular and plural at the same time?

18.

+++ b/core/modules/system/lib/Drupal/system/Plugin/views/field/BulkFormBase.phpundefined
@@ -17,6 +19,27 @@
   /**
+   * @var array
+   */
+  protected $actions = array();
+
+  /**
+   * Constructs a new BulkForm object.

Needs docs.

19.

+++ b/core/modules/system/system.installundefined
@@ -2194,6 +2194,32 @@ function system_update_8056() {
+  $actions = db_query("SELECT * FROM {actions}")->fetchAllAssoc('aid', PDO::FETCH_ASSOC);

Is it save to assume that the actions table exists all the time? What happens if you have actions module not enabled yet?

20.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Action/BlockUser.phpundefined
@@ -0,0 +1,39 @@
+    if ($account !== FALSE && $account->status == 1) {
...
+      $account->status = 0;

Can we add a follow up for proper constants?

21.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Action/CancelUser.phpundefined
@@ -0,0 +1,63 @@
+  /**
+   * @var \Drupal\user\TempStore
+   */
+  protected $tempStore;
+
+  /**
+   * Constructs a DeleteNode object.
+   */
+  public function __construct(array $configuration, $plugin_id, array $plugin_definition, TempStoreFactory $temp_store_factory) {

Docs

tim.plunkett’s picture

FileSize
194.41 KB
22.86 KB

1. That's what we do for many of the base forms, provide an empty validate method: SystemConfigFormBase, EntityFormController, ConfirmFormBase, field_ui\OverviewBase, Ajax\ViewsFormBase.
2. This is why we introduced setOperation() in the first place, this was just unfortunately left out somehow.
3. It was in the original code. I deleted it anyway.
4. Fixed
5. Fixed
6. Fixed
7. Fixed
8. Fixed
9. Fixed
10. Fixed
11. Switched to storage controller, we need the account loaded.
12. Fixed
13. I left this as is. I was just replacing Operation with Action.
14. Fixed
15. Fixed
16. That's what the original code did. Because if you promote an unpublished node, you don't care, you want that thing PROMOTED. Idk.
17. Nope, that's just wrong. Currently only some of the actions are configurable, and that's only used by configurable actions so far.
18. Fixed
19. The actions table was part of system in D7, it was only moved out to a module in D8.
20. Sure, also it'd be nice to have ->disable() on stuff like this.
21. Fixed.

Holy moly duplicating the __construct() docblocks for plugins is so annoying.

dawehner’s picture

+++ b/core/modules/action/lib/Drupal/action/ActionListController.phpundefined
@@ -22,6 +27,38 @@ class ActionListController extends ConfigEntityListController {
+  /**
+   * @param string $entity_type
...
+  public function __construct($entity_type, EntityStorageControllerInterface $storage, ActionManager $action_manager) {

+++ b/core/modules/node/lib/Drupal/node/Plugin/Action/AssignOwnerNode.phpundefined
@@ -23,6 +25,38 @@
+  /**
...
+  public function __construct(array $configuration, $plugin_id, array $plugin_definition, Connection $connection) {

Let's nitpick that.

tim.plunkett’s picture

FileSize
194.52 KB
1.24 KB

Whoops!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Manually testing worked pretty fine, code looks perfect and test coverage existed before.

fago’s picture

Status: Reviewed & tested by the community » Needs work

This looks already good to me. Some remarks though:

- We'll need a follow-up issue for moving $entity parameters to contexts.

+++ b/core/lib/Drupal/Core/Action/ActionInterface.php
@@ -0,0 +1,36 @@
+ */
+interface ActionInterface extends ExecutableInterface {

This adds a way to get the configuration - good, but:
1) I guess we'd need a way to set the configuration on an object also?
2) We have ConfigurableActionInterface, so getConfiguration() should be in there. There is point in having getConfiguration when it is not configurable. (setting to needs-work for that)

+++ b/core/modules/action/lib/Drupal/action/ActionAddFormController.php
@@ -0,0 +1,75 @@
+      $key = Crypt::hashBase64($id);

This is something really weird. Any chance to do away with it? Could be a follow-up though.

+++ b/core/modules/system/lib/Drupal/system/ActionInterface.php
@@ -0,0 +1,38 @@
+/**
+ * Provides an interface defining a action entity.
+ */
+interface ActionInterface extends ConfigEntityInterface {

Here we run into a weird naming clash, which we had for fields already also. The solution we agreed in discussions here was to use something along the lines of ActionConfigEntityInterface + ActionInterface, so should we do that here as well in order to avoid confusion?

fago’s picture

Issue summary: View changes

update

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
194.84 KB
6.87 KB

Okay, moved getConfiguration() and renamed to ActionConfigEntityInterface.

The Crypt thing is pre-existing. Yes it's weird. No idea why its there.

Opened #2011038: Use Context system for actions

Status: Needs review » Needs work

The last submitted patch, action-1846172-66.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
707 bytes
194.84 KB

/me facepalms

Status: Needs review » Needs work

The last submitted patch, action-1846172-68.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.7 KB
194.27 KB

I think that's the first bug caught by #1961938: Test the interactive installer. chx++

The change here is to not call into the tempstore factory during __construct(), since it's not needed for initialization.

fago’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, improvements look good.

tim.plunkett’s picture

FileSize
194.41 KB

#1911728: Remove hook_init() changed code in action_loop_test.module, which we delete completely, so this doesn't apply.
Straight reroll, no changes.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Some very minor nits...

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTestBase.phpundefined
@@ -283,27 +283,27 @@ function commentContactInfoAvailable() {
   /**
-   * Performs the specified operation on the specified comment.
+   * Performs the specified action on the specified comment.
    *
    * @param object $comment
-   *   Comment to perform operation on.
-   * @param string $operation
-   *   Operation to perform.
+   *   Comment to perform action on.
+   * @param string $action
+   *   Action to perform.
    * @param boolean $aproval
-   *   Operation is found on approval page.
+   *   Action is found on approval page.
    */
-  function performCommentOperation($comment, $operation, $approval = FALSE) {
+  function performCommentOperation($comment, $action, $approval = FALSE) {
     $edit = array();
-    $edit['operation'] = $operation;
+    $edit['operation'] = $action;
     $edit['comments[' . $comment->id() . ']'] = TRUE;
     $this->drupalPost('admin/content/comment' . ($approval ? '/approval' : ''), $edit, t('Update'));
 
-    if ($operation == 'delete') {
+    if ($action == 'delete') {
       $this->drupalPost(NULL, array(), t('Delete comments'));
-      $this->assertRaw(format_plural(1, 'Deleted 1 comment.', 'Deleted @count comments.'), format_string('Operation "@operation" was performed on comment.', array('@operation' => $operation)));
+      $this->assertRaw(format_plural(1, 'Deleted 1 comment.', 'Deleted @count comments.'), format_string('Action "@action" was performed on comment.', array('@action' => $action)));
     }
     else {
-      $this->assertText(t('The update has been performed.'), format_string('Operation "@operation" was performed on comment.', array('@operation' => $operation)));
+      $this->assertText(t('The update has been performed.'), format_string('Action "@action" was performed on comment.', array('@action' => $action)));
     }
   }

Looks unnecessary

+++ b/core/modules/menu_link/menu_link.moduleundefined
@@ -130,7 +130,7 @@ function menu_link_save(MenuLink $menu_link) {
- *   Operation to perform: insert, update, enable, disable, or delete.
+ *   Action to perform: insert, update, enable, disable, or delete.

Looks unnecessary

+++ b/core/modules/system/system.installundefined
@@ -1921,11 +1921,11 @@ function system_update_8045() {
 function system_update_8046() {
   $front_page = config('system.site')->get('page.front');
-  if (!isset($front_page) || $front_page == 'node') {
+  $module_list = drupal_container()->getParameter('container.modules');
+  if (isset($module_list['node']) && (!isset($front_page) || $front_page == 'node')) {
     update_module_enable(array('views'));
 
     // Register views to the container, so views can use it's services.
-    $module_list = drupal_container()->getParameter('container.modules');
     drupal_load('module', 'views');
 
     drupal_container()->get('kernel')->updateModules($module_list, array('views' => 'core/modules/views/views.module'));

This change looks unrelated and I think might conflict with the update module handler change...

+++ b/core/modules/system/tests/modules/action_test/lib/Drupal/action_test/Plugin/Action/SaveEntity.phpundefined
@@ -0,0 +1,32 @@
+ * Provides an operation to save user entities.
...
+ *   label = @Translation("Save a user"),

Shouldn't this just be save entity/ies?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
191.01 KB
215.05 KB

Those first two hunks were just overzealous renames. The third *should* be handled by the update module handler patch. I went with "Save entities" for the fourth.

One patch includes the entirety of the update module handler patch, the other is rerolled on top of it.

Status: Needs review » Needs work
Issue tags: -actions, -rules, -VDC, -Blocks-Layouts

The last submitted patch, action-1846172-75-standalone.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review

#75: action-1846172-75-standalone.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +actions, +rules, +VDC, +Blocks-Layouts

The last submitted patch, action-1846172-75-standalone.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
190.87 KB

Quick reroll after the user patch went in, as I know Tim was keen to try and get this one done and dusted.

Status: Needs review » Needs work

The last submitted patch, 1846172-79.patch, failed testing.

damiankloip’s picture

FileSize
1.83 KB
191.67 KB

This should fix those remaining failures, todo with user being a shiny new BCEntity...

damiankloip’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

FileSize
191.67 KB

Just reuploading to maybe get on a better bot... That one is running very long.
Thanks @damiankloip for sorting out the UserNG stuff

fago’s picture

Status: Needs review » Reviewed & tested by the community

It looks weird it does $account->status->value == 0 instead of just checking $account->status->value, but that's minor and nothing new anyway. Else, UserNG changes look good - back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, action-1846172-83.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

I manually cancelled that test, it was identical to #81 and was only uploaded because of testbot.

alexpott’s picture

Title: Replace the actions API » Change notice: Replace the actions API
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed b0cf1be and pushed to 8.x. Thanks!

clemens.tolboom’s picture

I did not have the actions module enabled so

function system_update_8057() {

failed. This needs #2014623: system_update_8057() failes when no action module enabled.

tim.plunkett’s picture

We don't support HEAD2HEAD. Action module is enabled in system_update_8021().

cosmicdreams’s picture

HOLY CRAP! GREAT JOB EVERYONE!

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

I had four change notice issues assigned to me but YesCT told me I could give away two of them. Sorry this one.

YesCT’s picture

https://drupal.org/contributor-tasks/draft-change-notification
is instructions for drafting a change notice.
Anyone can try the first draft. :)
Just make a comment saying you are going to try, and ask questions if you have any.

bojanz’s picture

Assigned: Unassigned » bojanz

I'll write it.

bojanz’s picture

Title: Change notice: Replace the actions API » Replace the actions API
Priority: Critical » Major
Status: Active » Fixed

Here it is, feel free to extend it: https://drupal.org/node/2020549

YesCT’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

xjm’s picture

Issue tags: -Needs change record

Untagging. Please remove the tag when the change notification task is completed.

xjm’s picture

Issue summary: View changes

added followup