WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 13 months ago

#16215 closed defect (bug) (fixed)

Post Revision history displays the incorrect author

Reported by: mdawaffe Owned by: westi
Milestone: 3.6 Priority: normal
Severity: normal Version: 2.6
Component: Revisions Keywords: needs-testing has-patch
Focuses: Cc:

Description

Steps to reproduce:

  1. Create and Publish a new post as user admin.
  2. Edit that post and Update as user mdawaffe.
  3. View revision history for that post. Note that the revisions are attributed to the wrong authors.

The revision's post_author is currently being set to whoever edited the post away from the state represented by that revision row. It should be set to the person who edited the post *to* the state represented by that row.

This bug has been around since the introduction of post revisions.

To fix for future posts, I can think of two options.

  1. We should be able to pull the correct author for a new revision row from the _edit_lock/_edit_last post meta of the post (as long as we grab that before we update the post row).
  1. Currently, when a post is updated, revisions first grabs a copy of the current state of the post, stores it as a revision, then updates the post row. To fix this bug, we could instead update the post row first then store a copy of that new state as a revision. That would mean the most recent revision for all posts would be a duplicate of the actual post.

Option 2 would be cleaner code, option 1 would be cleaner data.

To fix for existing posts, we need to go through each post and fix each revision. That's incredibly expensive to do on upgrade, so I suggest doing it per post on the fly when the post edit screen or post revisions screen is loaded.

If we fix on the fly, we have to be able to keep track of which posts/revisions have been fixed and which haven't. We could track that with:

  1. An option that is set on upgrade with a timestamp. Compare post_modified to that timestamp. This seems fragile to me since I bet there are plugins that override post_modifed.
  2. Post meta. Easy, but adds one post meta per post just to fix a lame bug.
  3. Bump the menu_order of each fixed revision row from 0 -> 1. (A version number for the revisioning system :)). Hacky.

I like 3: menu_order.

We could also leave everything as is, allow the data to be wrong, and "fix" it on display or even in get_post().

Aside: While we're in there, we may want to "fix" revisions' post_modified columns. Currently, post_modified is identical to post_date, which is the time the post was put into the state represented by the revision. We could make post_modified the time the post was edited away from the state represented by the revision.

Attachments (25)

Diff 1.png (49.8 KB) - added by mdawaffe 3 years ago.
Diff 2.png (53.5 KB) - added by mdawaffe 3 years ago.
16215.diff (5.7 KB) - added by mdawaffe 3 years ago.
First pass. New revisions: correct info DB. Old revisions: fix on the fly for display only
16215-1.diff (5.7 KB) - added by adamsilverstein 18 months ago.
only update is applies cleanly to current trunk
16215-2.diff (6.1 KB) - added by adamsilverstein 18 months ago.
corrects list in meta box
combined-3.diff (8.4 KB) - added by adamsilverstein 18 months ago.
combined patch for testing
Screen Shot 2013-01-24 at 12.29.46 AM.png (81.5 KB) - added by adamsilverstein 18 months ago.
after three edits test, combined patch
16215.4.diff (8.4 KB) - added by mdawaffe 18 months ago.
uses post_name for revision_version
revisions.php (2.6 KB) - added by adamsilverstein 17 months ago.
1st draft unit test for #16215
16215.2.diff (996 bytes) - added by adamsilverstein 16 months ago.
save post revision after post update not before
16215.3.diff (4.2 KB) - added by adamsilverstein 16 months ago.
add revision after post update, new hook, new check for duplicate before saving
23198.diff (6.7 KB) - added by adamsilverstein 16 months ago.
add revision after post update, fix old data on the fly, no duplicates
16215.5.diff (6.7 KB) - added by adamsilverstein 16 months ago.
add revision after post update, fix old data on the fly, no duplicates
16215.6.diff (6.0 KB) - added by adamsilverstein 16 months ago.
clean up white space, diff against current trunk
16215.7.diff (10.8 KB) - added by adamsilverstein 16 months ago.
updated against trunk
16215+23497.diff (75.7 KB) - added by adamsilverstein 16 months ago.
combined patch for testing
16215.8.diff (15.9 KB) - added by adamsilverstein 16 months ago.
fixes old data on the fly, add current post as revision on 1st update
16215.9.diff (19.3 KB) - added by adamsilverstein 16 months ago.
16215.10.diff (11.0 KB) - added by adamsilverstein 16 months ago.
16215.11.diff (11.0 KB) - added by adamsilverstein 16 months ago.
16215.12.diff (10.2 KB) - added by adamsilverstein 16 months ago.
16215-13.patch (4.4 KB) - added by azaozz 16 months ago.
16215-14.patch (12.1 KB) - added by azaozz 16 months ago.
16215-15.patch (2.3 KB) - added by azaozz 16 months ago.
16215-16.patch (3.1 KB) - added by azaozz 16 months ago.

Download all attachments as: .zip

Change History (123)

mdawaffe3 years ago

mdawaffe3 years ago

comment:1 nacin3 years ago

Sweet. As discussed, I like menu_order too.

I guess #11022 illustrated the issue as well.

comment:2 archon8103 years ago

Thanks for finally giving this proper attention, guys. It's caused numerous problems when trying to figure out who edited what.

comment:3 miqrogroove3 years ago

For the record, nacin has closed my FIFTEEN MONTH OLD TICKET as a duplicate of this one.

comment:4 nacin3 years ago

