Opened 6 years ago
Closed 4 years ago
#46671 closed defect (bug) (fixed)
wp-admin/revisions.php uses the $post global in the wrong way
Reported by: | tofandel | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 5.5 | Priority: | normal |
Severity: | major | Version: | 5.2 |
Component: | Revisions | Keywords: | has-patch |
Focuses: | administration | Cc: |
Description
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
Attachments (4)
Change History (14)
#3
@
6 years ago
- Keywords needs-patch added
- Milestone changed from Awaiting Review to Future Release
#7
@
5 years ago
@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)
#8
@
5 years ago
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.
#9
@
5 years ago
- Milestone changed from Future Release to 5.5
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
#10
@
4 years ago
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
.
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 #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 correctnew WP_Query()
,have_posts()
, andthe_post()
, followed bywp_reset_postdata()
.The last post in this secondary query erroneously sticks around as
$post
afterwp_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.