Problem/Motivation

6 node action plugins have nearly identical implementations of both execute and access methods. Those plugins are:

  • PublishNode
  • UnpublishNode
  • StickyNode
  • UnstickyNode
  • PromoteNode
  • DemoteNode

all in Drupal\node\Plugin\Action namespace. The only difference is the name of the field (ie. status, sticky or promoted) and the value to set (NODE_PUBLISHED, ...). There are two problems with this approach:

  1. Each action plugin is reimplementing the same logic.
  2. If you end up needing to write a similar action for your own entity, you need to copy that same structure making the amount of code to maintain even bigger.

Proposed resolution

Long story short - all the actions mentioned above do the same thing. They update a field on an entity, so let's turn this (times 6)


class PublishNode extends <strong>ActionBase</strong> {

  /**
   * {@inheritdoc}
   */
  public function execute($entity = NULL) {
    $entity->status = NODE_PUBLISHED;
    $entity->save();
  }

  /**
   * {@inheritdoc}
   */
  public function access($object, AccountInterface $account = NULL, $return_as_object = FALSE) {
    /** @var \Drupal\node\NodeInterface $object */
    $result = $object->access('update', $account, TRUE)
      ->andIf($object->status->access('edit', $account, TRUE));

    return $return_as_object ? $result : $result->isAllowed();
  }

}

into this


class PublishNode extends <strong>FieldUpdateActionBase</strong> {

  /**
   * {@inheritdoc}
   */
  protected function getFieldsToUpdate() {
    return ['status' => NODE_PUBLISHED];
  }

}

and move all common code to base class.

Results:

  • Less code to maintain.
  • Bonus: better DX. Implementing a new action (eg. PublishMyCustomEntity) becomes super easy. No need to worry about field or entity access, as base class covers that.
  • Bonus: Defining complex actions, such that update more than one field is a breeze: 

class FinishTask extends <strong>FieldUpdateActionBase</strong> {

  /**
   * {@inheritdoc}
   */
  protected function getFieldsToUpdate() {
    return [
      'status' => TASK_FINISHED,
      'progress' => '100%',
      'assignee' => null,
    ];
  }

}

Comments welcome.

Remaining tasks

Decide where to put the base class. Drupal\Core\Field? Drupal\Core\Action?

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#16 interdiff.txt1.34 KBblazey
#15 action_base_class-2805993-15.patch9.11 KBblazey
#12 action_base_class-2805993-12.patch9.26 KBtrzcinski.t@gmail.com
#10 interdiff.txt1.4 KBblazey
#10 action_base_class-2805993-10.patch9.1 KBblazey
#2 action_base_class-2805993-2.patch8.62 KBblazey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

blazey created an issue. See original summary.

blazey’s picture

blazey CreditAttribution: blazey as a volunteer commented
Status: Active » Needs review
FileSize
action_base_class-2805993-2.patch8.62 KB
blazey’s picture

blazey CreditAttribution: blazey as a volunteer commented
Issue summary: View changes
blazey’s picture

blazey CreditAttribution: blazey as a volunteer commented
Issue tags: +Dublin2016
blazey’s picture

blazey CreditAttribution: blazey as a volunteer commented
Assigned: blazey » Unassigned
Issue tags: +Needs manual testing, +Novice

To test:

  • Create a node
  • Go to /admin/content
  • For each action:
  1. Check the checkbox next to the node
  2. Execute the action (e.g Publish, Unpublish, etc)
  3. Visit node edit page and check if action has been applied.
Lund’s picture

Lund CreditAttribution: Lund at Adapt commented
Status: Needs review » Reviewed & tested by the community

I tested the patch provided following the steps given, and it works as intended. Also tested that other users without the right permissions cant change the state of the node.

Virbius’s picture

Virbius CreditAttribution: Virbius as a volunteer and commented

Done further testing of dropdown options, including selecting Sticky when already Sticky, and the settings were reflected when in Edit mode. Did this for Publish/Unpublish, Sticky/Unsticky, and finally Delete. Did this with a second node present but unselected, and all OK.

Also did the same with 2 node in parallel, while not selecting a 3rd.