That one (#11022) had no patch. This one doesn't either, but what it does have is a proper and complete explanation of the issue. (Also, dd32 closed it, albeit referencing the wrong ticket.)

comment:5 miqrogroove3 years ago

  • Version changed from 3.1 to 2.6

Can we at least correct the version number?

comment:6 nacin3 years ago

Yes, that's fine. It's always been like this.

comment:7 solarissmoke3 years ago

  • Keywords dev-feedback added

For future posts, it seems to me that easiest would be simply to add post_author to the versioned fields in _wp_post_revision_fields(), and then the existing post author would be saved rather than the current one. But in that function it says for post_author:

// WP uses these internally either in versioning or elsewhere - they cannot be versioned

Why?

comment:8 miqrogroove3 years ago

For the record, I don't see a reason to fix this for "existing posts". This problem has gone unresolved from day one when post versioning was conceived. If WordPress could just save the correct freakin username when I edit posts, I would be very happy.

comment:9 archon8103 years ago

If fixing previous posts is what's holding up the release, I'd rather see the fix for displaying first, and for previous posts as a separate release, maybe in another milestone.

comment:10 danielbachhuber3 years ago

  • Cc d@… added

comment:11 SergeyBiryukov3 years ago

  • Keywords needs-patch added

mdawaffe3 years ago

First pass. New revisions: correct info DB. Old revisions: fix on the fly for display only

comment:12 mdawaffe3 years ago

  • Keywords needs-testing has-patch added; needs-patch removed

attachment:ticket:16215:16215.diff

  • Stores revisioning system version number in wp_posts.comment_count.
  • Updates _edit_last post meta during wp_insert_post() so that meta value is always correct.
  • For new revisions, uses _edit_last post meta as the revision author (the user who edited the post to that state).
  • For old revisions, fixes the post_author on the fly during display. Does not store that fix in the DB.
  • Fixes display of "current revision" author in wp_list_post_revisions().
  • Does not address the potential post_modified/post_date enhancement.

I took the menu_order approach outlined above in the ticket, but used comment_count instead since a page's menu_order is actually versioned by the revisioning system. This comment_count trick won't work if plugins (or future core) allow comments on revisions. As an alternative, we could hack the version into post_name.

I chose not to update the old revisions' DB rows so we could try the patch out without committing ourselves to the (destructive) UPDATEs. UPDATEs should be reasonably straightforward to add, and we should do UPDATEs since the way the patch fixes on display could mix poorly with persistent caches.

comment:13 mdawaffe3 years ago

I tested (and so should you!) attachment:ticket:16215:16215.diff with old and new posts. That is, posts with revisions having comment_counts:

  • Old post: 0,0,0
  • New post: 1,1,1
  • Newly edited old post: 0,0,0,1,1,1

Where the revisions cycled between three different authors. Two authors isn't enough to really test the issue.

When we add UPDATEs, we'll also need to test another edge case:

  • Old post in strange state (due to races in the eventual UPDATE code): 0,0,0,1,1,1,0,0,0,1,1,1

comment:14 nacin3 years ago

  • Keywords 3.4-early added

Time to fix this.

comment:15 markpea2 years ago

I hesitate to add a +1 or 'me too' comment since I'm new here and don't know the correct protocol. But correct attribution of post revisions is a pretty basic feature so +1 to getting it fixed.

comment:16 aaroncampbell18 months ago

  • Cc aaroncampbell added
  • Keywords 3.4-early removed
  • Milestone changed from Future Release to 3.6

comment:17 westi18 months ago

  • Keywords revisions-3.6 added
  • Owner set to westi
  • Status changed from new to accepted

Can we fix this?
Yes we can!

comment:18 westi18 months ago

  • Keywords revisions-3.6 removed

comment:19 follow-up: adamsilverstein18 months ago

i'm working on getting this ticket resolved! thought i would start by testing the patch provided by mdawaffe.

the patch did not apply cleanly, but i was able to get it applied to current and tested locally using three users. the patch works, with some minor issues, i think its easiest to see in screen shots.

for each of the tests i created a post with the content 'admin' as the user admin, next i switched to user 'testuser' and edited the content to be 'testuser', next i switched to 'testuserb' and changed the content to 'testuser B', finally i logged as the user admin and checked revisions. the screenshots show the meta box on the edit post screen and the revisions review screen as well as each revision comparison.

new post created before patch applied - here we see the bug in action, comparison users are all off. note that the meta box on the post edit page actually looks right (i think).

new post created after patch applied - the patch fixed all the comparisons, yea! except...

note: there two revisions listed for my initial admin action of creating the post, the first is triggered when i left the title field, the second being my 'publish' action (publishing with only a title in another test removed this extra initial revision) . this seems like it would be confusing to end users, shouldn't the initial revision (shown) be my first publish?

also note another problem here, the meta-box on the post edit page shows one admin revision, one testuser revision, but no testuserb revision, which was the last revision. i'm logged back in as admin and haven't hit update yet - i should see testuserb's change in the revision list, and I DO in fact see it once i go to the compare revisions screen. this seems like a new bug/regression and needs fixing, might just be showing one too few revisions.

post created before patch applied, viewed after patch applied - the patch works as advertised on the comparison screen, the post created without the patch now looks good on the comparison screen and UPDATES should be apply-able. exhibits same issue with meta-box on post edit page as in 2nd case above.

what remains to-do:

  • fix issue with meta box display (fixed)
  • add UPDATE statements

things to consider:

  • is using comment_count for revisions a good idea? will we ever want comments on revisions? any good ideas for another approach?

here were comments from original patch "I took the menu_order approach outlined above in the ticket, but used comment_count instead since a page's menu_order is actually versioned by the revisioning system. This comment_count trick won't work if plugins (or future core) allow comments on revisions. As an alternative, we could hack the version into post_name."

any other input here or leave as is?

  • address this issue from original ticket?: "Currently, post_modified is identical to post_date, which is the time the post was put into the state represented by the revision. We could make post_modified the time the post was edited away from the state represented by the revision." comments? is this issue in the post or in the revisions? move to another ticket

*more testing, unit tests?


Last edited 18 months ago by adamsilverstein (previous) (diff)

adamsilverstein18 months ago

only update is applies cleanly to current trunk

adamsilverstein18 months ago

corrects list in meta box

comment:20 adamsilverstein18 months ago

the function wp_list_post_revisionsL1376 was checking if ( $parent ), otherwise removing top revision from list. this must be legacy code, could there be a use case or is this all internal?

didn't want to remove that check, so for now, my patch sets parent to true before calling wp_list_post_revisions(). this fixes the meta box for all my test posts, created before after patch etc.

comment:21 in reply to: ↑ 19 mdawaffe18 months ago

Replying to adamsilverstein:

i'm working on getting this ticket resolved!

I too am going to work on this ticket :) Thanks for the refresh and the detailed testing and writeup.

I don't have any thoughts yet. I'll start poking around.

comment:22 adamsilverstein18 months ago

great, thanks!

comment:23 adamsilverstein18 months ago

  • Cc adamsilverstein@… added

adamsilverstein18 months ago

combined patch for testing

comment:24 adamsilverstein18 months ago

attached combined patch for testing, also fixes (related) #9843; removes initial revision (last one in list) from display so after test scenario above you see only three revisions, admin save, testuser save, testuserb save.

discovered the lovely 'self comparison detected' easter egg along the way! he he cute! :}

Last edited 18 months ago by adamsilverstein (previous) (diff)

adamsilverstein18 months ago

after three edits test, combined patch

comment:25 ethitter18 months ago

  • Cc erick@… added

comment:26 mdawaffe18 months ago

Cool. Let's work on one bug at a time. Smaller, focused patches will move forward more quickly.

The most important thing to fix is the revisions' authorship. Once we have that done, we can look at any UI/UX issues.

I'm working on a patch that moves the revision version data to post_name. We already take that field over for revisions.

The patch also takes a different approach in displaying the correct author (one that won't poison any persistant caches), and will have a way to upgrade each revision row. I'm not yet sure when we should call that upgrade function.

I'll get the patch up this weekend or Monday.

mdawaffe18 months ago

uses post_name for revision_version

comment:27 follow-up: mdawaffe18 months ago

attachment:16215.4.diff

  • Stores revision_version wp_posts.post_name. Revisions already completely takes over that field, so we may as well use it.
  • Updates _edit_last post meta during wp_insert_post() so that meta value is always correct.
  • For new revisions, uses _edit_last post meta as the revision author (the user who edited the post to that state).
  • For old revisions, fixes the post_author on the fly during display using an expanded get_the_modified_author.
    • If the correct author cannot be determined, it makes an essentially arbitrary guess) and appends a '?' to the end of the author's display name.
    • The post_author as corrected by get_the_modified_author is not stored in the post object to avoid cache pollution.
    • The post_author as corrected by get_the_modified_author is not stored in the DB.
    • As a side effect of using get_the_modified_author everywhere, the display of the "current revision" author in wp_list_post_revisions() is fixed
    • This addition to get_the_modified_author could be made a filter.
  • Includes a new _wp_upgrade_revisions_of_post() to upgrade a post's revisions. Currently uncalled.
    • Goes through each revision from most recent to oldest and determines if and how the revision needs to be upgraded.
    • Uses a options table hack to get an exclusive lock on the upgrade routine for that post. Calling this function multiple times in parallel would be racy.

