Delete draft revisions safely

Hi, thanks for the issue and for use the Issue Summary Template.

We can't delete the current revision of a node, and I can't get the point of what are you proposing here.

I think this made sense. And is easy to accomplish as we only need to add a condition to the query searching for the revision candidates.

Do you want to create the patch?

Adding related issue #3118464: Add support for "Maximum number allowed" revisions per content node translation

This has become a priority for our stakeholders. We will begin to outline a solution and plan to include it in future sprint work.

It sounds like @adriancid confirmed that Node Revision Delete should/could account for this and is accepting of patches!

So on to a solution. I'm not sure the UI needs another checkbox to indicate this an optional functionality. I recommend the query logic should account for this by default. Thoughts?

I will post again after I've had time to review logic and propose a solution.

@jasonawant I think we just need to check if the conditions are present and add the conditions to the query, as I think this is the logical behavior.

Thanks @adriancid!

Sounds like the OP was recommending protecting all revisions that were created after the current published revision. Protecting revisions by omitting matching records using node_revision.revision_timestamp > node.changed or node_field_revision.changed > node_field_data.changed? Is that your understanding?

Alright, here's a pass at different query logic to exclude future (draft, moderation state could be anything) revisions. I wanted to post now while I continue to prepare a patch for review. My patch will include changes for #3118464: Add support for "Maximum number allowed" revisions per content node translation.

This query logic would replace the logic found in NodeRevisionDelete::getCandidatesRevisionsByNids();

Current logic

    $sub_query = $this->connection->select('node_field_data', 'n');
    $sub_query->join('node_revision', 'r', 'r.nid = n.nid');
    $sub_query->fields('r', ['vid', 'revision_timestamp']);
    $sub_query->condition('n.nid', $nids, 'IN');
    $sub_query->condition('changed', $content_type_config['when_to_delete'], '<');
    $sub_query->where('n.vid <> r.vid');
    $sub_query->groupBy('n.nid');
    $sub_query->groupBy('r.vid');
    $sub_query->groupBy('revision_timestamp');
    $sub_query->orderBy('revision_timestamp', 'DESC');
    // We need to reduce in 1 because we don't want to count the default vid.
    // We excluded the default revision in the where call.
    $sub_query->range($content_type_config['minimum_revisions_to_keep'] - 1, PHP_INT_MAX);

    $query = $this->connection->select($sub_query, 't');
    $query->fields('t', ['vid']);
    $query->condition('revision_timestamp', $content_type_config['minimum_age_to_delete'], '<');

    $candidate_revisions = array_merge($candidate_revisions, $query->execute()->fetchCol());

Replacement

This approach does require ordering by change date and offsetting the range b/c it excludes the current and future revisions by joining on the node_field_data's nid and changed timestamp.

I'm a little unsure with having two conditions for when_to_delete and minimum_age_to_delete, seems redundant but probably doesn't hurt.

Note the langcode condition. This is new to account for #3118464: Add support for "Maximum number allowed" revisions per content node translation.

    $query = $database->select('node_field_revision', 'nfr');
    $query->fields('nfr', ['vid']);
    // Join node_field_data to only to exclude current revision and any future (draft)
    // revisions.
    $query->join('node_field_data', 'nfd', 'nfr.nid = nfd.nid AND nfr.changed < nfd.changed');
    $query->condition('nfr.langcode', $lang, '=');
    $query->condition('nfr.changed', $content_type_config['minimum_age_to_delete'], '<');
    $query->condition('nfr.changed', $content_type_config['when_to_delete'], '<');
    $query->condition('nfr.nid', $candidate_node, '=');
    $query->orderBy('nfr.changed', 'DESC');
    $query->range($content_type_config['minimum_revisions_to_keep'], PHP_INT_MAX);
    $results = $query->execute()->fetchCol();

Shoot, I guess can't scale b/c of I've made an assumption about looping over the $candidate_nodes, which would generate a query per node, see this condition in the above replacement, $query->condition('nfr.nid', $candidate_node, '=');

I'll take another look at this.

Well, here's a draft patch that also includes #3118464: Add support for "Maximum number allowed" revisions per content node translation work as well.

I've caught a few issues with previous patch.

Per #3118464-11: Add support for "Maximum number allowed" revisions per content node translation, here's a patch for this issue only that does not account for per content translations.

@jasonawant thanks, it is better for me always review small patches or patches for small tasks, as I don't have much time to do it and I contribute in my free time. I will check this in the next days and then we can continue with the other issue.

Hi,
I'm also experiencing issues related to drafts being deleted when using the content_moderation core module.
I haven't had the time to look into this, but wouldn't simply changing

$sub_query->where('n.vid <> r.vid');

to

$sub_query->where('n.vid > r.vid');

prevent drafts that are newer than the current revision from being deleted ?

@semiaddict yes, the change is simple, but the patch is trying to fix the problem with the language too (this is why is more complex), I just need to find time in these crazy virus days to take a look into the patch.

@semiaddict you're right, I mixed the issues, this is the only thing we need here.

@jasonawant the query you're trying to build doesn't work, I tested the query and I'm not getting all the revisions I should by getting concerning the issue description.

I'm uploading a patch to be tested and I think we will need to add in someplace a description about the module behavior and then I will commit the patch.

@adriancid,

That seems like a simply fix here! I probably over complicated this b/c I was coming from #3118464: Add support for "Maximum number allowed" revisions per content node translation. Hopefully, there's just as simple fix for that issue too.

Regarding,

the query you're trying to build doesn't work, I tested the query and I'm not getting all the revisions I should by getting concerning the issue description.

Which patch were you trying?

@jasonawant I'm talking about #14 this is the patch I was testing and it doesn't work, can you test the patch I submitted #18?

Thanks for the info!

tested, and works expected. Tested with patch, it preserves a draft revision. Tested w/out patch, draft revision is deleted.

Setting back to needs work

add in someplace a description about the module behavior and then I will commit the patch.

@jasonawant thanks, I will see where is the better place to explain this behavior, maybe I will need to write in many places (readme, help, and configs)

I am wondering if this behavior should be optional instead of hard-coded.
Some might still want drafts to be deleted if they exceed a certain number, thus the current behavior (before patch) could fit their need.
What do you think ?
I might be able to put in an hour or so next week to take care of this.

God point @semiaddict I will work on this, I think it is better like an option, in this case, I will turn this into a plan and I will add some child issues as we will need to add explanations about this in some places. Just keep an eye on the issue queue to help me reviewing the issues. You can change the notifications for the project in https://www.drupal.org/project/issues/subscribe-mail/node_revision_delet...

I think all the issues cover the new functionality, as we will need at some point to write all the help pages I think is better to wait until this moment to do it for this feature. For the moment I add a description in the configuration form.