Make WordPress Core

Opened 18 months ago

Closed 15 months ago

Last modified 15 months ago

#57320 closed enhancement (fixed)

Revisions: add filter before deleting revisions

Reported by: jhned's profile jhned Owned by: johnbillion's profile johnbillion
Milestone: 6.2 Priority: normal
Severity: trivial Version:
Component: Revisions Keywords: has-patch has-unit-tests needs-dev-note changes-requested commit
Focuses: administration Cc:

Description

I'm in a situation where I need to preserve the original post revision and prevent it from being deleted once a post is past the max number of revisions for the site. This is more difficult than it seemed, because WordPress doesn't have many filters around the revisions delete process.

If we add a new filter to the result of wp_get_post_revisions in revision.php, then I'd be able to slice out the revision that I don't want to get deleted and instead delete the next-oldest one.

Change History (22)

#1 @costdev
18 months ago

Hi @jhned, thanks for creating this ticket!

Would hooking the the_posts filter suit your needs? The first parameter is the list of posts (or revisions in this case), and the second is the query object.

I'm thinking something along the lines of this:

<?php

add_filter( 'the_posts', 'myprefix_skip_original_revision', 10, 2 );

function myprefix_skip_original_revision( $revisions, $query ) {
    if ( 0 < $query->get( 'post_parent' ) && 'revision' === $query->get( 'post_type' ) ) {
        array_shift( $revisions );
    }

    return $revisions;
}

#2 @jhned
18 months ago

Hi @costdev, thanks for the thought. I tried both the_posts and pre_get_posts in coming up with this idea. In pre_get_posts I was able to modify the query and set the "exclude" or "post_not__in" args, but the wp_get_post_revisions call still returned all the revisions. With the_posts, for some reason it wasn't hitting my breakpoint for the wp_get_post_revisions call; I tried your example as well and it's not intercepting the revision deletion process.

When using either pre_get_posts or the_posts, however, we'd have to do extra work to make sure that the query in question is one that's getting ready to delete some revisions after it gets them. Using pre_get_posts or the_posts wouldn't give us that context. We could try debug_backtrace, but that seems like overkill compared to adding a new hook.

#3 @costdev
18 months ago

Thanks for the confirmation @jhned! I'll submit a patch for testing now.

This ticket was mentioned in PR #3740 on WordPress/wordpress-develop by @costdev.


18 months ago
#4

  • Keywords has-patch has-unit-tests added

This PR introduces a new filter for wp_save_post_revision().

'wp_save_post_revision_revisions_before_deletion' passes the revisions to be considered for deletion, and the new revision's post ID.

This allows extenders to exclude specific revisions from being considered for deletion.

Trac ticket: https://core.trac.wordpress.org/ticket/57320

#5 @costdev
18 months ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 6.2

#6 @audrasjb
16 months ago

  • Keywords needs-testing removed
  • Owner set to audrasjb
  • Status changed from new to reviewing

I tested this filter using the following test snippet:

<?php
add_filter(
        'wp_save_post_revision_revisions_before_deletion',
        function( $revisions, $post_id ) {
                $original_revision = get_transient( 'original_revision_for_post_' . $post_id );
                if ( $original_revision ) {
                        // Always remove the oldest revision for the array of revisions to potentially delete.
                        unset( $revisions[ $original_revision ] );
                } else {
                        // Set the oldest revision in a transient, so we can verify that it is always ignored.
                        $original_revision = array_key_first( $revisions );
                        set_transient( 'original_revision_for_post_' . $post_id, $original_revision );
                }
                return $revisions;
        },
        10,
        2
);

I also set WP_POST_REVISIONS constant to 3.

Test report:
Works fine on my side: the oldest revision is never deleted ✅

This ticket was mentioned in Slack in #core by costdev. View the logs.


16 months ago

#8 @adamsilverstein
16 months ago

Filter and tests look good to me!

#9 @costdev
16 months ago

  • Keywords commit added

Adding for commit consideration 🙂

#10 @audrasjb
16 months ago

  • Keywords needs-dev-note added

#11 @audrasjb
16 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 55254:

Revisions: Add a way to filter the revisions considered for deletion.

This changeset introduces a new filter for wp_save_post_revision(). wp_save_post_revision_revisions_before_deletion passes the revisions to be considered for deletion, and the new revision's post ID.

This allows extenders to exclude specific revisions from being considered for deletion.

Props jhned, costdev, audrasjb, adamsilverstein, mukesh27.
Fixes #57320.

#13 @johnbillion
16 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version trunk deleted

I don't think there's a need to use a doing it wrong message here, especially as it introduces a new localised string that's practically never going to be used. There's an inherent expectation in WordPress that filters return the correct type, and casting to an array is probably sufficient here. Let's not add unnecessary burden to translators.

#14 @johnbillion
16 months ago

In fact if $filtered_revisions is not an array then a PHP warning will be triggered further down on lines 232, 238, etc when it's treated as an array.

#15 @johnbillion
16 months ago

Sorry for the comment spam. $revisions can also only be WP_Post[] so int[] needs to be removed from the doc for the filter.

#16 @audrasjb
16 months ago

  • Keywords changes-requested added; commit removed

@audrasjb commented on PR #4087:


16 months ago
#18

Alright! Thanks John for putting together this PR! 👌

#19 @johnbillion
15 months ago

  • Keywords commit added
  • Owner changed from audrasjb to johnbillion
  • Status changed from reopened to accepted

#20 @johnbillion
15 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 55406:

Revisions: Remove an unnecessary call to _doing_it_wrong() and corresponding new text string from the implementation of the new wp_save_post_revision_revisions_before_deletion filter.

While the guard condition was technically correct, it's not practical or necessary to provide this protection for every use of every filter, and it adds unnecessary burden to translators to provide translations for strings that will likely not be seen.

Follow up to [55254].

Fixes #57320

Note: See TracTickets for help on using tickets.