Here's how I tested:

  1. New blog with 3 users running unpatched trunk.
  2. Create a new post as user_id=1.
  3. Edit the post as user_id=2.
  4. Edit the post as user_id=3.
  5. Edit the post as user_id=1.
  6. Edit the post as user_id=2.
  7. Edit the post as user_id=3.
  8. View the revisions for that post. Notice how they are off by one.
  9. Apply the patch.
  10. View the revisions for that post. Notice how they are correct.
  11. Edit the post as user_id=1.
  12. Edit the post as user_id=2.
  13. Edit the post as user_id=3.
  14. View the revisions for that post. Notice how they are correct.
  15. Revert the patch.
  16. View the revisions for that post. Notice how the revisions created before the patch was applied are off by one.
  17. Edit the post as user_id=1.
  18. Edit the post as user_id=2.
  19. Edit the post as user_id=3.
  20. Apply the patch.
  21. View the revisions for that post. Notice how all are correct except one: whenever there is a revision_version=1 revision chronologically followed by a revision_version=0 revision, we do not know what author created the revision_version=0 revision. Authorship for revision_version=0 revisions is stored in the chronologically preceding revision. Notice the question mark after that revision author's name. It's a guess (a bad one).
  22. Run the upgrade function on that post.
  23. View the revisions for that post. Should be the same as in step 21.

In the above testing procedure, we end up with revisions having something like the following revision_versions going from oldest to newest:

0,0,0,0,0,0,1,1,1,0,0,0

After the upgrade function is run, we end up with:

1,1,1,1,1,1,1,1,1,0,1,1

That revision on the boundary between 1 → 0 is the revision for which we have to guess the author.

Those guessed authors should be rare. They only occur when the site is upgraded to include this patch, then downgraded, then upgraded again.

In the normal wp-admin editor, a revision is created essentially every time a post is first created. Without that initial revision, it's possible that for posts created pre-patch, get_the_modified_author() will have to guess at the author of the first revision. So created pre-patch with XML-RPC, for example, might have guessed first revision authorship. I'm not sure: I haven't tested that.


I'm not sure when to run the upgrade function. On post edit? On revision listing? Asynchronously on post edit via WP_Cron?

In theory we could let the data in the DB be incorrect for ever, and always correcty it dynamically on display or even in WP_Post.


The patch is probably missing some post_type_supports( $post->post_type, 'revisions' ) and/or WP_POST_REVISIONS checks.


I just discovered there's a bug in the upgrade script: the very first revision is never upgraded... I think that may be intentional :) I'll look into it later.

comment:28 in reply to: ↑ 27 adamsilverstein17 months ago

Replying to mdawaffe:

attachment:16215.4.diff

  • Stores revision_version wp_posts.post_name. Revisions already completely takes over that field, so we may as well use it.
  • Updates _edit_last post meta during wp_insert_post() so that meta value is always correct.
  • For new revisions, uses _edit_last post meta as the revision author (the user who edited the post to that state).
  • For old revisions, fixes the post_author on the fly during display using an expanded get_the_modified_author.
    • If the correct author cannot be determined, it makes an essentially arbitrary guess) and appends a '?' to the end of the author's display name.
    • The post_author as corrected by get_the_modified_author is not stored in the post object to avoid cache pollution.
    • The post_author as corrected by get_the_modified_author is not stored in the DB.
    • As a side effect of using get_the_modified_author everywhere, the display of the "current revision" author in wp_list_post_revisions() is fixed
    • This addition to get_the_modified_author could be made a filter.
  • Includes a new _wp_upgrade_revisions_of_post() to upgrade a post's revisions. Currently uncalled.
    • Goes through each revision from most recent to oldest and determines if and how the revision needs to be upgraded.
    • Uses a options table hack to get an exclusive lock on the upgrade routine for that post. Calling this function multiple times in parallel would be racy.

Here's how I tested:

  1. New blog with 3 users running unpatched trunk.
  2. Create a new post as user_id=1.
  3. Edit the post as user_id=2.
  4. Edit the post as user_id=3.
  5. Edit the post as user_id=1.
  6. Edit the post as user_id=2.
  7. Edit the post as user_id=3.
  8. View the revisions for that post. Notice how they are off by one.
  9. Apply the patch.
  10. View the revisions for that post. Notice how they are correct.
  11. Edit the post as user_id=1.
  12. Edit the post as user_id=2.
  13. Edit the post as user_id=3.
  14. View the revisions for that post. Notice how they are correct.
  15. Revert the patch.
  16. View the revisions for that post. Notice how the revisions created before the patch was applied are off by one.
  17. Edit the post as user_id=1.
  18. Edit the post as user_id=2.
  19. Edit the post as user_id=3.
  20. Apply the patch.
  21. View the revisions for that post. Notice how all are correct except one: whenever there is a revision_version=1 revision chronologically followed by a revision_version=0 revision, we do not know what author created the revision_version=0 revision. Authorship for revision_version=0 revisions is stored in the chronologically preceding revision. Notice the question mark after that revision author's name. It's a guess (a bad one).
  22. Run the upgrade function on that post.
  23. View the revisions for that post. Should be the same as in step 21.

In the above testing procedure, we end up with revisions having something like the following revision_versions going from oldest to newest:

0,0,0,0,0,0,1,1,1,0,0,0

After the upgrade function is run, we end up with:

1,1,1,1,1,1,1,1,1,0,1,1

That revision on the boundary between 1 → 0 is the revision for which we have to guess the author.

Those guessed authors should be rare. They only occur when the site is upgraded to include this patch, then downgraded, then upgraded again.

In the normal wp-admin editor, a revision is created essentially every time a post is first created. Without that initial revision, it's possible that for posts created pre-patch, get_the_modified_author() will have to guess at the author of the first revision. So created pre-patch with XML-RPC, for example, might have guessed first revision authorship. I'm not sure: I haven't tested that.


I'm not sure when to run the upgrade function. On post edit? On revision listing? Asynchronously on post edit via WP_Cron?

In theory we could let the data in the DB be incorrect for ever, and always correcty it dynamically on display or even in WP_Post.


The patch is probably missing some post_type_supports( $post->post_type, 'revisions' ) and/or WP_POST_REVISIONS checks.


I just discovered there's a bug in the upgrade script: the very first revision is never upgraded... I think that may be intentional :) I'll look into it later.

i don't think we need to worry about upgrade/downgrade/upgrade you mentioned. i'm not even sure we need to upgrade the old db entries, i think just showing them correctly and storing them correctly from now on might suffice.

Version 0, edited 17 months ago by adamsilverstein (next)

comment:29 adamsilverstein17 months ago

