Make WordPress Core

Opened 2 years ago

Last modified 4 weeks ago

#49532 assigned defect (bug)

Clicking "preview" multiple times on a post causes an identical autosave revision to be created and destroyed repeatedly

Reported by: inwerpsel Owned by: adamsilverstein
Milestone: 6.0 Priority: normal
Severity: normal Version: 5.4
Component: Revisions Keywords: has-patch reporter-feedback
Focuses: rest-api, performance Cc:

Description

How to reproduce:
Click the preview button multiple times in a row, without making any changes to the post in between. This will result with the autosave revision being created and destroyed repeatedly (check the response of the api request that does the autosave, it alternates between 200 and 400).

Detailed explanation:

Suppose the post has no autosave revision yet.

Clicking preview the first time results in a request being sent to "/wp-json/wp/v2/post_type/{id}/autosaves?_locale=user". In that request the autosave revision is looked up for the post.
https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/endpoints/class-wp-rest-autosaves-controller.php#L360

Then, because there is no record, it will create a new one "as a special post revision" at

https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/endpoints/class-wp-rest-autosaves-controller.php#L393

After creating the request returns 200 and the autosave revision record is in the database.

Pressing the second time it will again look up the autosave revision and this time find it. Then an object is created with the $_POST data, but which has the old autosave revision's ID. That object is then compared to the current post, and if it's not different, wp_delete_post_meta is called with the old autosave ID and the record is deleted.

Notice how now we're back in the same state as at the start (a post with no autosave revision). So we can keep on pressing the preview button and the record will keep on getting created and deleted, even though it contains the exact same data as the post of which it is an autosave.

Why is this an issue:
This creates and deletes records in the wp_posts table with no real use, causing unwanted load and exhausting the ids of the wp_posts table.

Proposed solution:
Either no record should be created in the first place if there is no difference, or the existing record shouldn't be deleted, even if it doesn't have any changes.

Attachments (1)

49532.diff (2.5 KB) - added by adamsilverstein 4 weeks ago.

Download all attachments as: .zip

Change History (11)

#1 @aduth
2 years ago

Upstream (Gutenberg) issue: https://github.com/WordPress/gutenberg/issues/20505

As noted in the original report, I can see how there may be multiple solutions to this implementation that may or may not take place in Gutenberg, or via change to the REST API to be "smarter" about how it decides to create revisions.

#3 @inwerpsel
2 years ago

Looks like this issue occurs in more cases than I previously found. The autosave record is also temporarily cleaned up when loading the block editor in post.php. The autosave is looked up there to see if it is newer than the latest edit of the post, if it's not then it's deleted there.

https://github.com/WordPress/WordPress/blob/master/wp-admin/edit-form-blocks.php#L323

Will anything break if the autosave isn't deleted in both cases? I would guess not at first glance, but maybe I'm missing something. I forced this locally by adding a filter:

<?php
// ...
        add_filter('pre_delete_post', [$this, 'do_not_delete_autosave'], 1, 3);
// ...
        public function do_not_delete_autosave( $something = null, $post = null, $force_delete = null ): ?bool {
                if ( preg_match( '/autosave-v\d+$/', $post->post_name ) ) {
                        return false;
                }

                return null;
        }

This doesn't seem to break anything. The only downside I see is that for each different author the autosave will be preserved indefinitely. However this is already the case for some of the autosaves, if no request that deletes the autosave is done after it has been created. Also that still seems preferable compared to deleting and creating new records many times over.

#4 @inwerpsel
2 years ago

The filter I proposed above resulted in the autosave not being deleted if the post is permanently deleted. Unfortunately the arguments of the filter are identical in both that case and the unwanted deletions, so I couldn't do anything but using $_GET to check if the current request is one for permanently deleting posts.

<?php
        private function add_filters() {
                add_filter('pre_delete_post', [$this, 'do_not_delete_autosave'], 1, 3);
        }

        public function do_not_delete_autosave( $something = null, $post = null, $force_delete = null ): ?bool {
                if (
                        $force_delete
                        || ( isset( $_GET['action'] ) && $_GET['action'] === 'delete' )
                        || ( isset( $_GET['delete_all'] ) )
                        || !preg_match( '/autosave-v\d+$/', $post->post_name ) ) {

                        return null;
                }

                return false;
        }

#5 @adamsilverstein
4 weeks ago

Proposed solution:
Either no record should be created in the first place if there is no difference, or the existing record shouldn't be deleted, even if it doesn't have any changes.

@inwerpsel thank you for opening this ticket and all the details around your efforts to prevent the behavior you described. Indeed: creating an autosave, then deleting it, then recreating it in an endless cycle is not the ideal (or intended) behavior.

I came across this ticket when working on previewing featured images in block themes because the underlying preview code that adjust post terms and the "thumbnail" (featured) image was failing when the autosave is missing. As you point out this happens every other autosave and autosaves are fired every time you preview a post.

I believe your suggestion that no autosave should be created when the autosave content matches the post content would be the cleanest way to resolve the issue, I'll look into that.

This ticket was mentioned in PR #2093 on WordPress/wordpress-develop by adamsilverstein.


4 weeks ago

  • Keywords has-patch added

#7 @adamsilverstein
4 weeks ago

In 49532.diff:

Revisions: don't create autosaves when all revisioned fields match the post

  • In the autosave controller (wp-includes/rest-api/endpoints/class-wp-rest-autosaves-controller.php), adjust the existing check to see if the autosave exactly matches the post content to run a little earlier, setting autosave_is_different appropriately.

Then, when the autosave content matches the post content:

  • Do not create an autosave, since that data is redundant (all revisioned fields match)
  • Persist the current behavior of removing any existing autosaves for the current user that (again, because the data us redundant)

Prevents a cycle bug where the autosave would be added and removed with each autosave trigger, needlessly consuming resources and potentially exhausting post ids.

@inwerpsel if you are able, it would be great to have you confirm this patch resolves the issue you reported.

#8 @adamsilverstein
4 weeks ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#9 @adamsilverstein
4 weeks ago

  • Keywords reporter-feedback added
  • Milestone changed from Awaiting Review to 6.0

#10 @adamsilverstein
4 weeks ago

Marking this for 6.0 since its probably too late to commit something like this for 5.9.

Note: See TracTickets for help on using tickets.