wp-admin/revisions.php uses the $post global in the wrong way

[tofandel]

I found a bug quite interesting when restoring a revision, in wp-admin/revision.php the code is not encapsulated and thus using the globals $post, $revision etc

A problem arrise because of those lines

<?php
if ( ! $post = get_post( $revision->post_parent ) )
                break;
// ...
wp_restore_post_revision( $revision->ID );
$redirect = add_query_arg( array( 'message' => 5, 'revision' => $revision->ID ), get_edit_post_link( $post->ID, 'url' ) );

As you can see the $post global is exposed and then used again after the the wp_restore_post_revision function call which triggers countless hooks

The problem is if the $post global is edited in one of those hooks, then the ID passed to get_edit_post_link is wrong and since post has not been set via WP_Query, the wp_reset_postdata function does nothing, here is a snippet you can use to demonstrate the bug, after restoring a revision you will be redirected to the edit page of the post 4 instead of the post you were editing

<?php
add_action('wp_restore_post_revision', function() {
        global $post;

        $post->ID = 4;
        
        wp_reset_postdata(); //This will do nothing because the post has not been defined through WP_Query
});

To fix this bug I would suggest encapsulating the logic into a function to stop exposing the variables as globals or to rename the variable being used

PS: this design flaw is present in other files as well, but that's the only one I noticed causing issues

[archon810]

I just spent hours trying to figure out why restoring revisions sometimes redirects to the wrong post and after debugging and pulling my hair out arrived at a conclusion that this has to be a bug in WP core.

Then I found this ticket (along with https://core.trac.wordpress.org/ticket/18408 which is still unresolved after 8 years and may or may not be related).

In our case, we have a hook into transition_post_status, and in this hook, we perform a secondary query using the correct new WP_Query(), have_posts(), and the_post(), followed by wp_reset_postdata().

The last post in this secondary query erroneously sticks around as $post after wp_reset_postdata().

This happens as part of wp-includes/post.php->wp_restore_post_revision()->wp_update_post()->wp_insert_post()->wp_transition_post_status(). The redirect ends up using this incorrect $post.

Here's the call stack:

Is there anyone who can take a look at properly resolving this bug @SergeyBiryukov?

Thanks.

[markparnell]

Renamed $post variable to $parent to avoid conflicts with global variable

[tofandel]

@markparnell You should probably set the WP Query's post variable to the $parent

<?php
global $wp_query;

$post = $parent;
$wp_query->posts = [$post];
$wp_query->post = $post;
setup_postdata($post);

to avoid breaking existing code and solve the issue with wp_reset_postdata not doing anything

(Though it feels hackish by lack of other way to do this)

[markparnell]

Thanks @tofandel. Had to tweak your logic slightly, but 46671.2.diff makes the global post data available again to anything hooked into the restore process while still fixing the original issue.

[SergeyBiryukov]

Just noting 46671.2.diff​ didn't work in my testing, as there are some other functions in the file depending on the $post global:

• get_edit_post_link()
• _draft_or_post_title()
• wp_print_revision_templates()

46671.3.diff​ seems to work, however it also appears to be a lot of effort to move away from the global that we ultimately have to set up again.

I don't think setup_postdata() is necessary here, or that wp_reset_postdata() not working as expected is a concern here, because the hooks in wp_insert_post() and wp_transition_post_status() should have all the necessary context without relying on the $post global. Since these hooks are not in the WordPress Loop, any functions hooked to them should not expect the $post global to be available.

So it looks like the only thing to fix here is to make sure core doesn't break if a plugin sets up the $post global for its own purposes, either intentionally or accidentally, as in comment:4.

46671.4.diff​ does just that, using the approach already applied in wp-admin/edit-form-blocks.php.

[SergeyBiryukov]

In 48625:

Revisions: Ensure the global $post remains the same after revision is restored.

Because wp_insert_post() and wp_transition_post_status() are called during the process, plugins can unexpectedly modify $post.

Props markparnell, tofandel, archon810, SergeyBiryukov.
Fixes #46671.