i am trying to create a unit test for this bug, but need a little help. my attached test goes thru the steps outlined above to reproduce the bog, and should fail if the bug exists, but it passes... (usually anyway - in my test environment it sometimes fails, i'm not sure why).

i must be doing something, or several things, wrong because the assertions all pass before the patch and fail after the patch. if someone can help me out here, i'm happy to write more tests for the revisions bugs we are fixing.

Last edited 17 months ago by adamsilverstein (previous) (diff)

adamsilverstein17 months ago

1st draft unit test for #16215

comment:30 ethitter17 months ago

I tested the latest patch over the weekend, and it corrects this issue as well as #20982 as best I can tell. As I haven't wrapped my head around unit tests yet, I'm of no help there though.

The upgrade routine is a great inclusion, in my opinion; whenever we can, showing consistent and correct information is a good idea.

comment:31 westi17 months ago

In 1210/tests:

Revisions: Add a test case which checks the _edit_last postmeta value after a revision restore

Also validates some of the things to be fixed for the off-by-one issues with authorship.
See: #20982, #16215
Props adamsilverstein for the original patch.

comment:32 follow-ups: nacin17 months ago

Currently, when a post is updated, revisions first grabs a copy of the current state of the post, stores it as a revision, then updates the post row. To fix this bug, we could instead update the post row first then store a copy of that new state as a revision. That would mean the most recent revision for all posts would be a duplicate of the actual post.

This was suggested as having cleaner code, but less clean data.

I want to argue that this is actually the best approach in terms of data as well. When talking extensively with benbalter about revisions, conflicts, locking, post forking, and editorial flows (author of the Post Forking plugin and contributor to both #18515 and #12706), a big issue pointed out is that you don't actually have a revision ID in hand until the post is edited an additional time. This makes it fairly difficult to do anything interesting. Ben ended up writing a routine that calculates "what is the revision after revision X, because that's the one I actually wanted but it didn't exist at the time, I only had X". Instead of storing Y, you have to store X+1. It makes everything simpler if you have a revision for everything.

(Incidentally, _edit_last is also discarded on export/import.)

IRC conversation: https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2013-02-13&sort=asc#m554412

comment:33 in reply to: ↑ 32 adamsilverstein16 months ago

Replying to nacin:

Currently, when a post is updated, revisions first grabs a copy of the current state of the post, stores it as a revision, then updates the post row. To fix this bug, we could instead update the post row first then store a copy of that new state as a revision. That would mean the most recent revision for all posts would be a duplicate of the actual post.

This was suggested as having cleaner code, but less clean data.

I want to argue that this is actually the best approach in terms of data as well. When talking extensively with benbalter about revisions, conflicts, locking, post forking, and editorial flows (author of the Post Forking plugin and contributor to both #18515 and #12706), a big issue pointed out is that you don't actually have a revision ID in hand until the post is edited an additional time. This makes it fairly difficult to do anything interesting. Ben ended up writing a routine that calculates "what is the revision after revision X, because that's the one I actually wanted but it didn't exist at the time, I only had X". Instead of storing Y, you have to store X+1. It makes everything simpler if you have a revision for everything.

(Incidentally, _edit_last is also discarded on export/import.)

IRC conversation: https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2013-02-13&sort=asc#m554412

i will try to rearrange the order of post update/revision save as per suggestion 2 above. any idea why the current order is used and if i'm likely to break anything by making this change?

this should fix the issue with authors on new revisions, but we still need a way to correct old data unless we decide old bad data is ok. it looks like the existing patch does a reasonable job of that, so i'm going to leave that code and just update the part where revisions are created.

this fix should also address the post_modified issue mentioned in the original ticket.

adamsilverstein16 months ago

save post revision after post update not before

comment:34 adamsilverstein16 months ago

16215.2.diff​ shifts the revision save until after the post update and seems to make sense based on discussion above: namely by shifting until after the post is actually updated, we should capture the latest post info - including post_author and post_modified.

comment:35 adamsilverstein16 months ago

note: the current save revisions code compares the passed data to the current post data and if there is no change doesn't store a revision (see #7392 and #9843). 16215.2.diff​ would break that. if we are storing the revision after the post is updated we need a different way to check for changes. i will work on that.

existing method may be just fine, see below

Last edited 16 months ago by adamsilverstein (previous) (diff)

adamsilverstein16 months ago

add revision after post update, new hook, new check for duplicate before saving

comment:36 adamsilverstein16 months ago

16215 comments

  • under the old system when users clicked update on a post the system did two things:
    • saved a copy of the post information stored in the database as a revision (eg. the previous post update data)
    • saved the current post editor information to the database post record

This resulted in:

  • revisions stored for the previous edit only, not for the current post information
  • however, the current post information IS stored in the database as the post data, it's just not stored as a revision
  • in addition, the old system stored two revisions for the first save, further confusing things!


Furthermore:

  • The old revisions compare system did a poor job of displaying the revisions, treating the last revision as the current post even though it was not, meaning all revisions were one position off - hence the current ticket.

However:

  • The new revision version in #23497 works around this data issue by prepending a copy of the current post to the list of revisions when building the diffs for display.
  • testing existing post data with the new revisions system the data (i believe) looks fine, restores work correctly, and the current version of the post is used for comparisons: in the one handle mode the current post is compared to all previous revisions; in the two handle mode, the leftmost handle position represents the current post version (and data is pulled from). the language properly indicates the 'current version' is being compared. this could use more testing!

I already created 16215.3.diff implementing fix 2 above, after this fix when users click update on a post the system does two things:

  • saves the post to the database
  • stores a copy of the post data as a revision

This results in:

  • one revision stored for each update... sounds good so far, except that:
    • the new revision data no longer matches the old revision data, with messy results
    • the code adds a new hook and breaks anything that assumes wp_save_post_revision is going to run at pre_post_update

Therefore, I suggest closing this ticked as fixed in #23497, but would appreciate a second option on this conclusion

Last edited 16 months ago by adamsilverstein (previous) (diff)

comment:37 in reply to: ↑ 32 adamsilverstein16 months ago

can't we just treat the currently stored post data as the most recent revision, why do we need to store an additional copy of the current post data as a revision? i think the real issue this ticket exposes was on the revision display/compare side, the storage logic makes sense.

Replying to nacin:

Currently, when a post is updated, revisions first grabs a copy of the current state of the post, stores it as a revision, then updates the post row. To fix this bug, we could instead update the post row first then store a copy of that new state as a revision. That would mean the most recent revision for all posts would be a duplicate of the actual post.

This was suggested as having cleaner code, but less clean data.

I want to argue that this is actually the best approach in terms of data as well. When talking extensively with benbalter about revisions, conflicts, locking, post forking, and editorial flows (author of the Post Forking plugin and contributor to both #18515 and #12706), a big issue pointed out is that you don't actually have a revision ID in hand until the post is edited an additional time. This makes it fairly difficult to do anything interesting. Ben ended up writing a routine that calculates "what is the revision after revision X, because that's the one I actually wanted but it didn't exist at the time, I only had X". Instead of storing Y, you have to store X+1. It makes everything simpler if you have a revision for everything.

(Incidentally, _edit_last is also discarded on export/import.)

IRC conversation: https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2013-02-13&sort=asc#m554412

adamsilverstein16 months ago

add revision after post update, fix old data on the fly, no duplicates

adamsilverstein16 months ago

add revision after post update, fix old data on the fly, no duplicates

comment:38 adamsilverstein16 months ago

changes in 16215.5.diff

  • wp_save_post_revision is now called before and after post update is fired, the pre post update call corrects old revision data on the fly by saving a copy of the previous post save as a revision before the post data is updated. once the most recent revision modified date matches the current post modified date, the pre update calls stops doing anything. the post update call always saves a copy of the updated post as a revision.
  • after updating a post, the most recent revision will always match the current post; for old posts two revisions will be added: one for the previous save (not yet stored) and one for the current version.
  • a new helper method wp_first_revision_matches_current_version tests if the most recent revision matches the post (new data), the revisions display code in #23497 will also need to know if the current post is stored as a revision.
  • two revisions are still created for the 1st post publish since post update gets called when users leave the title field, one revision is title only with blank content, the next includes the content.
  • revisions are not stored if no _wp_post_revision_fields have changed


comment:39 adamsilverstein16 months ago

  • Summary changed from Post Revision history displays the incorrect author to on post update, ensure most recent revision data matches stored post data; fix old revisions
  • Type changed from defect (bug) to enhancement

comment:40 nacin16 months ago

  • Summary changed from on post update, ensure most recent revision data matches stored post data; fix old revisions to Post Revision history displays the incorrect author
  • Type changed from enhancement to defect (bug)

adamsilverstein16 months ago

clean up white space, diff against current trunk

comment:41 follow-up: markjaquith16 months ago

What's the status on getting this fixed for existing posts?

comment:42 in reply to: ↑ 41 adamsilverstein16 months ago

Replying to markjaquith:

What's the status on getting this fixed for existing posts?

ignore my previous response, thought you were asking on backbone ticket.

latest patch on this ticket does correct data for existing posts, the first time a post is updated.

had asked @nacin about upgrading post data on WordPress database upgrade, but he said no. all thats required is running wp_save_post_revision on every post. wp_save_post_revision checks if a current post copy exists as a revision and saves a copy if not.

developers can safely call wp_save_post_revision() to make sure a current revision exists (it will not create a duplicate if one already exists).

Last edited 16 months ago by adamsilverstein (previous) (diff)

adamsilverstein16 months ago

updated against trunk

adamsilverstein16 months ago

combined patch for testing

comment:43 adamsilverstein16 months ago

16215.7.diff is a refresh of 16215.4.diff diffed against the current trunk. i tested and it indeed fixes the author off by one issue; also after applying patch from 23497 it continues to show correct revisions.

i also updated ajax-actions to use the corrected call for correct display of the revision author. 16215+23497.diff​ combines the two tickets for testing, applying both patches doesn't work directly.

comment:44 follow-up: mdawaffe16 months ago

I'm very happy to see this bug getting the attention it deserves. Thanks, adamsilverstein, for championing it :)

With the latest commit at #23497 [23769], what patch should I apply to test things?

comment:45 in reply to: ↑ 44 adamsilverstein16 months ago

Replying to mdawaffe:

I'm very happy to see this bug getting the attention it deserves. Thanks, adamsilverstein, for championing it :)

>
> With the latest commit at #23497 [23769], what patch should I apply to test things?

yes, this ticket is really going to help button up the new revisions updates!

now that the latest patch on #23497 was committed, this patch needs one more refresh; i am planning to work on it tonight. in addition to your code - which works perfectly as far as i can tell, thanks - i made the requested change so that a current copy of the post is stored as a revision, instead of the previous changes being the most recent revision.

for a while i thought that was all that was required, but I was very wrong! both fixes together yield the best results - corrected authors for old revision data (and correct authors for new revisions), and revisions data that includes the current post data as a revision (once a post is updated or a developer calls wp_save_post_revision())

i should have a refreshed patch for testing for you tonight, or if you want you can piece it together:) i also need to update ajax-actions to get the new revisions ui to use your function call to get the post author and will include that in my patch.

adamsilverstein16 months ago

fixes old data on the fly, add current post as revision on 1st update

comment:46 adamsilverstein16 months ago

in 16215.8.diff​:

  • combined previous patches and diffed against trunk
  • incorporated get_the_modified_author function in meta box, revision page meta info and tooltips
  • did initial testing, created post before patch with several authors, off by one; applied patch, authors corrected (in display, on the fly); added new revisions, attribution correct, newest revision matches post


ready for testing & review

comment:47 follow-up: markjaquith16 months ago

The gravatars are wrong on fixed-on-display-on-the-fly views (in both places). The name is right, but the gravatar is off.

comment:48 follow-up: markjaquith16 months ago

When a new revision is saved, the old incorrectly attributed revisions aren't fixed.

comment:49 in reply to: ↑ 47 ; follow-up: adamsilverstein16 months ago

Replying to markjaquith:

The gravatars are wrong on fixed-on-display-on-the-fly views (in both places). The name is right, but the gravatar is off.

good catch. the datastamp is similarly off. i will revisit the code to see if we can fix that.

comment:50 in reply to: ↑ 48 adamsilverstein16 months ago

Replying to markjaquith:

When a new revision is saved, the old incorrectly attributed revisions aren't fixed.

correct. the update code is there, but inactive - it needs review to see if a. its coded well and b. its needed.

the names are always corrected on display, even though old revisions have the wrong data and new revisions contain the correct data, the author in the list and revisions ui should always show the correct author. i will work on correcting the gravatar and dates mentioned above.

comment:51 in reply to: ↑ 49 adamsilverstein16 months ago

Replying to adamsilverstein:

Replying to markjaquith:

The gravatars are wrong on fixed-on-display-on-the-fly views (in both places). The name is right, but the gravatar is off.

good catch. the datastamp is similarly off (i think). i will revisit the code to see if i can fix that.

comment:52 follow-up: markjaquith16 months ago

I do think it's needed. It's pretty terrible that our revisions have bad data.

adamsilverstein16 months ago

comment:53 adamsilverstein16 months ago

  • removed ( 'form-table' == $format ) section from wp_list_post_revisions, no longer used
  • updated _wp_upgrade_revisions_of_post() and called after revision save via pre_post_update hook
  • added get_the_corrected_modified_author - returns corrected revision user name & id as array

in my initial testing, this patch:

  • corrects autor name and gravatar in revision list and revision comparison, on the fly
  • on first post update old data is corrected, further updates leave data intact
  • author and gravatar dislpay correctly before and after data update

before patch, create post edit as user admin, user1, user2
note: user ID and content showing who updated are 'off by one' (shown in reverse order, bottom 'blank' content is 1st revision title only made by admin)

http://f.cl.ly/items/1d2X2l2V142e380e1w45/beforepatchbaddata.png

then applied patch, edited post again as user1, note corrected data:

http://f.cl.ly/items/1M023m3m0k3T370V2c3O/postupdatedafterpost.png

comment:54 in reply to: ↑ 52 adamsilverstein16 months ago

Replying to markjaquith:

I do think it's needed. It's pretty terrible that our revisions have bad data.

yes, bad data is terrible, and there is lots of it! latest patch corrects the old data on first update; added to pre_post_update thinking database updates happening here anyway.

comment:55 markjaquith16 months ago

The tooltip author is the wrong author when fixed on the fly. Looks good once fixed.

comment:56 adamsilverstein16 months ago

as per IRC discussion today, I am going to refactor this patch:

  • fix data on opening the post for editing screen
  • remove on the fly code, no longer needed
  • switch version tracking to use suffixes, with format: {$parent_id}-revision-v{$revisioning_version} (dropping $n for revisions), for better compatibility with other code

comment:57 follow-ups: mdawaffe16 months ago

fix data on opening the post for editing screen

I worry about race conditions when multiple people (or multiple windows) open the same edit screen. Has anyone reviewed my option-based locking code?

comment:58 in reply to: ↑ 57 adamsilverstein16 months ago

Replying to mdawaffe:

fix data on opening the post for editing screen

I worry about race conditions when multiple people (or multiple windows) open the same edit screen. Has anyone reviewed my option-based locking code?

i originally placed the update call at pre_post_update thinking that would be a safer spot, but markjaquith pointed out that if we can update the data before the post edit screen displays, there is no need for the on the fly correction at all.

not sure about the locking code, needs review.

comment:59 follow-up: azaozz16 months ago

Wondering if we can use the actual post lock instead of an option. By the time the revisions version update functions are called, the post is already locked to the user.

wp_check_post_lock() would return false when checked for the current user but we can look at get_post_meta( $post->ID, '_edit_lock', true ) for the lock "freshness" (regardless of the user) and prevent updating the revisions if the post was locked a few seconds ago.

Edit: on second thought, that can miss if two requests come in at the exact same second, so probably not good enough.

Last edited 16 months ago by azaozz (previous) (diff)

comment:60 in reply to: ↑ 59 adamsilverstein16 months ago

Replying to azaozz:

Wondering if we can use the actual post lock instead of an option. By the time the revisions version update functions are called, the post is already locked to the user.

wp_check_post_lock() would return false when checked for the current user but we can look at get_post_meta( $post->ID, '_edit_lock', true ) for the lock "freshness" (regardless of the user) and prevent updating the revisions if the post was locked a few seconds ago.

Edit: on second thought, that can miss if two requests come in at the exact same second, so probably not good enough.

so, do you think the method used in the patch will work correctly?

comment:61 in reply to: ↑ 57 markjaquith16 months ago

Replying to mdawaffe:

I worry about race conditions when multiple people (or multiple windows) open the same edit screen. Has anyone reviewed my option-based locking code?

Looks like it uses time(), which is only granular to the second. Let's get some more granularity with number_format( microtime( true ), 10, '.', '' )

adamsilverstein16 months ago

comment:62 adamsilverstein16 months ago

in 16215.10.diff​:

  • changed time() to number_format( microtime( true ), 10, '.', ) for more precision
  • removed on the fly conversion function 'get_the_corrected_modified_author'
  • added call to _wp_upgrade_revisions_of_post in wp-admin/post.php

here is some post data showing whats happening:

initial data - no patch, create as admin, update as user 1, update as user 2 (with content matching user)

  • note post_author does not match author indicated in content updates; ID 170 & 171 should be post_author = 1, ID 172 should be post_author = 2; the current revision is missing

http://f.cl.ly/items/112g3I290W361H1T1F14/initial%20data%20after%203%20updates.png

got an auto save in there while testing:

http://f.cl.ly/items/0B252s1x1c0W0b3c3u0n/bad%20data%20before%20update.png

after applying patch and viewing post, note all post_author's are correct:

http://f.cl.ly/items/3n2s0Y1f2g052e3h3d0c/corrected%20data.png

after another update:

http://f.cl.ly/items/3l1O2m130T0g2C372z1M/corrected%20data%20with%20post.png

a new post created with same sequence, but after patch applied, note correct post_author data:

http://f.cl.ly/items/2S3Y051u2P2O2a1r231l/newpostwithpatch.png

just notice one needed update and adding...

comment:63 adamsilverstein16 months ago

16215.11.diff​ fixes an issue where the 1st revision saved after applying had the wrong post_author (unless you had an autosave) - this was the replacement missing current revision; removed post_updated action, instead wp_save_post_revision in _wp_upgrade_revisions_of_post;

tests below use sequence admin, user1, user2, upgrade, admin...

before newest patch, note 185 is off:

http://f.cl.ly/items/0r1S0e1S0H2M143Z031y/beforelatest.png

here is data from same sequence, now looks good

http://f.cl.ly/items/2H1h0s0k1h3x182u3f1s/withcurrent.png

adamsilverstein16 months ago

adamsilverstein16 months ago

comment:65 markjaquith16 months ago

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

In 23823:

Fix a longstanding "off by one" revision authorship bug.

  • Fixes old revision data on the fly when you open a post for editing.
  • Uses post_name of revisions to store a post version number (-v1), so we know what has been fixed.
  • Latest version should also have a revision stored, whereas before it did not.

props adamsilverstein, mdawaffe. fixes #16215.

comment:66 azaozz16 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Seeing some inconsistencies in 16215.12.diff:

  • If we are saving a new revision after saving the main post (the change of filter in default-filters.php), there's no need to use get_post_meta( $post['ID'], '_edit_last', true ); in _wp_post_revision_fields(). In this case the revision will be exact copy of the last saved post and will have the right author. This also is not needed for autosaves, they use _wp_post_revision_fields() too.
  • Each time we do:
if ( $revisions = wp_get_post_revisions( $post_id ) ) {
	// grab the last revision
	$last_revision = array_shift( $revisions );
}

the $last_revision could be an autosave. In some cases that works, in other, not.

  • _wp_upgrade_revisions_of_post() seems to be running each time a post is loaded for editing. It needs to run only once per post and update the revisions versions. Also some of the logic there is "backwards" :) First we save a revision, then check if the post_type supports revisions...

Edit: _wp_upgrade_revisions_of_post() also uses wp_update_post() to update revisions when fixing the author and revision version. This results in changing post_modified on all revisions to the current date/time. When listing them that looks pretty weird. Perhaps we can do a custom db query to only update the two fields that need updating?

Last edited 16 months ago by azaozz (previous) (diff)

comment:67 azaozz16 months ago

In 23830:

Fix wp_list_post_revisions() to include all autosaves when listing autosaves only and properly include or exclude the current post, see #23665, see #16215

azaozz16 months ago

comment:68 follow-up: azaozz16 months ago

  • Keywords dev-feedback removed

In 16215-13.patch:

  • Always update the revision version when updating the post authors.
  • Check if revisions have been updated and return early in _wp_upgrade_revisions_of_post().
  • Update the revisions by direct query to avoid resetting post_modified in wp_update_post() and making all revisions show as saved right now. A lower level API would have been good here :)
  • Removed copying the post to a revision when no revisions exist. Can be added after this conversion if needed.

comment:69 in reply to: ↑ 68 adamsilverstein16 months ago

Replying to azaozz:

In 16215-13.patch:

>
> - Always update the revision version when updating the post authors.
> - Check if revisions have been updated and return early in _wp_upgrade_revisions_of_post().
> - Update the revisions by direct query to avoid resetting post_modified in wp_update_post() and making all revisions show as saved right now. A lower level API would have been good here :)
> - Removed copying the post to a revision when no revisions exist. Can be added after this conversion if needed.
>