This was done at DrupalCon Dublin 2016

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record
  1. This looks like a good idea - definitely makes adding new actions that respect access easier. We need a change record to tell people about the new class.
  2. +++ b/core/lib/Drupal/Core/Field/FieldUpdateActionBase.php
@@ -0,0 +1,48 @@
+abstract class FieldUpdateActionBase extends ActionBase {

    Needs a class doc to describe what it is for and how to implement. 

  3. +++ b/core/lib/Drupal/Core/Field/FieldUpdateActionBase.php
@@ -0,0 +1,48 @@
+    /**
+     * @var string $field
+     * @var mixed $value
+     */

    This looks a bit out of place

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented 
  1. +++ b/core/lib/Drupal/Core/Field/FieldUpdateActionBase.php
@@ -0,0 +1,48 @@
+   * Map of fields and values to set.

    Should start with something like "Gets..." 

  2. +++ b/core/lib/Drupal/Core/Field/FieldUpdateActionBase.php
@@ -0,0 +1,48 @@
+   * @return array
+   */

    Needs a description - should be similar to the one line.

blazey’s picture

blazey CreditAttribution: blazey as a volunteer commented
Status: Needs work » Needs review
Issue tags: -Needs manual testing, -Novice, -Needs change record
FileSize
action_base_class-2805993-10.patch9.1 KB
interdiff.txt1.4 KB

Thank you Alex. Attached patch is addressing all the points from #8 and #9. First draft of change record: https://www.drupal.org/node/2817655. Does it sound acceptable in English?

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.

trzcinski.t@gmail.com’s picture

trzcinski.t@gmail.com CreditAttribution: trzcinski.t@gmail.com as a volunteer commented
FileSize
action_base_class-2805993-12.patch9.26 KB

The patch was not applying to 8.3.3 and 8.4.x. Attaching a reroll with replaced the use of deprecated constants.

trzcinski.t@gmail.com’s picture

trzcinski.t@gmail.com CreditAttribution: trzcinski.t@gmail.com as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Since the tests passed I am marking this as RTBC (I have tested the thing before, now just re-rolled :) ).

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +DX (Developer Experience)
  1. The code changes look great! :)
  2. +++ b/core/lib/Drupal/Core/Field/FieldUpdateActionBase.php
@@ -0,0 +1,65 @@
+ * Provides a base for action plugins that update one or more fields on an
+ * entity. Extending class is responsible of providing an array of values with
+ * field names as keys.

    Must be a single line. 

  3. +++ b/core/lib/Drupal/Core/Field/FieldUpdateActionBase.php
@@ -0,0 +1,65 @@
+ * Example implementation:
+ *
+ * class PromoteAndMakeSticky extends FieldUpdateActionBase {
+ *
+ *   protected function getFieldsToUpdate() {
+ *     return [
+ *       'status' => NODE_PROMOTED,
+ *       'sticky' => NODE_STICKY,
+ *     ];
+ *   }
+ *
+ * }

    This is missing the right formatting 

  4. +++ b/core/lib/Drupal/Core/Field/FieldUpdateActionBase.php
@@ -0,0 +1,65 @@
+    /**
+     * @var \Drupal\Core\Entity\EntityInterface $object
+     * @var \Drupal\Core\Access\AccessResultInterface $result
+     */

    This is the wrong notation.

blazey’s picture

blazey CreditAttribution: blazey as a volunteer commented
Status: Needs work » Needs review
FileSize
action_base_class-2805993-15.patch9.11 KB

Thanks for review. How about now?

blazey’s picture

blazey CreditAttribution: blazey as a volunteer commented
FileSize
interdiff.txt1.34 KB

Attaching an interdiff.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

Thanks!

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented 
+++ b/core/modules/node/src/Plugin/Action/StickyNode.php
@@ -14,23 +14,14 @@
 
...

nit: two empty lines here instead of one, can be fixed on commit

  • catch committed 18cffdc on 8.4.x 
    Issue #2805993 by blazey, tom_ek, alexpott, Wim Leers, Lund, Virbius:...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 18cffdc and pushed to 8.4.x. Thanks!

Status: Fixed » Closed (fixed)

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