Add generic entity actions

I would propose to create the following generic actions:

• Move DeleteAction from the contrib entity module to core - http://cgit.drupalcode.org/entity/tree/src/Plugin/Action/DeleteAction.ph...
• Create Publish/Unpublish actions for every EntityType that implements EntityPublishedInterface
• Create Save action for every EntityType that implements EntityChangedInterface

Here is a first patch, that implements Publish/Unpublish actions and Save action.

I fixed some tests. No interdiff, because i regenerated the drupal-8.filled.standard.php.gz

So i regenerated all the fixtures for the update tests, that the actions use the new plugin ids.

Ok, hopefully all the tests are fixed now.

This patch is just for better reviewing. It just doesn't contain the updated fixtures.

Yay, I like this a lot!

1. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriver.php
   @@ -0,0 +1,83 @@
   +namespace Drupal\Core\Action\Plugin\Action\Derivative;
   

   Discussed this with @chr.fritsch in person at DrupalCamp Schwerin 2017. I'm not sure whether this (along with the rest of the new classes in the patch) belongs in the Action component or the Entity component. I'm slightly leaning towards Entity, but that component is already heavily bloated, so not really sure.

2. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriver.php
   @@ -0,0 +1,83 @@
   + * Provides a base action for each entity types with specific interfaces.
   

   Wow, that's a pretty sweet idea! Great success.

3. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriver.php
   @@ -0,0 +1,83 @@
   +abstract class EntityActionDeriver extends DeriverBase implements ContainerDeriverInterface {
   

   I'm not 100% sure, but I think our standards require this to be called "EntityActionDeriverBase".

4. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriver.php
   @@ -0,0 +1,83 @@
   +   * Interface which the entity types have to implement.
   

   Per our standards this should be a proper sentence with a verb, i.e. "Returns the interface which..."

5. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriver.php
   @@ -0,0 +1,83 @@
   +   *   Interface name.
   

   "Interface" -> "The interface"

6. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriver.php
   @@ -0,0 +1,83 @@
   +   * The list consists of all content entity types which implements
   +   * the interface which is set by the child classes.
   

   1. I don't think this is technically restricted to content entities, I think that word can be dropped.

   2. "implements" -> "implement"

   3. Instead of mentioning "child classes" explicitly I would word this something like "the interface that this deriver requires." Because this documentation could for example be displayed inline when using this method in a child class and then it might be confusing to mention "child classes" here explicitly.

   4. The sentence wraps too early, it should wrap at 80 characters.

7. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityChangedActionDeriver.php
   @@ -0,0 +1,19 @@
   +class EntityChangedActionDeriver extends EntityActionDeriver {
   
   +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityPublishedActionDeriver.php
   @@ -0,0 +1,19 @@
   +class EntityPublishedActionDeriver extends EntityActionDeriver {
   

   This is extremely beautiful. Congrats and thank you!

8. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/PublishAction.php
   @@ -0,0 +1,39 @@
   +    $result = $object->$key->access('edit', $account, TRUE)
   +      ->andIf($object->access('update', $account, TRUE));
   

   I realize that you just copied this, but I think it would be more self-documenting to switch the two conditions because field-level access always implies entity-level access has been checked.

   Same for the unpublish action.

9. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/PublishAction.php
   similarity index 61%
   rename from core/modules/node/src/Plugin/Action/SaveNode.php
   
   rename from core/modules/node/src/Plugin/Action/SaveNode.php
   rename to core/lib/Drupal/Core/Action/Plugin/Action/SaveAction.php
   

   Nice!!

10. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/SaveAction.php
    @@ -22,15 +22,14 @@ class SaveNode extends ActionBase {
    -    $entity->changed = 0;
    -    $entity->save();
    +    $entity->setChangedTime(0)->save();
    

    Wow, this is incredibly strange. So we set the value to 0 so that ChangedItem will then detect a change and set itself to the request time? I know that's not your fault, but could we just set this to the request time directly?

11. +++ b/core/modules/action/migration_templates/d7_action.yml
    @@ -20,6 +20,12 @@ process:
    +        comment_publish_action: entity_publish_action:comment
    +        comment_unpublish_action: entity_unpublish_action:comment
    +        comment_save_action: entity_save_action:comment
    +        node_publish_action: entity_publish_action:node
    +        node_unpublish_action: entity_unpublish_action:node
    +        node_save_action: entity_save_action:node
    

    Does this need test coverage? I think the answer is yes, but not sure.

12. +++ b/core/modules/comment/config/schema/comment.schema.yml
    @@ -15,11 +15,11 @@ field.widget.settings.comment_default:
    -action.configuration.comment_publish_action:
    +action.configuration.entity_publish_action:comment:
    

    Not sure if this actually works, but could we provide a generic "action.configuration.entity_publish_action:*" configuration schema?

13. +++ b/core/modules/comment/config/schema/comment.schema.yml
    @@ -34,7 +34,7 @@ action.configuration.comment_unpublish_by_keyword_action:
    diff --git a/core/modules/comment/src/Plugin/Action/PublishComment.php b/core/modules/comment/src/Plugin/Action/PublishComment.php
    
    diff --git a/core/modules/comment/src/Plugin/Action/PublishComment.php b/core/modules/comment/src/Plugin/Action/PublishComment.php
    deleted file mode 100644
    

    Discussed this with @chr.fritsch in person on whether this is BC break. But we couldn't really think of any possible way were a removed plugin would hurt. On the other hand, leaving this around (either as just the class or with the full annotation), so maybe a release manager can make a call here.

14. +++ b/core/modules/content_moderation/content_moderation.module
    @@ -229,11 +229,13 @@ function content_moderation_action_info_alter(&$definitions) {
       // moderated node. If another module has already swapped out those classes,
    

    The comment should no longer mention "node".

15. +++ b/core/modules/content_moderation/content_moderation.module
    @@ -229,11 +229,13 @@ function content_moderation_action_info_alter(&$definitions) {
    -  if (isset($definitions['node_publish_action']['class']) && $definitions['node_publish_action']['class'] == PublishNode::class) {
    -    $definitions['node_publish_action']['class'] = ModerationOptOutPublishNode::class;
    -  }
    

    Great success to be able to remove node-specific code from poor Content Moderation that is really trying hard to be generic. Very nice!

16. +++ /dev/null
    @@ -1,26 +0,0 @@
    -class PublishNode extends FieldUpdateActionBase {
    

    Ahh I forgot about FieldUpdateActionBase. I think we should be able to use that for the new actions, though, which is pretty nice, as we can then get rid of the access() methods altogether.

17. +++ b/core/modules/system/src/Entity/Action.php
    @@ -150,4 +150,17 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
    +      $dependencies->addDependency('module', $entity_type);
    

    Nice catch! So this is a bug fix, do we need dedicated test coverage for that? Should not be too hard to add to PublishActionTest

    Also, the fix is not quite correct, as the module name might not be the entity type ID. You have to load the entity type and fetch its provider.

18. +++ b/core/tests/Drupal/KernelTests/Core/Action/PublishActionTest.php
    @@ -0,0 +1,89 @@
    + * Tests the publish entity action.
    

    ... "publish and unpublish entity actions."

19. +++ b/core/tests/Drupal/KernelTests/Core/Action/PublishActionTest.php
    @@ -0,0 +1,89 @@
    +  public function testPublishUnpublishAction() {
    +
    

    Unneeded newline.

20. +++ b/core/tests/Drupal/KernelTests/Core/Action/PublishActionTest.php
    @@ -0,0 +1,89 @@
    +        'title' => $this->randomString(),
    

    I don't feel that strongly, but the trend is to not use that method, and just do something like "Test node " . $i to avoid random ness (and, thus, random failures) in tests

21. I don't see a test for the save action, would be nice to add that as well.
22. I don't see an update path for the updated action configs. That will need to be added and tested as well.

2-10: Fixed
11: I think it's already covered by MigrateActionsTest
12: Really nice, moved it to core.entity.schema.yml
14/15: Fixed
16: Is it ok to introduce the $entity parameter to getFieldsToUpdate?
17-20: Fixed
21/22: Needs still work

Added update path and SaveAction test

After writing the update path i realized that changing the fixtures is totally bullshit. Sorry for that. So the new patch is without fixtures and with a update path test.

Thank you a lot @chr.fritsch, this looks really nice. I think this is almost ready, I just have one note about the update path test. And since I'm knocking this back once more anyway I read through the patch again and found a couple of nitpicks.

+++ b/core/modules/system/tests/src/Functional/Update/UpdateActionsWithEntityPluginsTest.php
@@ -0,0 +1,45 @@
+      $config = \Drupal::configFactory()->get('system.action.' . $key);
+      $this->assertSame($before, $config->get('plugin'));
...
+      $config = \Drupal::configFactory()->get('system.action.' . $key);
+      $this->assertSame($after, $config->get('plugin'));

I think instead looking at the config directly it would be nice to do more of an "integration test" and actually load the Action config entity and look at $action->getPlugin()->getPluginId(). That way we validate that the entity can still be loaded properly and the plugin can be instantiated.

1. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/SaveAction.php
   @@ -0,0 +1,70 @@
   +    // We need to change at least one value, otherwise the changed timestamp
   +    // will not be updated.
   +    $entity->setChangedTime($this->time->getRequestTime())->save();
   

   I think this comment can now be removed.

2. +++ b/core/modules/system/system.install
   @@ -2042,3 +2042,43 @@ function system_update_8403() {
   + * Change plugin id's of actions.
   

   "plugin id's" -> "the plugin ID's"

3. +++ b/core/modules/system/system.install
   @@ -2042,3 +2042,43 @@ function system_update_8403() {
   +
   
   +++ b/core/modules/system/tests/src/Functional/Update/UpdateActionsWithEntityPluginsTest.php
   @@ -0,0 +1,45 @@
   +
   
   +++ b/core/tests/Drupal/KernelTests/Core/Action/PublishActionTest.php
   @@ -0,0 +1,89 @@
   +
   

   Unnecessary newline.

4. +++ b/core/modules/system/system.install
   @@ -2042,3 +2042,43 @@ function system_update_8403() {
   +  }
   +}
   

   Missing newline

Before the runUpdates() it's not possible to load the action, because the old plugin ID's don't exists anymore.

Fixed all the nits.

Perfect, thank you!!! <3

Can we deprecate the old classes, rather than removing them? I confirmed that actions only show up in the UI when they're supplied as default config, so with the update path and the new default config, we don't have to worry about the deprecated actions being surfaced as duplicates.

Sure, the old action plugins are now back as deprecated and they extend the new ones.

OK, thank you for your input @xjm and for the quick turnaround @chr.fritsch. Back to RTBC

For #2877383: Add action support to Media module we also need a DeleteAction. node and comment are implementing their own DeleteActions as well. So it would totally make sense to add a generic DeleteAction.
The question is, what about Drupal\entity\Routing\DeleteMultipleRouteProvider and Drupal\entity\Form\DeleteMultiple. They are needed that the DeleteAction can work OOTB. Should this functionality added here as well? Or should we pass everything around the generic DeleteAction to a follow-up?

I renamed the getInterface() to isApplicable. Now we are a little bit more flexible and a possible DeleteActionDeriver could use the EntityActionDeriverBase as well.

Ahh, that's a nice idea, makes sense.

Yes, I think handling the delete action in a separate issue makes sense.

@bojanz already opened an issue for that #2670730: Provide a delete action for each content entity type

I found out that @Sam152 did something similar in #2907075: Add EntityPublishedActionBase as a base class for all publishing status actions. So we have to merge them together nicely here.

I merged in some stuff from @Sam152. Mainly tests and some improvements around the plugin labels. So please also credit @Sam152 at commit.

Review as follows! I do wonder if it would have been more manageable to each of the types of action in their own issue, but happy to manage them here if that's easier for you. We pretty much landed on the same solution it seems, bar some differences in the testing, so happy if this issue replaces the one you linked in #26.

1. +++ b/core/config/schema/core.entity.schema.yml
   @@ -362,3 +362,15 @@ field.formatter.settings.entity_reference_label:
   +# Default schema for entity actions.
   +action.configuration.entity_publish_action:*:
   +  type: action_configuration_default
   +  label: 'Publish entity configuration'
   

   I think it *IS* the schema, not the default schema right? It's just got a wildcard to capture all the derivatives. Could be wrong, but I think that's how it works.

2. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriverBase.php
   @@ -0,0 +1,87 @@
   +/**
   + * Provides a base action for each entity types with specific interfaces.
   + */
   

   This is really neat, nice!

3. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriverBase.php
   @@ -0,0 +1,87 @@
   +   * The list consists of all entity types which implement the interface that
   +   * this deriver requires.
   

   Technically this could be based on anything, not just implementing interfaces right?

4. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityChangedActionDeriver.php
   @@ -0,0 +1,20 @@
   +/**
   + * Provides a save action for each content entity type.
   + */
   
   +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityPublishedActionDeriver.php
   @@ -0,0 +1,20 @@
   +/**
   + * Provides a publish/unpublish action for each content entity type.
   + */
   

   These aren't providing the actions, just the derivers. Maybe the comment could explain this more clearly? I think some @see references to the action classes would be great here too.

5. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/SaveAction.php
   @@ -0,0 +1,68 @@
   + * @Action(
   + *   id = "entity_save_action",
   + *   action_label = @Translation("Save"),
   + *   deriver = "Drupal\Core\Action\Plugin\Action\Derivative\EntityChangedActionDeriver",
   + * )
   ...
   +  public function execute($entity = NULL) {
   +    $entity->setChangedTime($this->time->getRequestTime())->save();
   +  }
   

   I'm probably missing the context from the media issue, but not sure why I would just save something? The label also doesn't really indicate updating the changed time? Wouldn't that be managed by the entity class itself? Ignore this if it's part of some other system/I'm missing something.

6. +++ b/core/modules/comment/src/Plugin/Action/PublishComment.php
   @@ -2,37 +2,18 @@
   + * @deprecated in Drupal 8.5.x, to be removed before Drupal 9.0.x. Use
   + * \Drupal\Core\Action\Plugin\Action\PublishAction instead.
   
   +++ b/core/modules/comment/src/Plugin/Action/SaveComment.php
   @@ -2,33 +2,18 @@
   + * @deprecated in Drupal 8.5.x, to be removed before Drupal 9.0.x. Use
   + * \Drupal\Core\Action\Plugin\Action\SaveAction instead.
   
   +++ b/core/modules/comment/src/Plugin/Action/UnpublishComment.php
   @@ -2,37 +2,18 @@
   + * @deprecated in Drupal 8.5.x, to be removed before Drupal 9.0.x. Use
   + * \Drupal\Core\Action\Plugin\Action\UnpublishAction instead.
   

   Do we indent these?

7. +++ b/core/modules/system/src/Entity/Action.php
   @@ -150,4 +150,18 @@ public static function sort(ConfigEntityInterface $a, ConfigEntityInterface $b)
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function calculateDependencies() {
   +    $dependencies = parent::calculateDependencies();
   +
   +    if (strpos($this->plugin, ':')) {
   +      list($deriver, $entity_type) = explode(':', $this->plugin);
   +      $module_name = \Drupal::entityTypeManager()->getDefinition($entity_type)->getProvider();
   +      $dependencies->addDependency('module', $module_name);
   +    }
   +    return $dependencies;
   +  }
   

   I don't know if we can assume all implementations of the plugin will create their derivatives in this scheme. I think if we're doing this, we'll need to add a calculateDependencies method to the actual plugins and delegate this logic to our specific actions.

   @see \Drupal\Component\Plugin\DependentPluginInterface for plugins responsible for exposing their own dependencies to their config entity counterparts and \Drupal\Core\Plugin\PluginDependencyTrait::calculatePluginDependencies for the code that gathers them up.

8. +++ b/core/modules/system/system.install
   @@ -2042,3 +2042,41 @@ function system_update_8403() {
   +    switch ($config->get('plugin')) {
   +      case 'comment_publish_action':
   +        $plugin = 'entity_publish_action:comment';
   +        break;
   +
   +      case 'comment_unpublish_action':
   +        $plugin = 'entity_unpublish_action:comment';
   +        break;
   +
   +      case 'comment_save_action':
   +        $plugin = 'entity_save_action:comment';
   +        break;
   +
   +      case 'node_publish_action':
   +        $plugin = 'entity_publish_action:node';
   +        break;
   +
   +      case 'node_unpublish_action':
   +        $plugin = 'entity_unpublish_action:node';
   +        break;
   +
   +      case 'node_save_action':
   +        $plugin = 'entity_save_action:node';
   +        break;
   +    }
   

   What about a simple map?

   $plugin_map = [
     'old' => 'new',
   ];
   $config->set('plugin', $plugin_map[$config->get('plugin')]);
   

9. +++ b/core/modules/system/tests/src/Functional/Update/UpdateActionsWithEntityPluginsTest.php
   @@ -0,0 +1,47 @@
   +    $this->databaseDumpFiles[0] = __DIR__ . '/../../../../tests/fixtures/update/drupal-8.bare.standard.php.gz';
   

   Why key to zero, [] will do the same thing?

10. +++ b/core/modules/system/tests/src/Functional/Update/UpdateActionsWithEntityPluginsTest.php
    @@ -0,0 +1,47 @@
    +    foreach ($array as $key => list($before, $after)) {
    

    Did not know this was possible! Cool.

11. +++ b/core/tests/Drupal/KernelTests/Core/Action/PublishActionTest.php
    @@ -0,0 +1,88 @@
    +/**
    + * Tests the publish and unpublish entity actions.
    + *
    + * @group action
    + */
    +class PublishActionTest extends KernelTestBase {
    
    +++ b/core/tests/Drupal/Tests/Core/Action/PublishActionTest.php
    @@ -0,0 +1,76 @@
    +/**
    + * @group Action
    + */
    +class PublishActionTest extends UnitTestCase {
    

    Do these tests cover the same thing?

12. +++ b/core/tests/Drupal/KernelTests/Core/Action/SaveActionTest.php
    @@ -0,0 +1,76 @@
    +      'id' => 'node_save_action',
    +      'label' => 'Publish node',
    

    Label mismatch.

Wow, nice catch with #7, you are absolutely correct! Thanks for spotting that

Thank you @Sam152 for the review.

1-4: Fixed
5: Node and comment have an action for re-saving. Media should get one, too. To trigger a re-save, we have to change something.
6-10: Fixed
11: Yes, they are covering the same thing. I think it's fine to remove KernelTestBase based ones. We are still covering these things in CommentActionsTest::testCommentPublishUnpublishActions()
12: Fixed

Fix the tests

Changes look great! Few little bits of nit feedback left.

1. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriverBase.php
   @@ -0,0 +1,87 @@
   +  abstract protected function isApplicable(EntityTypeInterface $entity_type);
   ...
   +  protected function getParticipatingEntityTypes() {
   

   Nit, but we use "applicable" and "participating". Why not standardise "getApplicableEntityTypes"?

2. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriverBase.php
   @@ -0,0 +1,87 @@
   +   * The list consists of all entity types which match the condition that
   +   * this deriver requires.
   

   Super nit, I think the "this" on the next line can be moved the previous line.

3. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/EntityActionBase.php
   @@ -0,0 +1,62 @@
   +/**
   + * Publishes an entity.
   + */
   +abstract class EntityActionBase extends ActionBase implements DependentPluginInterface, ContainerFactoryPluginInterface {
   

   I think the docs are wrong.

4. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/EntityActionBase.php
   @@ -0,0 +1,62 @@
   +  public function calculateDependencies() {
   +    $module_name = $this->entityTypeManager
   +      ->getDefinition($this->getPluginDefinition()['type'])
   +      ->getProvider();
   +    return ['module' => [$module_name]];
   

   Nice.

5. +++ b/core/modules/comment/src/Plugin/Action/PublishComment.php
   @@ -2,37 +2,18 @@
   + * @deprecated in Drupal 8.5.x, to be removed before Drupal 9.0.x.
   + *   Use \Drupal\Core\Action\Plugin\Action\PublishAction instead.
   

   May as well add all references to classes in comments to a @see so the docs work better.

Thanks for reviewing again. Here are the fixes

Looks good to me!

I really like the way this is turning out.

Detailed review, mostly small docs stuff but a couple questions and things related to the deprecations.

1. +++ b/core/config/schema/core.entity.schema.yml
   @@ -362,3 +362,15 @@ field.formatter.settings.entity_reference_label:
   +  label: 'Publish entity configuration'
   ...
   +  label: 'Save entity configuration'
   ...
   +  label: 'Unpublish entity configuration'
   

   I don't think the word "configuration" is right here. Configuration can't be published or unpublished. I think it's just publishing/unpublishing/saving?

2. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriverBase.php
   @@ -0,0 +1,87 @@
   + * Provides a base action for each entity types with specific interfaces.
   

   Typo: "each entity types".

3. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriverBase.php
   @@ -0,0 +1,87 @@
   +   * Returns if the deriver can be used for the provided entity type.
   

   Nit: "Returns if" would mean it only returns at all in that case. I think it returns regardless. :) Maybe:

   Indicates whether the deriver can be used..."

4. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriverBase.php
   @@ -0,0 +1,87 @@
   +   * The list consists of all entity types which match the condition that this
   +   * deriver requires.
   

   This is a little unclear; maybe an example would help? Maybe:

   The list consists of all entity types which match the conditions for the given deriver. For example, if the action applies to entities that are publishable,
   this method will find all entity types that are publishable.

5. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriverBase.php
   @@ -0,0 +1,87 @@
   +   *   The applicable entity types, keyed by entity type id.
   

   Nit: "ID" should always be capitalized.

6. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriverBase.php
   @@ -0,0 +1,87 @@
   +  protected function getApplicableEntityTypes() {
   

   +1 for this name.

7. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/EntityActionBase.php
   @@ -0,0 +1,62 @@
   + * Base class for entity based actions.
   

   Nit: "entity-based".

8. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/EntityActionBase.php
   @@ -0,0 +1,62 @@
   +   * @param array $configuration
   +   *   A configuration array containing information about the plugin instance.
   
   +++ b/core/lib/Drupal/Core/Action/Plugin/Action/SaveAction.php
   @@ -0,0 +1,75 @@
   +   * @param array $configuration
   +   *   A configuration array containing information about the plugin instance.
   

   Usually we shouldn't type a generic array unless it's a render array. Is it a mixed[][], a \Drupal\something\Something[], etc.?

   Also, it would help to explain what the array structure is. This could just be a reference to an example or another method that returns it.

   Sorry if this is just copied from other plugins in core.

9. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/EntityActionBase.php
   @@ -0,0 +1,62 @@
   +   *   The plugin_id for the plugin instance.
   
   +++ b/core/lib/Drupal/Core/Action/Plugin/Action/SaveAction.php
   @@ -0,0 +1,75 @@
   +   *   The plugin_id for the plugin instance.
   

   Nit: "plugin ID".

10. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/EntityActionBase.php
    @@ -0,0 +1,62 @@
    +   * @param mixed $plugin_definition
    +   *   The plugin implementation definition.
    
    +++ b/core/lib/Drupal/Core/Action/Plugin/Action/SaveAction.php
    @@ -0,0 +1,75 @@
    +   * @param mixed $plugin_definition
    +   *   The plugin implementation definition.
    

    Is there no low-level interface for the data type here?

    Again maybe this is copied from other plugins in core, but I figured it was worth asking.

11. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/PublishAction.php
    @@ -0,0 +1,38 @@
    +  public function access($object, AccountInterface $account = NULL, $return_as_object = FALSE) {
    +    $key = $object->getEntityType()->getKey('published');
    +
    +    /** @var \Drupal\Core\Entity\EntityInterface $object */
    +    $result = $object->access('update', $account, TRUE)
    +      ->andIf($object->$key->access('edit', $account, TRUE));
    +
    +    return $return_as_object ? $result : $result->isAllowed();
    

    Usually when we have access control methods we add some cacheability metadata; is that applicable here?

12. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/SaveAction.php
    @@ -0,0 +1,75 @@
    + * Provides an action that can save any entity.
    + *
    + * @Action(
    + *   id = "entity_save_action",
    + *   action_label = @Translation("Save"),
    + *   deriver = "Drupal\Core\Action\Plugin\Action\Derivative\EntityChangedActionDeriver",
    + * )
    + */
    +class SaveAction extends EntityActionBase {
    ...
    +    $entity->setChangedTime($this->time->getRequestTime())->save();
    

    FWIW (in response to the earlier question) this makes total sense to me. A save action would be used in e.g. a bulk save form and might be combined with other changes or updates. Plus, we have both node and comment save actions in core already, so we do need this.

13. +++ b/core/modules/comment/src/Plugin/Action/PublishComment.php
    @@ -2,37 +2,20 @@
    + * @deprecated in Drupal 8.5.x, to be removed before Drupal 9.0.x.
    + *   Use \Drupal\Core\Action\Plugin\Action\PublishAction instead.
    + *
    + * @see \Drupal\Core\Action\Plugin\Action\PublishAction
    
    +++ b/core/modules/comment/src/Plugin/Action/SaveComment.php
    @@ -2,33 +2,20 @@
    + * @deprecated in Drupal 8.5.x, to be removed before Drupal 9.0.x.
    + *   Use \Drupal\Core\Action\Plugin\Action\SaveAction instead.
    + *
    + * @see \Drupal\Core\Action\Plugin\Action\SaveAction
    
    +++ b/core/modules/comment/src/Plugin/Action/UnpublishComment.php
    @@ -2,37 +2,20 @@
    + * @deprecated in Drupal 8.5.x, to be removed before Drupal 9.0.x.
    + *   Use \Drupal\Core\Action\Plugin\Action\UnpublishAction instead.
    
    +++ b/core/modules/node/src/Plugin/Action/PublishNode.php
    @@ -2,25 +2,20 @@
    + * @deprecated in Drupal 8.5.x, to be removed before Drupal 9.0.x.
    + *   Use \Drupal\Core\Action\Plugin\Action\PublishAction instead.
    
    +++ b/core/modules/node/src/Plugin/Action/SaveNode.php
    @@ -2,36 +2,20 @@
    + * @deprecated in Drupal 8.5.x, to be removed before Drupal 9.0.x.
    + *   Use \Drupal\Core\Action\Plugin\Action\SaveAction instead.
    
    +++ b/core/modules/node/src/Plugin/Action/UnpublishNode.php
    @@ -2,25 +2,20 @@
    + * @deprecated in Drupal 8.5.x, to be removed before Drupal 9.0.x.
    + *   Use \Drupal\Core\Action\Plugin\Action\UnpublishAction instead.
    

    The deprecations should include a link to the change record (also we need a change record). See https://www.drupal.org/core/deprecation for examples.

    Also, minor nitpick, it should say "9.0.0". 9.0.x could be any time in the first minor release, which isn't what we mean. :)

14. +++ b/core/modules/comment/src/Plugin/Action/PublishComment.php
    @@ -2,37 +2,20 @@
    +class PublishComment extends PublishAction {}
    
    +++ b/core/modules/comment/src/Plugin/Action/SaveComment.php
    @@ -2,33 +2,20 @@
    +class SaveComment extends SaveAction {}
    
    +++ b/core/modules/comment/src/Plugin/Action/UnpublishComment.php
    @@ -2,37 +2,20 @@
    +class UnpublishComment extends UnpublishAction {}
    
    +++ b/core/modules/node/src/Plugin/Action/PublishNode.php
    @@ -2,25 +2,20 @@
    +class PublishNode extends PublishAction {}
    
    +++ b/core/modules/node/src/Plugin/Action/SaveNode.php
    @@ -2,36 +2,20 @@
    +class SaveNode extends SaveAction {}
    
    +++ b/core/modules/node/src/Plugin/Action/UnpublishNode.php
    @@ -2,25 +2,20 @@
    +class UnpublishNode extends UnpublishAction {}
    

    These should all have a @trigger_error() as well. See https://www.drupal.org/core/deprecation for examples.

15. +++ b/core/modules/content_moderation/src/Plugin/Action/ModerationOptOutPublish.php
    @@ -3,18 +3,20 @@
    -class ModerationOptOutPublishNode extends PublishNode implements ContainerFactoryPluginInterface {
    +class ModerationOptOutPublish extends PublishAction implements ContainerFactoryPluginInterface {
    
    +++ b/core/modules/content_moderation/src/Plugin/Action/ModerationOptOutUnpublish.php
    @@ -3,18 +3,20 @@
    -class ModerationOptOutUnpublishNode extends UnpublishNode implements ContainerFactoryPluginInterface {
    +class ModerationOptOutUnpublish extends UnpublishAction implements ContainerFactoryPluginInterface {
    

    This also seems BC-break-ish. Content Moderation is in beta so it needs to have BC too. So I guess we should deprecate the old ones for Content Moderation as well.

16. +++ b/core/modules/system/system.install
    @@ -2042,3 +2042,25 @@ function system_update_8403() {
    + * Change plugin ID's of actions.
    

    Nit: "plugin IDs".

Thanks!

Hm, I think maybe we should also have some tests for the config dependency calculations.

Regarding #36:
1-7: Fixed
8: I changed it to mixed[]. Not sure what else to write. We get an empty array there.
9: Fixed
10: Not sure what to write. It's the copied comment from the base class.
11: I checked several other core action plugins and none of them adds cacheability metadata.
13-16: Fixed

Also started with the CR: https://www.drupal.org/node/2919303

NR for testing the patch. Test for #37 still missing.

Is that enough for checking the dependency?

The new plugin classes look great!

1. +++ b/core/modules/content_moderation/content_moderation.module
   @@ -227,13 +227,15 @@ function content_moderation_action_info_alter(&$definitions) {
   +    if ($definition['id'] === 'entity_publish_action' && $definition['class'] == PublishAction::class) {
   ...
   +    if ($definition['id'] === 'entity_unpublish_action' && $definition['class'] == UnpublishAction::class) {
   

   With this strict ID check, the deprecated classes won't work. Shouldn't this provide BC?

2. +++ b/core/modules/content_moderation/src/Plugin/Action/ModerationOptOutPublish.php
   @@ -54,9 +70,11 @@ public static function create(ContainerInterface $container, array $configuratio
   +      $bundle_info = $this->bundleInfo->getBundleInfo($entity->getEntityTypeId());
   +      $bundle_label = $bundle_info[$entity->bundle()]['label'];
   
   +++ b/core/modules/content_moderation/src/Plugin/Action/ModerationOptOutUnpublish.php
   @@ -54,9 +70,11 @@ public static function create(ContainerInterface $container, array $configuratio
   +      $bundle_info = $this->bundleInfo->getBundleInfo($entity->getEntityTypeId());
   +      $bundle_label = $bundle_info[$entity->bundle()]['label'];
   

   Is there a reason this is preferred over \Drupal\Core\Entity\EntityTypeInterface::getBundleLabel()?

3. +++ b/core/modules/system/system.install
   @@ -2042,3 +2042,25 @@ function system_update_8403() {
   +function system_update_8500() {
   

   Ah, this is why we don't have to worry about BC above.

4. +++ b/core/modules/system/system.install
   @@ -2042,3 +2042,25 @@ function system_update_8403() {
   +    'comment_publish_action' => ['comment_publish_action', 'entity_publish_action:comment'],
   ...
   +  foreach ($array as $key => list($before, $after)) {
   
   +++ b/core/modules/system/tests/src/Functional/Update/UpdateActionsWithEntityPluginsTest.php
   @@ -0,0 +1,47 @@
   +      'comment_publish_action' => ['comment_publish_action', 'entity_publish_action:comment'],
   ...
   +    foreach ($array as $key => list($before, $after)) {
   ...
   +    foreach ($array as $key => list($before, $after)) {
   

   Why the need for list()?
   I'd expect this to be 'comment_publish_action' => 'entity_publish_action:comment', and foreach ($array as $before => $after) {

5. +++ b/core/modules/system/system.install
   @@ -2042,3 +2042,25 @@ function system_update_8403() {
   +    $config = \Drupal::configFactory()->getEditable('system.action.' . $key);
   

   In theory these entities are not the only thing that could be using action plugins, but it is the only usage in core. 👍

In theory these entities are not the only thing that could be using action plugins, but it is the only usage in core. 👍

Hm, actually, that might be a reason to do a more generic update path, in order to truly provide full BC.

I changed the update hook. Now it's more generic and should support not core actions as well.

Nice work! I found mostly small things to change, except for points 3. and 4. for which I'm setting back to NW.

1. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriverBase.php
   @@ -0,0 +1,89 @@
   +   * The entity type manager.
   +   *
   +   * @var \Drupal\Core\Entity\EntityManagerInterface
   

   This should be EntityTypeManagerInterface.

2. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriverBase.php
   @@ -0,0 +1,89 @@
   +   * Constructs a new EntityActionDeriver object.
   

   EntityActionDeriverBase ;)

3. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/PublishAction.php
   @@ -0,0 +1,38 @@
   +  public function execute($entity = NULL) {
   +    $entity->setPublished()->save();
   
   +++ b/core/lib/Drupal/Core/Action/Plugin/Action/SaveAction.php
   @@ -0,0 +1,75 @@
   +  public function execute($entity = NULL) {
   +    $entity->setChangedTime($this->time->getRequestTime())->save();
   
   +++ b/core/lib/Drupal/Core/Action/Plugin/Action/UnpublishAction.php
   @@ -0,0 +1,38 @@
   +  public function execute($entity = NULL) {
   +    $entity->setUnpublished()->save();
   

   Shouldn't we check if we actually have an entity object available?

4. +++ b/core/modules/system/system.install
   @@ -2042,3 +2042,28 @@ function system_update_8403() {
   +  $actions = \Drupal\system\Entity\Action::loadMultiple();
   

   We can't use the entity API in update hooks, this needs to get the config files with something like \Drupal::configFactory()->listAll('system.action.').

   See field_update_8001() for an example.

5. +++ b/core/modules/system/tests/src/Functional/Update/UpdateActionsWithEntityPluginsTest.php
   @@ -0,0 +1,47 @@
   + * Runs UpdateActionsWithEntityPluginsTest with a dump.
   

   How about "Tests the upgrade path for converting comment and node actions to generic entity ones"?

6. +++ b/core/modules/system/tests/src/Functional/Update/UpdateActionsWithEntityPluginsTest.php
   @@ -0,0 +1,47 @@
   +  public function testUpdateActionsWithEntityPlugins() {
   

   This needs a docblock as well, we can copy the one above. And we should also put a @see system_update_8500().

Alternatively, a solution for #43.4 would be using a post_update hook (they can use the Entity API), see block_post_update_fix_negate_in_conditions() for an example.

Thanks for reviewing, amateescu. I'm happy to see, that so much experienced devs jumped on this issue.

Fixed all the nits from #43.

Regarding #43.3: I think the derivers already take care about the correct input values.

Regarding #43.3: I think the derivers already take care about the correct input values.

Then we need to remove the default NULL value for the $entity argument :)

Then we need to remove the default NULL value for the $entity argument :)

That's not possible, because then the method is not compatible to the ExecutableInterface::execute() :(

Oh, okay, so the default NULL value is there to satisfy the parent interface which doesn't have this argument, makes sense.

I think we're good to go then.

#2909435: Update block library page to adapt publishable block content implementation also needs this.

Good catch @amateescu on #43.4; I missed that when reviewing it before.

+++ b/core/modules/system/tests/src/Functional/Update/UpdateActionsWithEntityPluginsTest.php
@@ -6,7 +6,7 @@
- * Runs UpdateActionsWithEntityPluginsTest with a dump.
+ * Tests the upgrade path for converting comment and node actions to generic entity ones.

@@ -19,6 +19,11 @@
+  /**
+   * Tests the upgrade path for converting comment and node actions to generic entity ones.

These two lines need to be under 80 characters. Attached fixes that.

+++ b/core/config/schema/core.entity.schema.yml
@@ -362,3 +362,15 @@ field.formatter.settings.entity_reference_label:
+  label: 'Publishing entity'
...
+  label: 'Saving entity'
...
+  label: 'Unpublishing entity'

All our other actions in core use the infinitive/imperative verb form, "Make content sticky." "Save content." Etc. Let's follow that pattern here; no -ing.

Ok, i changed the label.

Hopefully this was the final fix. Now i cross my fingers to get this committed :-)

Interdiff looks good, back to RTBC assuming it comes back green :)
PS. <3 this issue!

err... crap, ignore my files please, @chr.fritsch beat me to it and i forgot to remove them before posting my comment, do not give me credit for these! :S

@Manuel Garcia, you took the time to move the issue forward, just at the same time as someone else, credits don't cost much after all :)

+1 RTBC

Crossposts seemed to weirdify patches in the summary, so fixing that so that I don't commit the wrong patch. :P

I'm in the process of doing a thorough review here. Thanks everyone!

1. +++ b/core/config/schema/core.entity.schema.yml
   @@ -362,3 +362,15 @@ field.formatter.settings.entity_reference_label:
   +# Schema for entity actions.
   +action.configuration.entity_publish_action:*:
   +  type: action_configuration_default
   +  label: 'Publish entity'
   +
   +action.configuration.entity_save_action:*:
   +  type: action_configuration_default
   +  label: 'Save entity'
   +
   +action.configuration.entity_unpublish_action:*:
   +  type: action_configuration_default
   +  label: 'Unpublish entity'
   

   Don't we use action.configuration.*.* usually if we want to share the same configuration between things?

2. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/PublishAction.php
   @@ -0,0 +1,38 @@
   +      ->andIf($object->$key->access('edit', $account, TRUE));
   

   🙃 I like using $object->get($key), its less magic, but yeah this is just a personal nitpick.

3. +++ b/core/modules/content_moderation/src/Plugin/Action/ModerationOptOutPublish.php
   @@ -3,18 +3,20 @@
   +class ModerationOptOutPublish extends PublishAction implements ContainerFactoryPluginInterface {
   
   +++ b/core/modules/content_moderation/src/Plugin/Action/ModerationOptOutPublishNode.php
   @@ -2,65 +2,16 @@
   +class ModerationOptOutPublishNode extends ModerationOptOutPublish {}
   

   Idea for a follow up: Not extend PublishAction either, but just make an an empty execute implementation.

4. +++ b/core/tests/Drupal/Tests/Core/Action/PublishActionTest.php
   @@ -0,0 +1,78 @@
   +/**
   + * @group Action
   + */
   +class PublishActionTest extends UnitTestCase {
   
   +++ b/core/tests/Drupal/Tests/Core/Action/SaveActionTest.php
   @@ -0,0 +1,68 @@
   +/**
   + * @group Action
   + */
   +class SaveActionTest extends UnitTestCase {
   

   Putting my test maintainer head one for a sec. Are these unit tests really testing something? For example testSaveAction looks really coupled to the current code. Given that I'm wondering whether writing a kernel test instead wouldn't be better, and probably much smaller!

Thanks @dawehner. Setting NR for #59.

I changed the tests to be KernelTests based.

This needed a reroll because #2917883: Rename 'migration_templates' directories in core modules to 'migrations' landed

I adjusted the testGetDerivativeDefinitions calls to be more simpler as well.

Testbot will still fail, because since #2870194: Ensure that process-isolated tests can use Symfony's PHPunit bridge to catch usages of deprecated code landed it's not clear how to correctly mark plugins deprecated. There is an ongoing discussion on irc with berdir and alexpott. We will see.

I think we have to postpone this on #2922451: [policy no patch] Make it possible to mark plugins as deprecated

After some research in the plugin world, i think we could solve it without waiting for #2922451: [policy no patch] Make it possible to mark plugins as deprecated.

I added FallbackPluginManagerInterface to the ActionManager to return the new plugin_id's in case a old one should be loaded. Then i removed the @Action annotation, that means, the old plugins are no longer picked up from the plugin discovery.

Fixed the last failing test, by just loading the config item before the update execution.

I updated #66 based on the current proposal from #2922451: [policy no patch] Make it possible to mark plugins as deprecated.

Regarding:

Could you also check for my other comment in #64 regarding the change to action.configuration.*.*.

Just checking, but don't we need the label for each separate action?

I think it's fine because action labels from the schema are not exposed to the UI. Action definitions have their labels which are used.

+++ b/core/modules/comment/config/schema/comment.schema.yml
@@ -15,14 +15,6 @@ field.widget.settings.comment_default:
 
-action.configuration.comment_publish_action:
-  type: action_configuration_default
-  label: 'Publish comment configuration'
-
-action.configuration.comment_save_action:
-  type: action_configuration_default
-  label: 'Save comment configuration'
-

We are not removing this, we are just deprecating it, so we can not remove the config schema, we can only do that once we actually remove the classes in 9.x

Thanks for reviewing @Berdir.

I brought the schemas back and added a deprecation message to them.

Also, the last failing test should be fixed.

1. +++ b/core/modules/content_moderation/src/Plugin/Action/ModerationOptOutPublishNode.php
   @@ -2,65 +2,16 @@
   +@trigger_error(__NAMESPACE__ . '\ModerationOptOutPublishNode is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\content_moderation\Plugin\Action\ModerationOptOutPublish instead. See https://www.drupal.org/node/2919303.', E_USER_DEPRECATED);
   
   +++ b/core/modules/content_moderation/src/Plugin/Action/ModerationOptOutUnpublishNode.php
   @@ -2,65 +2,16 @@
   +@trigger_error(__NAMESPACE__ . '\ModerationOptOutUnpublishNode is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\content_moderation\Plugin\Action\ModerationOptOutUnpublish instead. See https://www.drupal.org/node/2919303.', E_USER_DEPRECATED);
   

   Shouldn't this deprecation be part of the constructor as well?

2. +++ b/core/modules/system/system.install
   @@ -2056,3 +2056,26 @@ function system_update_8403() {
   +  $array = [
   
   +++ b/core/modules/system/tests/src/Functional/Update/UpdateActionsWithEntityPluginsTest.php
   @@ -0,0 +1,53 @@
   +    $array = [
   

   For a bit of more clarity I would named it old_new_action_id_map or something along those lines.

3. +++ b/core/tests/Drupal/KernelTests/Core/Action/PublishActionTest.php
   @@ -0,0 +1,78 @@
   +class PublishActionTest extends KernelTestBase {
   
   +++ b/core/tests/Drupal/KernelTests/Core/Action/SaveActionTest.php
   @@ -0,0 +1,61 @@
   +class SaveActionTest extends KernelTestBase {
   

   Is there a reason why there is a saving and a publish test, but not one for unpublish?

Fixed #76

1. Missed that, thanks
2. Done
3. There is a test for unpublish in PublishActionTest. The reason I added it there is PublishAction and UnpublishAction are using the same deriver.

Testbot hickup

1. +++ b/core/config/schema/core.entity.schema.yml
   @@ -362,3 +362,7 @@ field.formatter.settings.entity_reference_label:
   +# Schema for entity actions.
   +action.configuration.*:*:
   +  type: action_configuration_default
   +  label: 'Entity action'
   

   this looks very generic. Can't we have action.configuration.entity:*:* maybe by renaming the plugin's,
   for example,
   entity:publish_action

2. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriverBase.php
   --- /dev/null
   +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityChangedActionDeriver.php
   
   +++ b/core/modules/system/system.install
   @@ -2056,3 +2056,26 @@ function system_update_8403() {
   +  $array = [
   

   how about $action_plugin_map?

3. +++ b/core/modules/comment/tests/src/Functional/CommentActionsTest.php
   @@ -32,7 +32,7 @@ public function testCommentPublishUnpublishActions() {
   +    $this->assertArraySubset(['module' => ['comment']], $action->getDependencies());
   

   Nice! extra test coverage.

4. +++ b/core/modules/system/system.install
   @@ -2056,3 +2056,26 @@ function system_update_8403() {
   +      $config->save();
   

   Should trust the data to avoid schema rebuilds since that can cause issues at this point in an update.

Thanks for reviewing @alexpott

I fixed everything from #80

Testbot hickup

I've read through all the issue comments to try to ensure all the feedback has been incorporated.

+++ b/core/modules/system/system.install
@@ -2056,3 +2056,26 @@ function system_update_8403() {
+/**
+ * Change plugin IDs of actions.
+ */
+function system_update_8500() {
+  $old_new_action_id_map = [
+    'comment_publish_action' => 'entity:publish_action:comment',
+    'comment_unpublish_action' => 'entity:unpublish_action:comment',
+    'comment_save_action' => 'entity:save_action:comment',
+    'node_publish_action' => 'entity:publish_action:node',
+    'node_unpublish_action' => 'entity:unpublish_action:node',
+    'node_save_action' => 'entity:save_action:node',
+  ];
+
+  $actions = \Drupal::configFactory()->listAll('system.action.');
+  foreach ($actions as $action) {
+    $config = \Drupal::configFactory()->getEditable($action);
+    if (isset($old_new_action_id_map[$config->get('plugin')])) {
+      $config->set('plugin', $old_new_action_id_map[$config->get('plugin')]);
+      $config->save(TRUE);
+    }
+  }
+}

Thinking about this some more - it probably should be a post update and use the action configuration entity and not use raw configuration. This makes it less special and allows us to test the dependency calculation in UpdateActionsWithEntityPluginsTest which is something @xjm asked for in #37 - also @tim.plunkett had the idea of post updates in #44.

In #40.5 @tim.plunkett noted that the action plugins can be used by other things than the action configuration entities in contrib. The CR and the deprecation notices help to inform these use-cases - there's nothing else core can do.

+++ b/core/tests/Drupal/KernelTests/Core/Action/PublishActionTest.php
@@ -0,0 +1,78 @@
+class PublishActionTest extends KernelTestBase {

+++ b/core/tests/Drupal/KernelTests/Core/Action/SaveActionTest.php
@@ -0,0 +1,61 @@
+class SaveActionTest extends KernelTestBase {

Let's also add dependency checking to these tests so that it's not just the Update test and the Comment test that give us coverage.

Ok, I changed the update hook to be a post update and added dependency checks to UpdateActionsWithEntityPluginsTest, PublishActionTest and SaveActionTest.

As suggested by @alexpott via slack, I had to add the deprecation messages to DeprecationListenerTrait::getSkippedDeprecations in order to prevent the warnings in WebTest based update path tests.

Opened two issues as a result on #86/#88

• #2934336: Update symfony/phpunit-bridge to the latest released version
• #2934340: Remove action deprecations from DeprecationListenerTrait::getSkippedDeprecations

+1 to adding them to the skip list fort now because all upgrade path tests are going to trigger them.

Adding issue credit to people for their constructive reviews.

All feedback has been adressed, it seems to me. Read through the entire patch again and couldn't find anything to complain about. Thanks!! Let's get this in.

Just a small documentation fix

Oh, nice catch! Thanks. Still looks good.

1. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/SaveAction.php
   @@ -0,0 +1,75 @@
   +    return $object->access('update', $account, $return_as_object);
   
   +++ b/core/lib/Drupal/Core/Action/Plugin/Action/UnpublishAction.php
   @@ -0,0 +1,38 @@
   +    $result = $object->access('update', $account, TRUE)
   +      ->andIf($object->$key->access('edit', $account, TRUE));
   

   Any reason why the save action doesn't check field access like the other two? I tried to scan the comments to see why, but could not find any mention of it.

I suppose the reason is that the current SaveNode and SaveComment where this was taken from do not do that.

...but we totally should check that as well! Nice catch.

I looked into it a bit, I don't know how to check the field access because we don't have an entity key for the changed field. That means every entity type the implements EntityChangedInterface, could save the changed value in a different field. So we can not predict on which field the access should be checked.

So setting this back to NR.

Ahh, yes that's right. This is a bit weird currently in core. In \Drupal\Core\Entity\EntityChangedTrait we actually hardcode the field to be called 'changed' (which has also been discussed in #2209971: Automatically provide a changed field definition), which we could do here, as well. One could also argue, though, that the usage of the trait is not required even for people implementing the interface, so we shouldn't hardcode it here.

Leaving at needs review for now.

So maybe a way forward is to make sure that the save fails if the user doesn't have access. We can also check if the entity uses the trait and if it does check access for the changed field. That way if the entity uses a different field and the user has access it'll still work and we'll have tested what happens if the user does not have access. And for most use-cases ::access() will work as expected.

Another thought is we could make the action only applicable if the entity uses the trait.

Crediting @larowlan for the review.

I tried the approach to check if the object implements the trait and then check the field access on the changed field directly.

I had to use orIf to combine both results because $object->changed->access('edit', $account, TRUE) returns AccessResultNeutral.

AccessResultNeutral andIf AccessResultAllowed => AccessResultNeutral
AccessResultNeutral orIf AccessResultAllowed => AccessResultAllowed

+++ b/core/lib/Drupal/Core/Action/Plugin/Action/PublishAction.php
@@ -0,0 +1,38 @@
+    /** @var \Drupal\Core\Entity\EntityInterface $object */
+    $result = $object->access('update', $account, TRUE)
+      ->andIf($object->$key->access('edit', $account, TRUE));

Re #101: but why does the above work, then? I guess I am missing something but they look pretty similar to me.

Checkin the field access for the 'status' field returns AccessResultAllowed - not AccessResultNeutral.

I think it's because of NodeAccessControlHandler::checkFieldAccess

  protected function checkFieldAccess($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {
    // Only users with the administer nodes permission can edit administrative
    // fields.
    $administrative_fields = ['uid', 'status', 'created', 'promote', 'sticky'];
    if ($operation == 'edit' && in_array($field_definition->getName(), $administrative_fields, TRUE)) {
      return AccessResult::allowedIfHasPermission($account, 'administer nodes');
    }

I think that the default field access is allowed... see \Drupal\Core\Entity\EntityAccessControlHandler::checkFieldAccess()

Ok, now I found the real reason. It's Drupal\Core\Field\ChangedFieldItemList which doesn't allow to edit the change field.

So maybe it's the best to go with the solution we had before. Just checking if the user has 'update' permissions.
What the save action should achieve is only resaving the entity. But to do that we have to at least change one value. So changing the 'change' field is at least a workaround to tell the storage, something has changed, please resave my entity.

Yes, I think #106 is a very good explanation. Would be nice to confirm with @alexpott, but for what it's worth I agree with your assessment.

Here is a patch, based on my proposal from #106

@chr.fritsch works for me - it'd be also great to check with @larowlan as the access issue was raised by him in #94. I also think a comment to explain why field access is not taking into account is a good idea.

+++ b/core/lib/Drupal/Core/Action/Plugin/Action/SaveAction.php
@@ -69,20 +69,7 @@
   public function access($object, AccountInterface $account = NULL, $return_as_object = FALSE) {

I think a comment is worth it here.

Ok, fine. Here is some explanation.

This looks good to me

Thanks everyone. This is good to go.

Committed cbd3299 and pushed to 8.5.x. Thanks!

Thank you everybody for your patience. I am so happy to see this landed. I hope to see you all in #2670730: Provide a delete action for each content entity type, the last blocker before we can fix #2877383: Add action support to Media module in a proper way.

Currently the CR for this only details which plugins were deprectated.
Can we update it to document what a contrib entity type would need to do to benefit from this?

Something similar to what the generic delete action CR has would be very beneficial: https://www.drupal.org/node/2934349

The change in #80.1/81 broke getBaseId() and getDerivativeId() for Action plugins.

This was not a regression in HEAD at the time, because before this, Action plugins didn't use derivatives.
But #2458769: Derivative plugin definitions contain base plugin IDs instead of derivative IDs is exposing this bug.

Consider the plugin ID entity:publish_action:node
It is defined as entity:publish_action on the plugin, and the :node is added by the deriver.
Code:

$plugin = \Drupal::service('plugin.manager.action')->createInstance('entity:publish_action:node');
var_dump($plugin->getPluginId());
var_dump($plugin->getBaseId());
var_dump($plugin->getDerivativeId());

Expected:

string(26) "entity:publish_action:node"
string(21) "entity:publish_action"
string(4) "node"

Actual:

string(26) "entity:publish_action:node"
string(6) "entity"
string(19) "publish_action:node"

I might try to fix it in the other issue, or I may open a new issue.

Hi, i'm trying to figure out whether to open a new issue to add FallbackPluginManagerInterface to ActionManager, and I found this thread.

Looks like it was added in #68, then dropped off in subsequent patches:

I added FallbackPluginManagerInterface to the ActionManager to return the new plugin_id's in case a old one should be loaded. Then i removed the @Action annotation, that means, the old plugins are no longer picked up from the plugin discovery.

Can anyone help me understand why FallbackPluginManagerInterface and a default "Broken" action plugin was not ultimately included in this commit, and whether I should open a new issue to request it?