think you still need the wp_save_post_revision( $post->ID ); line (removed in 16215-13.patch), but haven't tested yet. i expect removing it will mess up some data:

the reason is that under the old system there was no revision matching the current post data, it was always saving the previous, but not the current post. this line adds a copy of the current post as a revision so the last post save data gets stored and can have its author data updated (it will be off initially because the current user might not match the user who made the last changes).

in other words, this call needs to happen before the revisions data is corrected, if we do it after we will have one revision with the wrong author data, or if it doesn't happen before the next update, we will have a gap with one missing revision as the new system saves a revision after the post update occurs so no current data revision will ever get saved.

Last edited 16 months ago by adamsilverstein (previous) (diff)

comment:70 follow-up: azaozz16 months ago

think you still need the wp_save_post_revision( $post->ID ); line... so the last post save data gets stored and can have its author data updated...

What's happening at the moment is:

  • New (last) revision is created matching the post data.
  • That revision has post_author set from post_meta '_edit_last' (so the author is correct) and -v1 in post_name.
  • When we get to the conversion code block, this revision is bypassed with:
    // 1 is the latest revision version, so we're already up to date 
    if ( 0 < $this_revision_version ) 
        continue;
    


We can still add that before the conversion but it won't affect it. I see it's convenient to have this in _wp_upgrade_revisions_of_post() so once that function runs, all the needed conversions are done.

