Provide bulk action to delete all old revisions of an entity

I will work on this on Monday, with a generic approach for all revisionable entities

Then maybe we should move this to the entity subsystem? :)

P.S. I mean this might need to be an operation offered by the storage.

Earlier common actions are present in the Drupal\Core\Action The node, comment specific actions are present in their specific modules.

Adding GDPR-tag as leaving behind old revisions compromise possibility to follow up privacy legislation.

This feature is also needed for translations, so when deleting a translation it should also be possible to bulk delete old translation revisions #2815779: Deleting a translation leaves behind orphaned revisions

Adding GDPR-tag as leaving behind old revisions compromise possibility to follow up privacy legislation.

How come? Can you please elaborate?

How come? Can you please elaborate?

If you're obliged to delete privacy data and that data is kept in revisions, it is illegal and a violate GDPR. GDPR doesn't only ask you to "unpublish" data. If you do not have the right to keep data you need to irrevocably delete it.

Should have a patch ready for this on Monday (and yep, it uses batch)

Added relation in #89181: Use queue API for node and comment, user, node multiple deletes
Because there some ui thought discussed

Ok this is green and ready for review folks 🎉

Whoops

Just an initial review, I'm sure some entity API/translation gurus might be able to provide a better review of the really nitty-gritty parts of this, but overall this patch looks awesome.

1. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/DeleteRevisionsAction.php
   @@ -0,0 +1,61 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function execute($object = NULL) {
   +    $this->executeMultiple([$object]);
   +  }
   

   Looks like DeleteAction already does this.

2. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/Derivative/EntityDeleteRevisionActionDeriver.php
   @@ -0,0 +1,91 @@
   +          $permission = "delete all revisions";
   

   Hm, seems like the node permissions are named pretty generically. Would probably be worth a follow-up, blocked by #2924785: Provide a mechanism to deprecate permissions.

