Provide a delete action for each content entity type

Postpone this on #2916740: Add generic entity actions, because this should be built on top of EntityActionDeriverBase

First patch based on #2916740: Add generic entity actions

Updated patch a bit. 2670730-9-full.patch also contains changes of #2916740: Add generic entity actions

I introduced a new content type key, to get the multiple delete form route for existing content types.

I applied #2879435: Make DeleteAction compatible with ConfigEntities on top of this patch and improved the messages.

Hopefully i fixed all the tests.

#2916740: Add generic entity actions is RTBC, so let's move on here.

I updated the patch accordingly to all the adjustments that happen in #2916740: Add generic entity actions.

Added DeleteMultipleFormTest from contrib entity module.

#2916740: Add generic entity actions lands. Wohoo 🎉
So unpostponing this.

Added "Media Initiative" tag because it's important for us in order to fix #2877383: Add action support to Media module

1. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultiple.php
   @@ -0,0 +1,256 @@
   +class DeleteMultiple extends ConfirmFormBase {
   

   I find it strange to have form classes without form suffix.

2. +++ b/core/lib/Drupal/Core/Entity/Routing/DeleteMultipleRouteProvider.php
   @@ -0,0 +1,45 @@
   +      $route->setRequirement('_permission', $entity_type->getAdminPermission());
   

   this doesn't work. Not just that not all entity types have an admin permission, it is also wrong.

   See #2779389: Node bulk delete confirmation from requires 'administer nodes' permission.

   What we likely need is a custom access check that ensures that the user can delete the entities in the tempstore.

I tried to address @Berdir's feedback.

There is currently one big issue with this patch. EntityDeleteMultipleAccessCheck and DeleteMultipleForm are sitting in the core space but they are depending on the user module because of the tempstore.

But moving them into the user modules feels wrong to me.

+++ b/core/lib/Drupal/Core/Entity/EntityDeleteMultipleAccessCheck.php
@@ -0,0 +1,62 @@
+  public function access(AccountInterface $account, $entity_type_id) {
+    $selection = $this->tempStore->get($account->id());
+    $storage = $this->entityTypeManager->getStorage($entity_type_id);
+    /** @var \Drupal\Core\Entity\EntityInterface[] $entities */
+    $entities = $storage->loadMultiple(array_keys($selection));
+    foreach ($selection as $id => $langcodes) {
+      $entity = $entities[$id];
+      if (!$entity->access('delete', $account)) {
+        return AccessResult::forbidden();
+      }
+    }
+    return AccessResult::allowed();

I'm wondering if we can limit it to having at least one entity that we can delete.

We could then also check on the form itself before deleting if the user is allowed to, but we already check before adding them to the temp store, so the only scenario that I can think of right now is that you remove delete permission *between* adding them to tempstore and actually deleting them. Which usually is just a few seconds.

I'm not sure about the module, but I know @plach recently basically blocked an issue because it added a new \Drupal\user dependency in \Drupal\Core\Entity so I don't think keeping it there is an option.

So afaik, our only options are:
* user.module: Because we depend on it, it is required so everyone has it
* another module that depends on user, not sure which that should be.
* system.module: both system and user are required and depend on each other a lot. The action config entity also lives there.

system.module: both system and user are required and depend on each other a lot. The action config entity also lives there.

System has my vote. Every site has it, so it might as well be part of the \Drupal\Core namespace.

Also, the DeleteAction requires the temp store and it would be a shame if DeleteAction lives in another place then PublishAction/UnpublishAction and SaveAction.

Every site has both the system and user module. Either module is fine. It could be argued that we should be looking to move them totally into the core namespace at some point - but is the disruption worth it? I vote for system too as it is the more generic of the two modules. The other option is to move the user.shared_tempstore and user.private_tempstore into core. These services have no dependencies on user code apart from themselves. However moving this will be disruptive and whilst BC is possible for most things the \Drupal\user\TempStoreException will be tricky. We could make it extend from the new core TempStoreException but core class can't throw \Drupal\user\TempStoreException. I think if we want Delete actions to be in core we need to do the service move. The path of least resistance is to put the new actions in system.

I'm wondering if we could do the following:

- move tempstore to core
- subclass with deprecations in user module
- for the exception in core, use class_alias() to simulate \Drupal\user\TempStoreException one. That way it could throw the same exception without an actual dependency on user module or an API change.

No way to trigger_error() if we do that, but obviously we can document it with a change record and anything catching the core exception will be forwards compatible with 9.x still.

@catch trying to do all of that in #2935617: Move User module's temp stores to core

So this patch is now based on top of #2935617: Move User module's temp stores to core.

I also tried to address #28

1. +++ b/core/lib/Drupal/Core/Entity/EntityDeleteMultipleAccessCheck.php
   @@ -0,0 +1,67 @@
   +    foreach ($selection as $id => $langcodes) {
   +      $entity = $entities[$id];
   +      if ($entity->access('delete', $account)) {
   +        return AccessResult::allowed();
   +      }
   +    }
   +    return AccessResult::forbidden();
   

   I think it is worth explaining that as long as you can delete one it will proceed but access is checked again in the ::submitForm() and only entities you are allowed to delete are deleted.

2. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,259 @@
   +      if (!$entity->access('delete', $this->currentUser)) {
   +        continue;
   +      }
   

   It's great that we check access again - just incase it changed after adding to the temp store but I also wonder if we should notify the user in some way about what has not been deleted. Yes this is new functionality but I think I it has been brought about by the change @Berdir requested in #28.

3. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,259 @@
   +      foreach ($langcodes as $langcode) {
   +        if ($entity instanceof TranslatableInterface) {
   +          $entity = $entity->getTranslation($langcode);
   +          if ($entity->isDefaultTranslation()) {
   +            $delete_entities[$id] = $entity;
   +            unset($delete_translations[$id]);
   +            $total_count += count($entity->getTranslationLanguages());
   +          }
   +          elseif (!isset($delete_entities[$id])) {
   +            $delete_translations[$id][] = $entity;
   +          }
   +        }
   +        elseif (!isset($delete_entities[$id])) {
   +          $delete_entities[$id] = $entity;
   +        }
   +      }
   +    }
   ...
   +        $count += count($translations);
   ...
   +        $total_count += $count;
   ...
   +    if ($total_count) {
   +      drupal_set_message($this->formatPlural($total_count, 'Deleted @count @item.', 'Deleted @count @items.', [
   +        '@item' => $this->entityDefinition->getSingularLabel(),
   +        '@items' => $this->entityDefinition->getPluralLabel(),
   +      ]));
   +    }
   

   Is there test coverage of all the counting logic? I don't think non translatable entities are counted correctly. Not sure though - it is complex.

4. +++ b/core/modules/action/tests/src/Functional/BulkFormTest.php
   @@ -148,7 +148,7 @@ public function testBulkForm() {
   -    $this->assertText(t('Deleted 5 posts.'));
   +    $this->assertText(t('Deleted 5 content items.'));
   

   Nice - consistency!

5. +++ b/core/modules/comment/src/Form/ConfirmDeleteMultiple.php
   @@ -2,76 +2,21 @@
   +use Drupal\Core\Entity\Form\DeleteMultipleForm as EntityDeleteMultiple;
   

   as EntityDeleteMultipleForm - for consistency - there a re multiple instances of this.

6. +++ b/core/modules/media/src/Form/MediaDeleteMultipleConfirmForm.php
   @@ -2,208 +2,12 @@
    use Drupal\Core\Url;
   

   Not used

7. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
   @@ -141,6 +141,8 @@ public static function getSkippedDeprecations() {
   +      'Drupal\node\Plugin\Action\DeleteNode is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\DeleteAction instead. See https://www.drupal.org/node/2934349.',
   +      'Drupal\comment\Plugin\Action\DeleteComment is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\DeleteAction instead. See https://www.drupal.org/node/2934349.',
   

   Yeah we have to add these here. Because update path tests are not yet in the legacy group and they are not all PHPUnit based tests.

Thanks for reviewing @alexpott

#36.1: Added some notes
#36.2: Injected the messenger and inform user if some entities are not possible to delete
#36.3: Extended the test coverage in DeleteMultipleFormTest. Now we also add translations to the tempstore. For me it seems that the calculation is correct.
#36.5: Fixed that in node and comment
#36.6: I reverted all the changes in MediaDeleteMultipleConfirmForm because we are going to deprecate this class in #2935610: Remove or deprecate MediaDeleteMultipleConfirmForm and it's route definition

+++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
@@ -0,0 +1,279 @@
+    foreach ($this->selection as $id => $langcodes) {
+      $entity = $entities[$id];
+      if (!$entity->access('delete', $this->currentUser)) {
+        $not_accessible_entities[] = $entity;
+        continue;
+      }
+      foreach ($langcodes as $langcode) {
+        if ($entity instanceof TranslatableInterface) {
+          $entity = $entity->getTranslation($langcode);
+          if ($entity->isDefaultTranslation()) {
+            $delete_entities[$id] = $entity;
+            unset($delete_translations[$id]);
+            $total_count += count($entity->getTranslationLanguages());
+          }
+          elseif (!isset($delete_entities[$id])) {
+            $delete_translations[$id][] = $entity;
+          }
+        }
+        elseif (!isset($delete_entities[$id])) {
+          $delete_entities[$id] = $entity;
+        }
+      }
+    }

I think the logic here could benefit from commentary. Something like:

    foreach ($this->selection as $id => $langcodes) {
      $entity = $entities[$id];
      if (!$entity->access('delete', $this->currentUser)) {
        $not_accessible_entities[] = $entity;
        continue;
      }
      foreach ($langcodes as $langcode) {
        if ($entity instanceof TranslatableInterface) {
          $entity = $entity->getTranslation($langcode);
          // If the entity is the default translation then deleting it will
          // delete all the translations.
          if ($entity->isDefaultTranslation()) {
            $delete_entities[$id] = $entity;
            // If there are translations already marked for deletion then remove
            // them as they will be deleted anyway.
            unset($delete_translations[$id]);
            // Update the total count. Since a single delete will delete all
            // translations, we need to add the number of translations to the
            // count.
            $total_count += count($entity->getTranslationLanguages());
          }
          // Add the translation to the list of translations to be deleted
          // unless the default translation is being deleted.
          elseif (!isset($delete_entities[$id])) {
            $delete_translations[$id][] = $entity;
          }
        }
        elseif (!isset($delete_entities[$id])) {
          $delete_entities[$id] = $entity;
          $total_count++;
        }
      }
    }

The code above also fixes the count for entities that don't implement TranslatableInterface.

I also wonder if a user can have access to delete translations but not the entity. Looking at the code in content_translation it looks possible. But the existing code does not cope with this so I guess we could open a follow-up.

Also

+++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
@@ -0,0 +1,279 @@
+            $total_count += count($entity->getTranslationLanguages());
...
+    if ($delete_entities) {
+      $storage->delete($delete_entities);
+      $message = $this->formatPlural(count($delete_entities), 'Deleted @count @item.', 'Deleted @count @items.', [
+        '@item' => $this->entityDefinition->getSingularLabel(),
+        '@items' => $this->entityDefinition->getPluralLabel(),
+      ]);
+      $this->logger('content')->notice($message);
+    }
...
+      if ($count) {
+        $total_count += $count;
+        $message = $this->formatPlural($count, 'Deleted @count @item translation.', 'Deleted @count @items translations.', [
+          '@item' => $this->entityDefinition->getSingularLabel(),
+          '@items' => $this->entityDefinition->getPluralLabel(),
+        ]);
+        $this->logger('content')->notice($message);
+      }
...
+    if ($total_count) {
+      $this->messenger->addStatus($this->formatPlural($total_count, 'Deleted @count @item.', 'Deleted @count @items.', [
+        '@item' => $this->entityDefinition->getSingularLabel(),
+        '@items' => $this->entityDefinition->getPluralLabel(),
+      ]));
+    }

The mixing of entity counts with translation counts in the message the user gets as opposed to the messages left in the log are weird. This happens in HEAD too but is perhaps worth a follow-up.

1. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityDeleteActionDeriver.php
   @@ -0,0 +1,46 @@
   +    return $entity_type->entityClassImplements(EntityInterface::class) && ($entity_type->get('delete_multiple_form_route') || $entity_type->hasLinkTemplate('delete-multiple-form'));
   

   Is checking EntityInterface really necessary? Seems a lot of things that are much worse than the multiple delete will be broken if an entity type does not implement this.

2. +++ b/core/lib/Drupal/Core/Entity/EntityDeleteMultipleAccessCheck.php
   @@ -0,0 +1,71 @@
   +    foreach ($selection as $id => $langcodes) {
   +      $entity = $entities[$id];
   ...
   +      if ($entity->access('delete', $account)) {
   

   I think similarly to #39 we should iterate over all translations and check access for each.

3. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,279 @@
   +   * The entity type id.
   

   id -> ID

4. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,279 @@
   +    $this->tempStore = $temp_store_factory->get('entity_delete_multiple_confirm');
   ...
   +    $this->selection = $this->tempStore->get($this->currentUser->id());
   

   So the selection is not entity-type-specific, it seems. What if I delete both multiple nodes and multiple comments simultaneously, i.e. in separate browser tabs?

5. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,279 @@
   +    return $this->formatPlural(count($this->selection), 'Are you sure you want to delete this @item?', 'Are you sure you want to delete these @items?', [
   +      '@item' => $this->entityDefinition->getSingularLabel(),
   +      '@items' => $this->entityDefinition->getPluralLabel(),
   

   I think it would make sense to use the same counting here that we later use for the status message below, i.e. $total_count instead of count($this->selection). Perhaps we can put the splitting up $this->selection in a helper method?

6. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,279 @@
   +    return new Url('entity.' . $this->entityTypeId . '.collection');
   

   We should check whether a collection link template exists and fall back to the front page otherwise (or is there anything more sensible to fall back to?). That will also avoid having to add a collection in the test coverage.

7. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,279 @@
   +    foreach ($this->selection as $id => $langcodes) {
   ...
   +          $languages = $entity->getTranslationLanguages();
   

   To avoid confusion we could rename this to $selected_langcodes and $entity_languages.

   Also $(entity_)languages could be outside of the inner foreach, as well.

8. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,279 @@
   +        $entity = $entities[$id];
   ...
   +          $default_key = $id . ':' . $entity->getUntranslated()->language()->getId();
   

   This could be outside of the inner foreach which would actually make the code more readable in my opinion.

9. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,279 @@
   +    $not_accessible_entities = [];
   

   I think "inaccessible" is better than "not accessible".

10. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
    @@ -0,0 +1,279 @@
    +    /** @var \Drupal\Core\Entity\ContentEntityInterface[] $entities */
    

    Why should these be content entities? EntityDeleteActionDeriver actually doesn't check for ContentEntityInterface and we explicitly check TranslatableInterface below.

11. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
    @@ -0,0 +1,279 @@
    +            $total_count += count($entity->getTranslationLanguages());
    ...
    +        $total_count += $count;
    

    I get why we are building up $total_count differently for entity deletes vs. translation deletes but it could use a code comment in my opinion.

12. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
    @@ -0,0 +1,279 @@
    +      $count = 0;
    ...
    +        $count += count($translations);
    

    I recently learned that count($foo, COUNT_RECURSIVE) is a thing, that could be used here.

13. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
    @@ -0,0 +1,279 @@
    +      $this->messenger->addWarning($this->formatPlural($total_count, '@count @item couldn\'t be deleted, because you don\'t have the permission to do that.', '@count @items couldn\'t be deleted, because you don\'t have the permission to do that.', [
    

    Let's use double quotes for the string so we can avoid the escaping.

14. +++ b/core/lib/Drupal/Core/Entity/Routing/DeleteMultipleRouteProvider.php
    @@ -0,0 +1,45 @@
    +class DeleteMultipleRouteProvider implements EntityRouteProviderInterface {
    

    Is the reason we are not putting this into DefaultHtmlRouteProvider the fact that that is not @internal anymore? That would be unfortunate in my opinion, if adding that would be considered a BC break.

15. +++ b/core/lib/Drupal/Core/Entity/Routing/DeleteMultipleRouteProvider.php
    @@ -0,0 +1,45 @@
    +      $route->setDefault('_form', '\Drupal\Core\Entity\Form\DeleteMultipleForm');
    

    I think to be more consistent with other routes we should use a form operation for this, i.e. do $entity_type->getFormClass('delete-multiple-confirm') here (and add a respective ->hasFormClass() to the if check). That would also allow Node and Comment to cleanly override the form class.

16. +++ b/core/modules/action/migrations/d6_action.yml
    @@ -26,9 +26,11 @@ process:
    +        comment_delete_action: entity:delte_action:comment
    
    +++ b/core/modules/action/migrations/d7_action.yml
    @@ -23,9 +23,11 @@ process:
    +        comment_delete_action: entity:delte_action:comment
    

    Missing "e" in "delte". I guess that means we are missing test coverage?

17. +++ b/core/modules/comment/src/Entity/Comment.php
    @@ -61,6 +61,7 @@
    + *   delete_multiple_form_route = "comment.multiple_delete_confirm",
    
    +++ b/core/modules/node/src/Entity/Node.php
    @@ -66,6 +66,7 @@
    + *   delete_multiple_form_route = "node.multiple_delete_confirm",
    

    I'm really not excited about adding another top-level entity type key, especially when it's really just for BC as new entity types will just use the link template and auto-generated route.

    I would rather have Node and Comment use the link template as well and then have them declare duplicated routes i.e. provide a "entity.comment.delete_multiple_confirm" route which just duplicates "comment.multiple_delete_confirm". Then we can deprecate the latter once [#928750] lands.

18. +++ b/core/modules/system/tests/src/Functional/Update/UpdateActionsWithEntityPluginsTest.php
    @@ -53,4 +53,33 @@ public function testUpdateActionsWithEntityPlugins() {
    +    // comment_delete_actions is not part of the dump files.
    

    Let's add it then?

Also re #39: You are absolutely right, but I don't think we should handle that in a follow-up. This might be a pre-existing bug with nodes and comments, but I think we should avoid introducing bugs in new code. Just my 2 cents.

re COUNT_RECURSIVE - I'm not sure it'll be that useful here - see https://3v4l.org/cjDeh. I think for it to be useful we'd need it to return 5 and not 7.

As as far as I can the whole translation deletion as provided by content_translation is encapsulated in that module so putting something into core for it would wrong. As this is not introducing a new bug doing things in smaller steps especially steps that might not even be simple is preferable - thats why I recommended a follow-up.

Re #43: Oops I totally thought that would yield 5, yes. Sorry about that and thanks for setting me straight

Re #44: Hmm, I guess I am missing something, but doesn't #39 mean that potentially translations are left around that should have been deleted? That does seem like a bug and one that we are introducing here. Maybe you can elaborate...

I addressed #39 and then started with #41.

1. Removed that check
2. Still open
3. Fixed
4. Fixed, added the entity type ID to the key
5. Still open
6. Fixed
7. Fixed
8. Moved things up, as far as possible
9. Fixed
10. Changed to TranslatableInterface
11. Still open
12. Still open
13. Fixed
14. Removed the new router and added it to DefaultHtmlRouteProvider. I think API additions should be ok.
15. Fixed
16. Fixed, test coverage might still be needed
17. Removed the entire delete_multiple_form_route key handling
18. Still open

Try to fix some tests

Some more comments:

1. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityDeleteActionDeriver.php
   @@ -0,0 +1,40 @@
   +    return $entity_type->hasLinkTemplate('delete-multiple-form');
   

   Genuine question (i.e. not sure, what the answer is): Should this also check hasFormClass('delete-multiple-confirm now?

2. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,309 @@
   +   * The selection, in the entity_id => langcodes format.
   

   This is now no longer true, I think, since it now includes the entity type ID, as well.

3. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,309 @@
   +  protected $entityDefinition;
   

   I think using $entityType would be more consistent with the rest of core.

4. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,309 @@
   +    if ($this->router->getRouteCollection()
   

   I think instead of checking the router explicitly, it should be sufficient to check $this->entityDefinition->hasLinkTemplate('collection')

5. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,309 @@
   +  public function buildForm(array $form, FormStateInterface $form_state, $entity_type_id = NULL) {
   ...
   +    foreach ($this->selection as $id => $selected_langcodes) {
   ...
   +      list($entity_type_id, $entity_id) = explode(':', $id);
   

   Hmm... so I'm still not sure the new way of storing all entities prefixed by entity type ID is correct. Again, if I simultaneously delete nodes and comments $this->selection will contain both, so this will still end up deleting both nodes and comments. I think what would make more sense is to do the prefixing at the collection level, i.e. $this->selection = $this->tempStore->get($this->currentUser->id() . ':' . $entity_type_id) (or similar...)

6. +++ b/core/modules/comment/comment.routing.yml
   @@ -62,6 +62,15 @@ comment.multiple_delete_confirm:
   +entity.comment.delete_multiple_form:
   ...
   +    _entity_delete_multiple_access: 'node'
   

   This should be "comment", not "node"

Regarding #41
2. Will leave that open because of #44
5. Not sure whats the best way to spilt that up
11. Fixed
12. Will leave that open because of #43

Regarding #50
1. I don't think so. The form class is only required for the route provider. For example, Node is not using the route provider, so there is no need to implement the form handler.
2. I changed the logic back as suggested in 5., so this should be valid again
3. Done
4. Done
5. Followed your suggestion
6. Done

looks like it needs a re-roll. Will attempt and post results.

1. +++ b/core/core.services.yml
   diff --git a/core/modules/node/src/Plugin/Action/DeleteNode.php b/core/lib/Drupal/Core/Action/Plugin/Action/DeleteAction.php
   similarity index 61%
   
   copy from core/modules/node/src/Plugin/Action/DeleteNode.php
   copy to core/lib/Drupal/Core/Action/Plugin/Action/DeleteAction.php
   

   When I run patch -p1 < ~/Desktop/2670730-51.patch this file is not created. If I preemptively create the file and run the patch again, the changes are not applied.

2. +++ b/core/modules/node/src/Plugin/Action/DeleteNode.php
   @@ -2,98 +2,33 @@
      /**
       * {@inheritdoc}
       */
   ...
   +  public function __construct($configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager, PrivateTempStoreFactory $temp_store_factory, AccountInterface $current_user) {
   

   Since this is inherited from DeleteAction.php we don't have to define the params for the annotation?

   Also, we dropped the type hint of array from the first parameter.

Locally, I was not able to create DeleteAction so I am pushing my patch without testing it locally.

Looks like I overcommitted (as both the patch and interdiff files are rather large). Here is the latest patch.

@robpowell use git apply my.patch then it works.

So hopefully I fixed the last failing test now

thanks for the heads up @chr.fritsch. can you weigh in on #54.2?

Regarding #54.2: The definitions will be first inherited from the base class and some keys will be added also from the deriver.

Correct, the array type hint should be added again.

This is really looking pretty nice now, thanks for all the effort! Some more comments while some of the comments above are still unresolved, as far as I can tell.

1. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityDeleteActionDeriver.php
   @@ -0,0 +1,40 @@
   +        $definition['label'] = sprintf('%s %s', $base_plugin_definition['action_label'], $entity_type->getSingularLabel());
   

   This is potentially problematic for translators, because translators cannot change the order of the words which might make sense in some languages. So while in general re-using the base label is a good thing, in this particular case I would propose the following: t('Delete @entity_type', ...). (I just checked and that's in fact the exact string that Entity API in contrib uses...)

2. +++ b/core/lib/Drupal/Core/Entity/EntityDeleteMultipleAccessCheck.php
   @@ -0,0 +1,86 @@
   +    if (!$this->requestStack->getCurrentRequest()->getSession()) {
   +      return AccessResult::neutral();
   +    }
   

   Wow, that's pretty bizarre, but unfortunately it's absolutely correct to include this check here.

3. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,294 @@
   +    return 'entity_delete_multiple_confirm';
   

   1. Usually our form IDs end with "_form"
   2. Would be nice to have "$entity_type_id . '_delete_multiple_confirm'" as the ID and then implement BaseFormIdInterface to provide the generic string as the base form ID.

Re #41.16: I couldn't find delete actions in d6 and d7. So, no need to migrate them 😃

Also fixed #60 and #61

So I think I covered everything now.

Seems like we missed the 8.5 window 😢 So I changed the CR and the deprecation messages.

Also made some minor adjustments.

1. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityDeleteActionDeriver.php
   @@ -0,0 +1,40 @@
   +        $definition['label'] = t('Delete @entity_type', ['@entity_type' => $entity_type->getSingularLabel()]);
   

   t()? I think \Drupal\Core\Action\Plugin\Action\Derivative\EntityActionDeriverBase should have string translation injected. Also we missed on the generic action issue:

   $definition['label'] = sprintf('%s %s', $base_plugin_definition['action_label'], $entity_type->getSingularLabel());
   

   This is in \Drupal\Core\Action\Plugin\Action\Derivative\EntityActionDeriverBase. I'm not sure it is correct. It is assuming word order.

2. +++ b/core/lib/Drupal/Core/Entity/EntityDeleteMultipleAccessCheck.php
   @@ -0,0 +1,86 @@
   +   *   Allowed or forbidden.
   

   The access result. Since neutral is also returned.

3. +++ b/core/lib/Drupal/Core/Entity/EntityDeleteMultipleAccessCheck.php
   @@ -0,0 +1,86 @@
   +      // he will be allowed to access the delete form. On
   

   Unnecessary use of he. Could rephrase as

         // As long as the user has access to delete one entity allow access to the
         // delete form. Access will be checked again in
         // Drupal\Core\Entity\Form\DeleteMultipleForm::submit() in case it has
         // changed in the meantime.
   

4. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,304 @@
   +    // ::buildForm was still not called before, so we have to get the entity
   +    // type ID from the route.
   

   It's best to say what you are doing and then why. Something like:

   Get entity type ID from the route because ::buildForm has not yet been called.</code.
   </li>
   
   <li>
   <code>
   +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,304 @@
   +          // If we have a translated entity we build a nested list of
   +          // translations that will be deleted.
   

   Build a nested list of translations that will be deleted if the entity has multiple translations.

5. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,304 @@
   +      $this->messenger->addWarning($this->formatPlural($total_count, "@count @item couldn't be deleted, because you don't have the permission to do that.", "@count @items couldn't be deleted, because you don't have the permission to do that.", [
   

   I think we should update the text to
   @count @item has not been deleted because you don't have the necessary permissions. Also as far as I can see this is not tested yet.

I adjusted all the things from #65

About 1: These generic translations are really hard. I talked to @Berdir and we agreed that changing the label in EntityActionDeriverBase should be done in a follow-up.

Damn it, forgot to fix the tests 😞

OK, read through the patch again and it looks fine to me.

I opened #2938844: Provide a more meaningful count for entity delete multiple confirm forms for #41.5 and #2938841: Avoid string concatenation of translatable string in EntityActionDeriverBaser for #66. Didn't mark those posponed as we could just as well use those to fix the current node and comment-specific implementations, but don't feel strongly about that.

So, nothing left to complain -> RTBC

Thanks for all the hard work!!

1. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriverBase.php
   @@ -20,21 +22,34 @@
   -  public function __construct(EntityTypeManagerInterface $entity_type_manager) {
   +  public function __construct(EntityTypeManagerInterface $entity_type_manager, TranslationInterface $string_translation) {
        $this->entityTypeManager = $entity_type_manager;
   +    $this->stringTranslation = $string_translation;
      }
   
   +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityDeleteActionDeriver.php
   @@ -0,0 +1,40 @@
   +        $definition['label'] = $this->stringTranslation->translate('Delete @entity_type', ['@entity_type' => $entity_type->getSingularLabel()]);
   

   We need to also use the trait \Drupal\Core\StringTranslation\StringTranslationTrait because the l.d.o can't find $this->stringTranslation->translate(). Once you use the trait you can use $this->t() and the translation can be found.

2. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
   @@ -0,0 +1,304 @@
   +    if ($inaccessible_entities) {
   +      $this->messenger->addWarning($this->formatPlural($total_count, "@count @item has not been deleted because you don't have the necessary permissions.", "@count @item has not been deleted because you don't have the necessary permissions.", [
   +        '@item' => $this->entityType->getSingularLabel(),
   +        '@items' => $this->entityType->getPluralLabel(),
   +      ]));
   +    }
   

   I think this needs testing.

Good that you asked for tests. The logic was still broken 😔

So I fixed both.

+++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
@@ -292,7 +292,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+      $this->messenger->addWarning($this->formatPlural(count($inaccessible_entities), "@count @item has not been deleted because you don't have the necessary permissions.", "@count @item has not been deleted because you don't have the necessary permissions.", [

Missed this earlier, but the plural string is actually wrong: It uses "... @item has ..." but that should be "... @items have ..." unless I'm mistaken.

I guess since we're now actually testing the singular case, might be reasonable to test the plural form?

Updating issue credit for reviews and providing the contrib code (@bojanz and @dawehner according to @chr.fritsch).

Nice catch @tstoeckler.

I added another test for untranslatable entities and cover #72 in that.

Also, that's another translation problem. As discussed in the issue that added singular/plural labels, those labels must not be used inside a text with a specifc quantity, it only works as a standalone label.

The probem is languages like russian which have 6 or so different plural versions.. it's impossible to correctly translate it for those cases (e.g. 3 needs a different term than 0 or 10).

Besides, lower/uppercase is also again a problem, in german you need "Inhalte", uppercase and in english you'd want "content" lowercase.

IMHO, I would just use "items" literally, like "@count items have been deleted" and forget about per-entity-type specific labels. If we want to keep the labels of the existing forms, then move those messages in helper functions and add subclasses for Node/Comment that keep the existing naming (for strings that actually

About the test: Interesting I was confused why you can test so easily (because in a real scenario, the action plugin will never add those entities to the tempstore in the first place, the only way to test it there would be to make a permission/access change between executing the operation and submitting the confirm form.

Re #76: I don't mind using more generic messages in this case, not a bad idea. Just to make sure, though: We do have $entity_type->getCountLabel($count) for language-specific counts. It does a format-plural internally, so I always thought that shouldn't be used as part of larger translated strings, but it seems you have more experience in this area, so wanted to bring it up at least.

+++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
@@ -0,0 +1,304 @@
+      $this->messenger->addWarning($this->formatPlural(count($inaccessible_entities), "@count @item has not been deleted because you don't have the necessary permissions.", "@count @items has not been deleted because you don't have the necessary permissions.", [

Two more nitpicks:
"don't" -> "do not"

We don't use contractions in user-facing code (no pun intended). That will also get rid of the ugly escaping of the strings in the test.

In the plural form:
"has" -> "have"

Based on what @Berdir suggested in #76, I updated the patch.

@Berdir pointed out on Slack that it is not allowed to pass translated strings into the logger. So we decided instead of passing a non-helpful message about how much items have been deleted, better pass a logger notice for every deleted item. Then we also don't have problems with plural messages there.

+++ b/core/lib/Drupal/Core/Entity/Form/DeleteMultipleForm.php
@@ -256,25 +256,29 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+      foreach ($delete_entities as $entity) {
+        $this->logger($entity->getEntityType()->getProvider())->notice('The @entity-type %label has been deleted.', [
...
       $storage->delete($delete_entities);
...
         foreach ($translations as $translation) {
+          $this->logger($entity->getEntityType()->getProvider())->notice('The @entity-type %label @language translation has been deleted.', [
...
           $entity->removeTranslation($translation->language()->getId());
...
         $entity->save();

In both cases the logging should happen after the ->delete()/->save() call.

Sure

As far as I can tell @alexpott's and @Berdir's points have been addressed.

Read through the patch again and my complain-o-meter didn't go off.

Back to RTBC.

I reviewed all the new code since my last review. Looks great. The only thing holding this back from a commit is a slightly incomplete change record. I think the change record should detail what a contrib entity type would need to do to benefit from this work.

CR adjusted: https://www.drupal.org/node/2934349

Thanks for updating the change record @chr.fritsch.

Committed 3d55922 and pushed to 8.6.x. Thanks!

diff --git a/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriverBase.php b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriverBase.php
index 3db02b229d..7a410d6cc9 100644
--- a/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriverBase.php
+++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityActionDeriverBase.php
@@ -8,7 +8,6 @@
 use Drupal\Core\Entity\EntityTypeManagerInterface;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
 use Drupal\Core\StringTranslation\TranslationInterface;
-use Drupal\Core\StringTranslation\TranslationManager;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 
 /**

Removed unused use on commit.

This is failing hard on PHP 5 environments:
https://www.drupal.org/pift-ci-job/870739
https://www.drupal.org/pift-ci-job/871059

Drupal\Core\Action\Plugin\Action\Derivative\EntityActionDeriverBase and Drupal\Core\StringTranslation\StringTranslationTrait define the same property ($stringTranslation) in the composition of Drupal\Core\Action\Plugin\Action\Derivative\EntityActionDeriverBase. This might be incompatible, to improve maintainability consider using accessor methods in traits instead. Class was composed

These test failures are also reported in #2939903: Duplicate definition of StringTranslationTrait

Oh damn it. Let's see if this would fix it.

Note: I think its always better to open up a new issue and link it from the old one.

Yeah, also saw this with contrib module test fails that still use 5.5 for daily tests. +1 on a new issue.

Here is the new issue: https://www.drupal.org/project/drupal/issues/2939991

I'm setting this back to fixed.

Hi @chr.fritsch, I found unwanted effects of this path in configuration entity system while importing configurations, and I write a patch in the issue https://www.drupal.org/project/drupal/issues/2999737 about it.

The problem is that "delete node" and "delete comment" plugins don't set a type, because was removed in your patch. Was a mistake or was a requirement?

Thanks :)

@psf_ I have faced similar problem, what I did in my case I have manually edited configuration file system.action.node_delete_action.yml

and replaced

plugin: node_delete_action

with

plugin: 'entity:delete_action:node'

I think we are missing an update hook which will reset it

Hi I executed the below on devel

$config_factory = \Drupal::configFactory();
$config = $config_factory->getEditable('system.action.node_delete_action');
$config->set('plugin', 'comment_delete_action');
$config->save();

$config_factory = \Drupal::configFactory();
$config = $config_factory->getEditable('system.action.comment_delete_action');
$config->set('plugin', 'comment_delete_action');
$config->save();

I hope this helps

thank you @LOBsTerr. your suggestion fixed the delete action for me. I believe after an update the delete action plugin id changed but I had it already exported in the system.action.node_delete_action.yml under config/sync. Changing it to plugin: 'entity:delete_action:node' did the trick.

Thanks, @LOBsTerr, indeed that was the issue I was facing, for some reason got config validation import errors on drush cim -y, I've had to create an update.

function MODULENAME_update_8600() {
  $config_factory = \Drupal::configFactory();
  $config = $config_factory->getEditable('system.action.node_delete_action');
  $config->set('plugin', 'entity:delete_action:node');
  $config->save();
}

Hi guys, this could be causing what I stated in this thread
https://www.drupal.org/project/drupal/issues/3045570#comment-13088526

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "entity:delete_action:node" plugin does not exist. in                                                                       [error]

Caused when upgrading Drupal in following function:
system_post_update_change_delete_action_plugins