Seems good place for it is right after we check whether the revisions for the current post have been converted so it doesn't run all the time. Then $revisions = wp_get_post_revisions( $post->ID ); is already set and we won't look at it while converting the rest of the revisions.

comment:71 in reply to: ↑ 70 adamsilverstein16 months ago

Replying to azaozz:

think you still need the wp_save_post_revision( $post-&gt;ID ); line... so the last post save data gets stored and can have its author data updated...

What's happening at the moment is:

  • New (last) revision is created matching the post data.
  • That revision has post_author set from post_meta '_edit_last' (so the author is correct) and -v1 in post_name.
  • When we get to the conversion code block, this revision is bypassed with:
    // 1 is the latest revision version, so we're already up to date 
    if ( 0 < $this_revision_version ) 
        continue;
    


We can still add that before the conversion but it won't affect it. I see it's convenient to have this in _wp_upgrade_revisions_of_post() so once that function runs, all the needed conversions are done.

Seems good place for it is right after we check whether the revisions for the current post have been converted so it doesn't run all the time. Then $revisions = wp_get_post_revisions( $post->ID ); is already set and we won't look at it while converting the rest of the revisions.

ok, great. i didn't realize the new last revision was already being created, as long as thats happening we are fine.

i did notice that removing the revision save entirely from pre_post_update removed the initial title only revision for new posts, which messed up the revision UI because it expects that revision (and its present on all old posts). see http://core.trac.wordpress.org/ticket/23497#comment:135

