Opened 14 years ago
Closed 11 years 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:
- Create and Publish a new post as user admin.
- Edit that post and Update as user mdawaffe.
- 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.
- 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).
- 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:
- 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.
- Post meta. Easy, but adds one post meta per post just to fix a lame bug.
- 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)
Change History (123)
#2
@
14 years ago
Thanks for finally giving this proper attention, guys. It's caused numerous problems when trying to figure out who edited what.
#3
@
14 years ago
For the record, nacin has closed my FIFTEEN MONTH OLD TICKET as a duplicate of this one.
#4
@
14 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.)
#7
@
14 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?
#8
@
13 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.
#9
@
13 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.
@
13 years ago
First pass. New revisions: correct info DB. Old revisions: fix on the fly for display only
#12
@
13 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 duringwp_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.
#13
@
13 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
#15
@
13 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.
#16
@
12 years ago
- Cc aaroncampbell added
- Keywords 3.4-early removed
- Milestone changed from Future Release to 3.6
#17
@
12 years ago
- Keywords revisions-3.6 added
- Owner set to westi
- Status changed from new to accepted
Can we fix this?
Yes we can!
#19
follow-up:
↓ 21
@
12 years 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?
#20
@
12 years 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.
#21
in reply to:
↑ 19
@
12 years 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.
#24
@
12 years 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! :}
#26
@
12 years 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.
#27
follow-up:
↓ 28
@
12 years ago
- 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 inwp_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:
- New blog with 3 users running unpatched trunk.
- Create a new post as user_id=1.
- Edit the post as user_id=2.
- Edit the post as user_id=3.
- Edit the post as user_id=1.
- Edit the post as user_id=2.
- Edit the post as user_id=3.
- View the revisions for that post. Notice how they are off by one.
- Apply the patch.
- View the revisions for that post. Notice how they are correct.
- Edit the post as user_id=1.
- Edit the post as user_id=2.
- Edit the post as user_id=3.
- View the revisions for that post. Notice how they are correct.
- Revert the patch.
- View the revisions for that post. Notice how the revisions created before the patch was applied are off by one.
- Edit the post as user_id=1.
- Edit the post as user_id=2.
- Edit the post as user_id=3.
- Apply the patch.
- 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).
- Run the upgrade function on that post.
- 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.
#28
in reply to:
↑ 27
@
12 years ago
Replying to mdawaffe:
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.
#29
@
12 years 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.
#30
@
12 years 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.
#31
@
12 years ago
In 1210/tests:
#32
follow-ups:
↓ 33
↓ 37
@
12 years 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
#33
in reply to:
↑ 32
@
12 years 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.
#34
@
12 years 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.
#35
@
12 years 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
#36
@
12 years 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.5.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
#37
in reply to:
↑ 32
@
12 years 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
#38
@
12 years 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
#39
@
12 years 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
#40
@
12 years 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)
#42
in reply to:
↑ 41
@
12 years 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).
#43
@
12 years 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.
#45
in reply to:
↑ 44
@
12 years 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.
#46
@
12 years 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
#47
follow-up:
↓ 49
@
12 years 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.
#48
follow-up:
↓ 50
@
12 years ago
When a new revision is saved, the old incorrectly attributed revisions aren't fixed.
#49
in reply to:
↑ 47
;
follow-up:
↓ 51
@
12 years 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.
#50
in reply to:
↑ 48
@
12 years 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.
#51
in reply to:
↑ 49
@
12 years 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.
#52
follow-up:
↓ 54
@
12 years ago
I do think it's needed. It's pretty terrible that our revisions have bad data.
#53
@
12 years 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)
then applied patch, edited post again as user1, note corrected data:
#54
in reply to:
↑ 52
@
12 years 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.
#55
@
12 years ago
The tooltip author is the wrong author when fixed on the fly. Looks good once fixed.
#56
@
12 years 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
#57
follow-ups:
↓ 58
↓ 61
@
12 years 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?
#58
in reply to:
↑ 57
@
12 years 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.
#59
follow-up:
↓ 60
@
12 years 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.
#60
in reply to:
↑ 59
@
12 years 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 atget_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?
#61
in reply to:
↑ 57
@
12 years 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, '.', '' )
#62
@
12 years 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
got an auto save in there while testing:
after applying patch and viewing post, note all post_author's are correct:
after another update:
a new post created with same sequence, but after patch applied, note correct post_author data:
just notice one needed update and adding...
#63
@
12 years 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:
here is data from same sequence, now looks good
#64
@
12 years ago
http://core.trac.wordpress.org/attachment/ticket/16215/16215.12.diff refresh against trunk
#66
@
12 years 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?
#68
follow-up:
↓ 69
@
12 years 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.
#69
in reply to:
↑ 68
@
12 years 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.
#70
follow-up:
↓ 71
@
12 years 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.
#71
in reply to:
↑ 70
@
12 years ago
Replying to azaozz:
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.
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
#72
follow-up:
↓ 75
@
12 years 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
#73
follow-up:
↓ 74
@
12 years 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.
#74
in reply to:
↑ 73
@
12 years 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 sameguid
: 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.
#75
in reply to:
↑ 72
@
12 years 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
#76
@
12 years 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.
#78
@
12 years ago
In 1251/tests:
#79
@
12 years 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.
#81
@
12 years ago
In 1252/tests:
#82
@
12 years 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?
#83
@
12 years 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.
#84
follow-up:
↓ 85
@
12 years ago
In 16215-15.patch:
- Move the call to
_wp_upgrade_revisions_of_post()
towp_get_post_revisions()
and run it after getting the revisions and checking their version, once per post.
#85
in reply to:
↑ 84
@
12 years ago
Replying to azaozz:
In 16215-15.patch:
- Move the call to
_wp_upgrade_revisions_of_post()
towp_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.
#86
@
12 years 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.
#87
@
12 years 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.
#88
@
12 years ago
In 1254/tests:
#89
@
12 years 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.
#91
@
12 years 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.
Sweet. As discussed, I like menu_order too.
I guess #11022 illustrated the issue as well.