3. +++ b/core/lib/Drupal/Core/Entity/EntityDeleteRevisionMultipleAccessCheck.php
   @@ -0,0 +1,36 @@
   +class EntityDeleteRevisionMultipleAccessCheck extends EntityDeleteMultipleAccessCheck {
   

   \Drupal\node\Access\NodeRevisionAccessCheck::checkAccess implies you need both the permission AND access to the 'delete' op, so extending this seems to make sense.

4. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteRevisionMultipleForm.php
   @@ -0,0 +1,499 @@
   +      '#description' => $this->t('Enter the number of revisions to keep, note that the current revision and any forward revisions (e.g. drafts) will always be retained.'),
   ...
   +      '#description' => $this->t('Delete revisions older than this date, note that the current revision and any forward revisions (e.g. drafts) will always be retained.'),
   

   "Pending revisions" is the lingo since #2890364: Replace all uses of "forward revision" with "pending revision".

5. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteRevisionMultipleForm.php
   @@ -0,0 +1,499 @@
   +    if ($strategy === self::RETAIN) {
   +      $newer = count($newer);
   +      // There are forward revisions already.
   +      $retain -= $newer;
   +      // And the current revision.
   +      $retain--;
   +      if ($retain) {
   +        // We need to retain additional revision beyond the current and forward
   +        // revisions.
   +        $revisions = array_slice($revisions, 0, count($revisions) - $retain);
   +      }
   +    }
   

   Finding this hard to parse. What I'm reading is: if there are pending revisions, we'll retain LESS revisions, but I would expect us to retain more?

6. +++ b/core/modules/node/tests/src/Functional/Update/NodeUpdateTest.php
   @@ -27,7 +28,7 @@ protected function setDatabaseDumpFiles() {
   -  public function testPublishedEntityKey() {
   +  public function stestPublishedEntityKey() {
   
   @@ -45,7 +46,7 @@ public function testPublishedEntityKey() {
   -  public function testStatusCheckbox() {
   +  public function stestStatusCheckbox() {
   
   @@ -70,7 +71,7 @@ public function testStatusCheckbox() {
   -  public function testOwnerEntityKey() {
   +  public function stestOwnerEntityKey() {
   

   let over debug

7. +++ b/core/modules/node/tests/src/Functional/Update/NodeUpdateTest.php
   @@ -83,4 +84,19 @@ public function testOwnerEntityKey() {
   +  /**
   +   * Tests that the bulk revision delete action is created.
   +   *
   +   * @see node_post_update_create_node_revision_delete_action()
   +   */
   +  public function testBulkRevisionDeleteAction() {
   +    $this->assertFalse(Action::load('node_revision_delete'));
   +
   +    // Run updates.
   +    $this->runUpdates();
   +
   +    $this->assertEquals('node', Action::load('node_revision_delete')->getType());
   +  }
   

   These tests are kind of expensive, updating from drupal-8-rc1. I wonder if the doTest... pattern might work here.

The update hook in this patch causes a circular dependency with action_permissions module... 🤕 (sub-module for vbo)

1. fixed
2. #3045358: [PP-1] Node module revision permission IDs are too generic
3. 👍
4. fixed
5. so we're working out how many we need to retain, but the list of revisions already excludes any pending revisions and the current one, so we need to reduce the number by those
6. fixed
7. #3045360: \Drupal\Tests\node\Functional\Update\NodeUpdateTest is expensive - refactor

Also refactored to prevent the action_permissions loop.

Note that this isn't compatible with views_bulk_operations because it doesn't support plugins that use their own tempstore (it ships its own delete plugins already).

You can get around this with some alter hooks to sub-class the views field plugin VBO provides and make sure to set the temp-store that is expected.

1. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteRevisionMultipleForm.php
   @@ -0,0 +1,499 @@
   +      '@item' => $context['sandbox']['totals'][$entity->id()] - count($context['sandbox']['revision_ids'][$entity->id()]) + 1,
   

   the +1 isn't needed here

2. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteRevisionMultipleForm.php
   @@ -0,0 +1,499 @@
   +      '@percentage' => (int) $context['finished'] * 100,
   

   casting this to an int happens before the *100, so we get 0 or 100% only

Here is a partial review only, as I don't have that much time at the moment.

1. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/DeleteRevisionsAction.php
   @@ -0,0 +1,54 @@
   +    $this->tempStore = $temp_store_factory->get(self::TEMPSTORE_KEY);
   

   Can we move this to the parent and define the constant there as well? In this case we would not need to override the constructor here.

2. +++ b/core/lib/Drupal/Core/Action/Plugin/Action/DeleteRevisionsAction.php
   @@ -0,0 +1,54 @@
   +  public function executeMultiple(array $entities) {
   +    /** @var \Drupal\Core\Entity\EntityInterface[] $entities */
   +    $selection = [];
   +    foreach ($entities as $entity) {
   +      $langcode = $entity->language()->getId();
   +      $selection[$entity->id()][$langcode] = $langcode;
   +    }
   +    $this->tempStore->set($this->currentUser->id() . ':' . $this->getPluginDefinition()['type'], $selection);
   

   Isn't this implementation the same as in the parent?

3. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteRevisionMultipleForm.php
   @@ -0,0 +1,499 @@
   +  public static function deleteRevisions(EntityInterface $entity, $current_revision_id, $strategy, $retain, $older_than, array &$context) {
   

   The $entity type hint should be RevisionableInterface.

4. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteRevisionMultipleForm.php
   @@ -0,0 +1,499 @@
   +    $current_revision_ids = array_map(function (RevisionableInterface $entity) {
   +      return $entity->getLoadedRevisionId();
   +    }, $entities);
   ...
   +            $batch['operations'][] = [[static::class, 'deleteRevisions'], [$entity, $current_revision_ids[$id], $form_state->getValue('strategy'), $form_state->getValue(self::RETAIN), $form_state->getValue(self::OLDER_THAN)]];
   ...
   +          $batch['operations'][] = [[static::class, 'deleteRevisions'], [$entity, $current_revision_ids[$id], $form_state->getValue('strategy'), $form_state->getValue(self::RETAIN), $form_state->getValue(self::OLDER_THAN)]];
   ...
   +          $batch['operations'][] = [[static::class, 'deleteTranslations'], [$entity, $current_revision_ids[$id], $form_state->getValue('strategy'), $form_state->getValue(self::RETAIN), $form_state->getValue(self::OLDER_THAN)]];
   

   I am not sure if I understand this correctly, but it looks like $entity->getLoadedRevisionId() is equal to $current_revision_ids[$id]. Why do we have to pass it then as an additional parameter when we could retrieve it from the entity object?

5. +++ b/core/lib/Drupal/Core/Entity/Form/DeleteRevisionMultipleForm.php
   @@ -0,0 +1,499 @@
   +        if ($entity instanceof TranslatableInterface) {
   

   All content entities implement TranslatableInterface, but not all are translatable.

   The translatability should be checked through the entity type instead:
   $entity->getEntityType()->isTranslatable();

I read the patch and have no concerns. I like how `providerRevisionMultipleDeleteStrategy` provides a set of scenarios to test.

I did a blind-test without first looking at the screenshots.

1. I found it a little jarring that the first radio option ("Retain at least N revisions"), and the textfield ("Retain at least [ ]") were visually separated (see 2925532-visually-separated-options.png).

2. I feel like if we are deleting revisions, lets just delete all revisions. Which also makes the initial form much easy to parse (see 2925532-simple-first-option.png).

3. In the previous screenshot I've also speculatively renamed "Retain at least N revisions" to "Delete all but a few recent revisions" which I *suspect* might stand up better to UX testing... since most people don't know what "N" is.

1. I found it a little jarring that the first radio option ("Retain at least N revisions"), and the textfield ("Retain at least [ ]") were visually separated (see 2925532-visually-separated-options.png).

That seems to me like JS hadn't loaded, states should hide the date option like in the screenshots in the OP - simplytest.me seems to be slow to load JS.

I like the option of 'delete full history', but will hold off for review from @benjifisher and UX team before implementing.

Still need to address feedback from @hchonov

That seems to me like JS hadn't loaded, states should hide the date option like in the screenshots in the OP - simplytest.me seems to be slow to load JS.

Yes it did hide the date option as you say it should. How can I explain it ... I think that the question Retain revisions [ 3 ] it could be great if it was inline with the radio button (or visually really close to it). It just feels like it's a completely new question, so it broke my mental flow and I had to take a step back, read the whole form, and then connect the dots, if that makes sense.

Rant

Before I start the usability review for this issue, I have to mention how many things are already wrong with the select list of available actions.

• How is it ordered? It is neither alphabetical nor does it group Publish/Unpublish and so on.
• Maybe we should combine Publish/Unpublish into a single, configurable action. Same for most of the other actions.
• What does "Save" do? I peeked at the code: it sets the "changed" timestamp and saves. I guess this triggers any node_save hooks; I do not see how else it is useful.

Usability review

I think that @sime is saying is that the relation between the choice of method and the configuration of that method is not clear. I do not think we can fix that by breaking up the Method radio buttons with the configuration options. How about separate Method and Configuration sections? Something like this:

  DELETION STRATEGY

  The current revision and any pending revisions (e.g. drafts) will always be retained.
  The choices below affect which older revisions will be kept.

  **Method**

  x Delete all older revisions.
  o Keep a fixed number of older revisions.
  o Delete revisions older than a specific date/time.

  **Configuration**

  x There is no configuration for this option.
  o Keep ___ older revisions.
  o Delete revisions older than MM/DD/YYYY HH:mm:ss AM.

1. I have made specific suggestions, but there is room for more wordsmithing.
2. I have included the extra option suggested in #35.
3. Use states, as in the current patch, so that only one option under Configuration is visible.
4. I have moved the repeated part of the help text to a #description element on the wrapping element, and I have expanded it.
5. I have removed the rest of the help text. It seems to me that "Enter the number of revisions to keep" and "Delete revisions older than this date" do not add anything useful.
6. Maybe I am just old-fashioned, but it bothers me that the current help text is written as a single sentence with a comma. If we were keeping the current help text, then I would ask to start a new sentence with "The current revision" (and drop "note that").
7. Based on #2875154: Clarify and document wording around drafts, revisions, published & friends and #1643354: [Policy, No patch] Overhaul the entity revision terminology in UI text, maybe we should drop "(e.g. drafts)".
8. I do not love my own suggestion for Delete/Keep/Delete. Maybe we should be more consistent. Maybe the title should be more specific than "deletion strategy" and the options under "Method" should be shorter.
9. Why use a details form element? Will the user ever want to collapse this section? I do like the placement of the #description element for a details element.

Where else do we have something like this, where the choice of one option leads to further configuration?

One example is adding a field to a node. Once you choose the field type, you save and continue to configuration on a new page. I think that is more than we want to do here.

Another example is adding a contextual filter to a view. Under "When the filter value is NOT in the URL" there are 6 radio buttons. Two of them, when selected, expose further configuration directly underneath (before the next radio button). This is exactly the pattern I rejected. Maybe I will be overruled, but I think that is a terrible UX. Besides, "advanced" views config is not expected to be as user-friendly as /admin/content.

P.S. @larowlan, notice that the issue is still assigned to you.

Maybe we should combine Publish/Unpublish into a single, configurable action. Same for most of the other actions.

100% agree, and this has bugged me for years - but the current patch didn't create this problem. Is it possible to look at usability of bulk actions as a separate issue?

Is it possible to look at usability of bulk actions as a separate issue?

Yeah that is 100% out of scope

Will re-roll for those UX changes and changes from @hchonov review this week

Sorry, I meant to say that the "rant" section was out of scope. Maybe there are already issues for some of those problems. I have not looked, yet.

I like the idea but would like to see this as a service or similar that modules like https://www.drupal.org/project/node_revisions_autoclean could build upon.

My specific use case would be to automatically clean up revisions older than 'X' days, or N revisions, but outside of the user path for performance reasons (cron, queue, etc). I know this change doesn't directly support the above, but if I could use the same infrastructure, that would be better.

So we discussed this on UX meeting today...

There was general acknowledgement about the "disconnect" problem between the radio options and configuration "somewhere down below." The way to handle it would be the way Contextual Filters works, and "inline" the config forms next to the radio options. However...

1) There was general confusion about why we even offer all of these options, esp. the "retain at least N revisions" one, since that's a) entirely arbitrary, b) likely to vary significantly on a per-node basis, and thus makes less sense in a bulk actions context.

2) If we are going to do date-based deletion, it is better UX to say "3 days ago", "1 week ago," "30 days ago," "1 year ago," etc. The current UX requires the user to manually compare the publish date of the node (which is on a different page) and this one.

3) However, since core aims toward "80% use cases," the 80% use case here seems like it's simply to "delete all old revisions" (where "old" means anything that's not the current / any pending revisions). Which doesn't require specifying any configuration settings.

4) And, it seems like this functionality is actually more useful to do on a per-node basis (like a "Delete all old revisions" button at node/X/revisions) vs. bulk updates... the two use cases we could think of were "I worked for a long time on this blog post and it's finally done, so let's clear out old crap" and "oh sh*t I accidentally had content in that node I didn't want", both of which are well-served by targeting one specific node vs. a collection of them.

5) In the case where "we have a large site where we don't want to keep old revisions around all the time," and you want to enable a site-wide policy, such as "delete all revisions older than 30 days" then bulk operations once again aren't the best way to go about this; a better way would be a cron-based solution, akin to what https://www.drupal.org/project/node_revisions_autoclean seems to do. (From reading the description.)

TL;DR: This feels like a more nuanced problem than core should be digging into; can we choose a simpler, not-bulk-updates version of this—such as a "Delete all old revisions" button on the node revisions page or "bulk delete all revisions across all [type] nodes after [y timeframe]' across the entire site?

(Note: we might be missing some GDPR nuances and/or other specific use cases, but didn't see that outlined in the issue summary. Happy to hear counter-arguments.)

Crediting folks from the UX meeting who were part of the discussion.

Here is the recording of the Drupal usability meeting 2019-04-16 on YouTube.

Just realised the title is wrong here.

Whilst the patch introduces the behaviour for nodes and some entity-types, what's not obvious is that the current functionality would also work for Media, Terms, Block Content and Menu links (which also support revisions) once we resolve #3043321: Use generic access API for node and media revision UI

So, from here I'll do the following

- move the functionality into a generic service instead of an action plugin
- implement background cleanup
- put a button on the node-revisions page that can invoke it for a single node

Just want to make sure that UX team is happy if this means it won't 'just work' for media/terms/etc when #3043321: Use generic access API for node and media revision UI is resolved, but might if #2350939: Implement a generic revision UI includes said button for all entity-types

#51 sounds good to me, I think Node is the only Core Entity Type which currently has revision routes, so it makes sense to implement the Action for Nodes, then implement it generically with #3043321: Use generic access API for node and media revision UI and #2350939: Implement a generic revision UI.

I was asked to chime back in here...

For most of these purported use cases, I think you want one just option: "delete all old revisions" (not further specifying "but keep N revisions around" or "since timeframe"). That would be fine, as it doesn't introduce further UX challenges (beyond hoping they don't click quickly past the confirmation form since the amount of data being deleted could be significant, and could harm a site's compliance with data retention policies).

What specifically didn't seem to make sense (to me, at least) was a set of checkboxes for all revisions across all entity types, and picking + choosing from that enormous list for potential targets. And/or doing a bulk "select all of these" and then making further refinements to the deletions that would happen, as the impact of those decisions is way too large. It seems like those further refinements would instead be very nuanced, e.g. delete revisions only for content of this type, only for node 75 (the dude using the Save button as Preview). Or, in the case where you want a site-wide revision retention policy, that's better enacted at admin/settings/foo, vs. done manually by checking a bunch of boxes every so often.

However, I think I got credited for caring about this issue a lot more than I actually do; I was just giving an opinion. :) It's totally fine to voice counter-opinions!

+++ b/core/lib/Drupal/Core/Entity/Form/DeleteRevisionMultipleForm.php
@@ -0,0 +1,499 @@
+  public static function deleteRevisions(EntityInterface $entity, $current_revision_id, $strategy, $retain, $older_than, array &$context) {
+    $instance = static::factory();
...
+  public static function deleteTranslations(EntityInterface $entity, $current_revision_id, $strategy, $retain, $older_than, array &$context) {
+    $instance = static::factory();
...
+  public static function done($success, array $results, array $operations) {
+    $instance = static::factory();

Is there any reason we can't make these non-static? Would save having to create the $instance every time.

Asking because I'm currently building a custom batch which uses non static methods and it works perfectly. You just have to pass $this into the operation rather than the class name.

mainly to avoid serializing $this

Here's a quick reroll of #30 and addresses 2. and 3. from #33. 1. 3. and 5. from #33 still need to be addressed.

Note this is based on 8.8 dev.

2925532-59.patch failed to patch 8.9.11.

Fixed some lint-issues

I used a few hours to have Drupal up and running with this patch, just find out that any "Bulk Delete revisions" action does not seem to appear in my installation.
Perhaps abandoned project?

According to the patch, this looks like it should be postponed on #3043321: Use generic access API for node and media revision UI, although leaving it as a "soft-postpone" as I don't think it's a hard-blocker, and that issue has now settled on the specific operations that will be used.

I do however wonder if we should hard-block this issue on #2350939: Implement a generic revision UI, since it would make sense for the work here to be aligned with the work done on that issue.

I guess this is "PP-2" then.

#3043321: Use generic access API for node and media revision UI landed

#2350939: Implement a generic revision UI is fixed.