comment:72 follow-up: markjaquith16 months ago

We have unit test failures surrounding revisions. Some of them are indubitably because we're storing an extra revision now, so the expectation of the unit test is no longer valid. But some of them seem wrong in the wrong direction.

There were 7 failures:

1) Tests_Post_Revisions::test_revision_dont_save_revision_if_unchanged
Failed asserting that actual size 1 matches expected size 2.

/Users/mark/svn/wp-unit-tests/trunk/tests/post/revisions.php:67

2) Tests_Post_Revisions::test_revision_force_save_revision_even_if_unchanged
Failed asserting that actual size 1 matches expected size 2.

/Users/mark/svn/wp-unit-tests/trunk/tests/post/revisions.php:96

3) Tests_Post_Revisions::test_revision_diff_caps_post
Failed asserting that actual size 1 matches expected size 2.

/Users/mark/svn/wp-unit-tests/trunk/tests/post/revisions.php:175

4) Tests_Post_Revisions::test_revision_restore_caps_before_publish
Failed asserting that actual size 1 matches expected size 2.

/Users/mark/svn/wp-unit-tests/trunk/tests/post/revisions.php:284

5) Tests_Post_Revisions::test_revision_diff_caps_cpt
Failed asserting that 2 matches expected 1.

/Users/mark/svn/wp-unit-tests/trunk/tests/post/revisions.php:312

6) Tests_XMLRPC_wp_getRevisions::test_revision_count_for_auto_draft_post_creation
Failed asserting that actual size 1 matches expected size 0.

/Users/mark/svn/wp-unit-tests/trunk/tests/xmlrpc/wp/getRevisions.php:60

7) Tests_XMLRPC_wp_restoreRevision::test_revision_restored
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'edit1'
+'edit2'

/Users/mark/svn/wp-unit-tests/trunk/tests/xmlrpc/wp/restoreRevision.php:47

comment:73 follow-up: azaozz16 months ago

As far as I can see the above tests fail because they don't expect the extra revision added after saving the post.

Moving from 'pre_post_update' to 'post_updated' is a big change and there would probably be more bugs and inconsistencies. This also creates a copy of the post when it may not be needed, like publishing from QuickPress and PressThis and updating a post programmatically.

Another inconsistency: for some reason the guid for revisions is a permalink (instead of ?p=123 like drafts). That makes all revisions for a post have the same guid: 123-revision-v1.

Last edited 16 months ago by azaozz (previous) (diff)

comment:74 in reply to: ↑ 73 adamsilverstein16 months ago

Replying to azaozz:

As far as I can see the above tests fail because they don't expect the extra revision added after saving the post.

Moving from 'pre_post_update' to 'post_updated' is a big change and there would probably be more bugs and inconsistencies. This also creates a copy of the post when it may not be needed, like publishing from QuickPress and PressThis and updating a post programmatically.

Another inconsistency: for some reason the guid for revisions is a permalink (instead of ?p=123 like drafts). That makes all revisions for a post have the same guid: 123-revision-v1.

i agree, moving from saving revisions from pre to post update is a big change. i specifically asked nacin about this in IRC and the answer was essentially, 'now is the time'.

a current revision is needed when you want to add information to the current revision - whats currently stored in the db - as it is, you can't do this until the post is updated, because you don't have a revision yet. in addition, the old pre_ system introduced the bug we are correcting - old post data was saved ('authored') by the current user, hence authors off by one.

the current package of fixes brings the old data up to date (once), and starts keeps current revisions. plugin authors will have to adapt, but will hopefully cheer the new current revision.

comment:75 in reply to: ↑ 72 adamsilverstein16 months ago

gonna check, but i suspect http://core.trac.wordpress.org/attachment/ticket/23497/23497.42.diff fixes this

Replying to markjaquith:

We have unit test failures surrounding revisions. Some of them are indubitably because we're storing an extra revision now, so the expectation of the unit test is no longer valid. But some of them seem wrong in the wrong direction.

There were 7 failures:

