Add support for "Maximum number allowed" revisions per content node translation

Thanks for the issue and for use the Issue Summary Template.

As I say in the release

" This version has not been tested in multilingual sites."

I know this is important. But at this time I concentrating all the efforts finishing the test for the actual code. Seeing we will need to refactor at some point the getCandidates method I will exclude these methods from the Unit tests for the moment.

If you have time to help me with this will be great.

When you create a new revision in a multilingual environment Drupal create a new revision for all the other configured language (here is where the module fails as it doesn't consider this behavior for the moment). If you have a content type configured to be multilingual and you have English (EN) and Spanish (ES). When you edit a node in EN a new revision is created for ES even if you don't change anything in the ES language. I don't know if is here where the database field revision_translation_affected can help us.

@jasonawant Is better if you can split the patch, I prefer to review small patches. Thanks.

Patch from #15 looked pretty good when testing it out, but minimum age to delete doesn't seem to get respected, tried adding this but not quite right.

diff --git a/node_revision_delete/src/NodeRevisionDelete.php b/node_revision_delete/src/NodeRevisionDelete.php
index d8a428677c..02b0a01857 100644
--- a/node_revision_delete/src/NodeRevisionDelete.php
+++ b/node_revision_delete/src/NodeRevisionDelete.php
@@ -345,6 +345,8 @@ class NodeRevisionDelete implements NodeRevisionDeleteInterface {
           $query->condition('nfr.nid', $candidate_node, '=');
           // Exclude revisions that are newer than the current revision.
           $query->condition('nfr.changed', $subquery, '<');
+          $query->join('node_revision', 'nr', 'nfr.vid = nr.vid');
+          $query->condition('nr.revision_timestamp', $content_type_config['minimum_age_to_delete'], '<');
           $query->isNotNull('nfr.revision_translation_affected');
           $query->groupBy('nfr.nid');
           $query->groupBy('nfr.vid');
@@ -400,6 +402,8 @@ class NodeRevisionDelete implements NodeRevisionDeleteInterface {
         $query->condition('nfr.changed', $content_type_config['when_to_delete'], '<');
         // Exclude revisions that are newer than the current revision.
         $query->condition('nfr.changed', $subquery, '<');
+        $query->join('node_revision', 'nr', 'nfr.vid = nr.vid');
+        $query->condition('nr.revision_timestamp', $content_type_config['minimum_age_to_delete'], '<');
         $query->isNotNull('nfr.revision_translation_affected');
         $query->groupBy('nfr.nid');
         $query->groupBy('nfr.vid');

I updated the patch from @jasonawant since it stopped applying after rc1. It seems like some of the changes in the patch was committed. Also, I updated it a bit to not change getCandidatesRevisionsByNids to getCandidatesRevisionsByNid.

It is confirmed that patch # 17 does not respect the "minimum age" setting.

Here is updated patch to fix minimum age consideration.

My bad, I name diff file extention to .patch, I also raised MR, for easier review.

Increasing priority.

I can confirm patch from #20 solves the issue from issue summary and code looks good, but I'd like to see tests included to avoid any regression on future releases.

1. +++ b/src/NodeRevisionDelete.php
   @@ -326,36 +326,41 @@ class NodeRevisionDelete implements NodeRevisionDeleteInterface {
   -        if ($this->configFactory->get($this->configurationFileName)->get('delete_newer')) {
   

   I don't see where is handled here this configuration 'delete_newer'

2. +++ b/src/NodeRevisionDelete.php
   @@ -372,41 +377,45 @@ class NodeRevisionDelete implements NodeRevisionDeleteInterface {
   -      if ($this->configFactory->get($this->configurationFileName)->get('delete_newer')) {
   

   And the 'delete_newer' configuration is not considered here either.

Confirmed patch #20 fixes the issue for me.

The patch maintains the latest revisions for all languages regardless of which language each revision belongs to.

If "Minimum number of revisions to keep" is set to 5, it effectively keeps 5 revisions, but not the last 5 for each translation, only the last 5 in all languages.

The patch seems to need a reroll for the latest rc4.

I've rerolled the patch, to check that it works for the scenario provided in https://www.drupal.org/project/node_revision_delete/issues/3262899, but as was mentioned in
- #25 "delete_newer" option is missing,
- #27 states that patch doesn't work at all

On my opinion it should include scenario where editor re-save a node without any change: in this case translation_affected is not set at all, but revision itself is pretty much redundant(doesn't contain any changes).

This discussions could be closed or resolved only with a help of tests. Currently module ships with tons of different tests, but they don't cover module core feature: the deletion of the revisions and merging this issue into dev without even confirming that it doesn't break anything is a really bad idea.
I will try to find an issue for this kind of a test or open a new one.

Hereby a rerolled patch that should apply to the latest 1.0-rc4 release.

I also tested the claim in #27 that the patch does not work and for me, it works fine.
For example, if you configure it to keep a minimum of 3, it will keep a minimum of 3 per language!

Here is a first review of the patch.

1. +++ b/src/Form/AdminSettingsForm.php
   @@ -162,19 +162,26 @@ class AdminSettingsForm extends ConfigFormBase {
   +          $candidate_nodes = array_unique(array_merge($nodes, $candidate_nodes));
   

   Doing an array_merge() inside a foreach loop is not very efficient. Can we do the merge outside the loop using the spread operator?

   https://wiki.php.net/rfc/argument_unpacking

   I see this is also being done in other placed in the module, so I guess this could also be a separate followup.

2. +++ b/src/NodeRevisionDelete.php
   @@ -376,7 +382,8 @@ class NodeRevisionDelete implements NodeRevisionDeleteInterface {
   +    $nid = current($nids);
   +    $node = $this->entityTypeManager->getStorage('node')->load($nid);
   
   @@ -384,39 +391,41 @@ class NodeRevisionDelete implements NodeRevisionDeleteInterface {
   +      foreach ($this->languageManager->getLanguages() as $lang) {
   ...
   +        $query->condition('nfr.nid', $nid, '=');
   

   It seems this method now only retrieves the candidate revisions for a single nid (the first one), not for all the nids passed? Within the module I see all calls only pass a single node ID, but I guess we should either:
   - Change this method to only support a single NID (might be less complex and better for performance?)
   - Fix the query to fetch all candidate revisions for all passed node IDs.

3. +++ b/src/NodeRevisionDelete.php
   @@ -384,39 +391,41 @@ class NodeRevisionDelete implements NodeRevisionDeleteInterface {
   +        $subquery = $this->connection->select('node_field_data', 'nfd');
   +        $subquery->fields('nfd', ['changed']);
   ...
   +        $query = $this->connection->select('node_field_revision', 'nfr');
   +        $query->fields('nfr', ['vid']);
   

   The query is quite complex, it would help a lot to add a bit more documentation. For example, explain a bit about the background of why we need a subquery to fetch the changed date from the node_field_data table etc.

4. +++ b/src/NodeRevisionDelete.php
   @@ -384,39 +391,41 @@ class NodeRevisionDelete implements NodeRevisionDeleteInterface {
   +        // Exclude revisions that are newer than the current revision.
   +        $query->condition('nfr.changed', $subquery, '<');
   +        $query->isNotNull('nfr.revision_translation_affected');
   

   This seems to be my biggest concern for now. New draft translations don't add a record to the node_field_data table (#3088790: Creating a new translation draft is not populating node_field_data leading to inconsistent data display.). It seems the subqueries in getCandidatesRevisions() / getCandidatesRevisionsByNids() and getCandidatesNodes() might not work without that being solved. Without a record in node_field_data for example we are also not able to fetch a node type for a revision by vid and langcode.

----------

I would have to think about this a bit more, but reading the patch I'm starting to wonder if a highly complex query like this is really the most robust and maintainable way to move forward. I have a feeling it would help a lot to maybe start using the queue system.

We could have several ways to add node IDs / revision IDs to the queue, and let the queue try to figure out whether that specific revision is ready to be deleted or not. This might be less performant, but I think trying to do all checks in a single query has a bunch of limits that might be tricky to overcome as well. Anyway, please excuse this sidestep, that is definitely not in scope for this issue, and I'll think a bit more on whether to open a follow-up for this.

*edit: Opened #3300328: Simplify architecture to solve issue around content moderation, translations and cron as a followup.

The attached patch is a re-roll for 8.x-1.0-rc5 based on #31. Unfortunately not being able to produce interdiff.

The patch should resolve:
- #25.1 and #25.2 so delete_newer setting is respected.
- #33.2 by renaming getCandidatesRevisionsByNids to getCandidatesRevisionsByNid and adjusting the code accordingly.

And I fully agree with @seanB's other points raised on #33, but I'd hope they could be resolved on follow-up issues.

This patch should pass the CI tests, but this is still missing test coverage for multilingual.

Adjusted the patch to work with delete_newer option. This patch also includes test coverage. Interdiff doesn't contain the new test file.

Since this is quite complex issue to review, I would appreciate any manual tests with real content. I'll write the test coverage I used in manual steps:

1. Install the site with Standard profile drush site:install standard
2. Enable modules drush en language content_moderation content_translation node_revision_delete workflows -y
3. Add a new non-english language /admin/config/regional/language/add.
4. Enable workflow to Article content type /admin/config/workflow/workflows/manage/editorial, set "Default moderation state" as "Published".
5. Enable translations and select "Limit the number of revisions" and "Minimum number of revisions to keep" as 1 for article. /admin/structure/types/manage/article.
6. Add node #1, Edit it two times.
7. Add translation to #1, edit translation two times.
8. Add node #2. Edit it two times, both times set moderation state as "Draft"
9. Add node #3.
10. Add translation to node #3 and edit translation two times.
11. Visit /admin/config/content/node_revision_delete and expect "Candidate nodes" to be 2 and "Candidate revisions" to be 3.
12. Select "Delete revisions newer than the current revision" and submit the form.
13. Expect "Candidate nodes" to be 2 and "Candidate revisions" to be 4.

Is this working as expected or would it need some changes?

The merge request diff applies against latest rc6 but there is a message in the merge request that it needs a rebase against the main branch.

We just added a 2.x version of the module. This is a complete rewrite based on plugins to determine which revisions can be deleted. It also uses a queue worker to actually delete revisions, and takes multiple translations of content into account. Could you please test if this solves the issues you currently experience?

For me, the new version 2.x does not work. Previously, too many revisions were deleted. But now too few revisions are deleted. At least that's how it looks.
In the sandbox, the module works great at first sight. But in our system with two languages and 40k revisions, it doesn't seem to work. Too bad!

I confirm what @vincent.hoehn say: the 2.x version does not work at all, if I specify to keep only 1 revision, nothing happens.

For example, I have a node with 2860 revisions, none is deleted.

For those whose need the patch for -rc6.

@vincent.hoehn @B-Prod could you maybe try and debug what is happening in the 2.x version to see why the revisions are not being deleted? Are you getting errors in the logs? Are items being added to the queue? Is the queue being processed? Is this process running out of memory? Any additional info to help resolve the issues would be appreciated!

#42 looks like working with 1.0 rc-7.

I have now check and calculate revisions count per node and node translations after clicking delete revisions on node.

#42 has one code change is missing from #37,
File CandidateNodesForm.php has below mentioned change inside the #37 patch, but which is missing inside the #42 patch i have added that change and created new patch and other changes are same as #42 and tested with latest version of module 8.x-1.-rc7.

Closing because the current development is being made in branch 2.x