1) Tests_Post_Revisions::test_revision_dont_save_revision_if_unchanged
Failed asserting that actual size 1 matches expected size 2.

/Users/mark/svn/wp-unit-tests/trunk/tests/post/revisions.php:67

2) Tests_Post_Revisions::test_revision_force_save_revision_even_if_unchanged
Failed asserting that actual size 1 matches expected size 2.

/Users/mark/svn/wp-unit-tests/trunk/tests/post/revisions.php:96

3) Tests_Post_Revisions::test_revision_diff_caps_post
Failed asserting that actual size 1 matches expected size 2.

/Users/mark/svn/wp-unit-tests/trunk/tests/post/revisions.php:175

4) Tests_Post_Revisions::test_revision_restore_caps_before_publish
Failed asserting that actual size 1 matches expected size 2.

/Users/mark/svn/wp-unit-tests/trunk/tests/post/revisions.php:284

5) Tests_Post_Revisions::test_revision_diff_caps_cpt
Failed asserting that 2 matches expected 1.

/Users/mark/svn/wp-unit-tests/trunk/tests/post/revisions.php:312

6) Tests_XMLRPC_wp_getRevisions::test_revision_count_for_auto_draft_post_creation
Failed asserting that actual size 1 matches expected size 0.

/Users/mark/svn/wp-unit-tests/trunk/tests/xmlrpc/wp/getRevisions.php:60

7) Tests_XMLRPC_wp_restoreRevision::test_revision_restored
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'edit1'
+'edit2'

/Users/mark/svn/wp-unit-tests/trunk/tests/xmlrpc/wp/restoreRevision.php:47

comment:76 markjaquith16 months ago

Problem: we're only saving revisions on updates. So the data from a wp_insert_post() is lost. Going to switch revision saving to the wp_insert_post hook to fix that.

comment:77 markjaquith16 months ago

In 23842:

Use the wp_insert_post hook to save revisions, so we save revisions for
an initial wp_insert_post() run, not just updates.

see #16215

comment:78 markjaquith16 months ago

In 1251/tests:

We now store a revision for the current version, so these counts must be bumped. see #16215

azaozz16 months ago

comment:79 azaozz16 months ago

In 16215-14.patch:

  • Bring back saving a copy of the current post while converting revisions (happens after the conversion now).
  • Fix wp_first_revision_matches_current_version() and rename to _wp_last_revision_matches_current_post().
  • Fix a bug where we may be comparing with an autosave but need to compare with the latest revision.
  • Simplify the revision post_name regex, use $post object where possible, etc.

comment:80 azaozz16 months ago

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

In 23849:

Post revisions:

  • Always update the revision version when updating post authors.
  • Check if revisions have been updated and return early.
  • Update the revisions by direct query to avoid resetting post_modified.
  • Fix a bug where we may be comparing with an autosave but need to compare with the latest revision.

Fixes #16215.

comment:81 kurtpayne16 months ago

In 1252/tests:

We now store a revision for the current version, so these counts must be bumped. see #16215.

This brings the XMLRPC revisions tests in line with the posts revisions tests in [UT1251].

comment:82 nacin16 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I have two feelings about _wp_upgrade_revisions_of_post():

  • It's actually a bit heavy. It seems to run on every admin edit, and causes queries for revisions and some other execution, even after a post has been upgraded.
  • It runs in a fairly random place. Couldn't it run in wp_insert_post(), possibly just as part of wp_save_post_revision()?

The locking is kind of weird, but I get its necessity.

mdawaffe, what do you think of the final code?

comment:83 azaozz16 months ago

It's actually a bit heavy. It seems to run on every admin edit, and causes queries for revisions and some other execution, even after a post has been upgraded.

Yeah, was looking into that too. _wp_upgrade_revisions_of_post() has to run before the revisions postbox is shown, so we show the corrected authors and have the extra revision in there.

We are calling wp_get_post_revisions() on every load of edit-form-advanced.php (when determining whether to show the postbox), perhaps we can move the call to _wp_upgrade_revisions_of_post() there. We can use the loaded revisions to check the version, then upgrade them if needed.

Another option is to check the version and upgrade on the first call to wp_get_post_revisions(), maybe using a static, etc.

Last edited 16 months ago by azaozz (previous) (diff)

comment:84 follow-up: azaozz16 months ago

In 16215-15.patch:

  • Move the call to _wp_upgrade_revisions_of_post() to wp_get_post_revisions() and run it after getting the revisions and checking their version, once per post.

azaozz16 months ago

comment:85 in reply to: ↑ 84 nacin16 months ago

Replying to azaozz:

In 16215-15.patch:

  • Move the call to _wp_upgrade_revisions_of_post() to wp_get_post_revisions() and run it after getting the revisions and checking their version, once per post.

While I'd tend to think that wp_get_post_revisions() should be accurate and thus is a great place to run the upgrade code, that also means we might be running upgrade code — that does write operations that requires a lock — on the frontend. No bueno.

comment:86 azaozz16 months ago

...that also means we might be running upgrade code — that does write operations that requires a lock — on the frontend.

Right, it's a possibility we have to avoid. Disregard 16215-15.patch​.

Seems best place would be in edit-form-advanced.php after we load the revisions.

azaozz16 months ago

comment:87 azaozz16 months ago

In 16215-16.patch:

  • Move the call to _wp_upgrade_revisions_of_post() to edit-form-advanced.php, in the code block checking whether we should show the revisions postbox.

comment:88 kurtpayne15 months ago

In 1254/tests:

First revision is empty, ignore it

See #16215, r23842

comment:89 nacin15 months ago

[23842] correctly ensures that a first call to wp_insert_post() inserts the first revision. But, this should not happen on auto-drafts. (Per IRC discussion.) To fix this, in get_default_post_to_edit(), we can simply remove then re-add this filter.

comment:90 azaozz15 months ago

In 23929:

Revisions: move the call to _wp_upgrade_revisions_of_post() to edit-form-advanced.php, in the code block checking whether we should show the revisions postbox. See #16215

comment:91 azaozz15 months ago

...first call to wp_insert_post() inserts the first revision. But, this should not happen on auto-drafts.

wp_save_post_revision() bails on 'auto-draft' == $post->post_status, so that's working properly. Wondering if we should remove and re-add the action in get_default_post_to_edit() anyway.

Last edited 15 months ago by azaozz (previous) (diff)

comment:92 azaozz15 months ago

In 23933:

Revisions: don't set revisions post_author with get_post_meta( $ID, '_edit_last' ). Not needed since we create revisions after saving the post and breaks per-user autosaves, see #16215

comment:93 iandunn15 months ago

  • Cc ian_dunn@… added

comment:94 azaozz15 months ago

In 24025:

Revisions: when upgrading the revision author don't add a copy of the post as latest revision when some of the revisions are already at v1. See #16215

comment:95 azaozz15 months ago

In 24026:

Revisions: look at the version of the earliest revision when checking whether to upgrade the revisions authors, see #16215

comment:96 ocean9014 months ago

Related IRC log from dev chat.

comment:97 ocean9013 months ago

Calling as fixed for now.

comment:98 ocean9013 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.