#26042 closed defect (bug) (fixed)
wp_save_post_revision() can compare against the wrong $last_revision post
Reported by: | wonderboymusic | Owned by: | wonderboymusic |
---|---|---|---|
Milestone: | 4.0 | Priority: | normal |
Severity: | major | Version: | |
Component: | Revisions | Keywords: | has-patch needs-unit-tests commit 4.0-early |
Focuses: | Cc: |
Description
There is a unit test that now consistently fails: Tests_Post_Revisions::test_revision_dont_save_revision_if_unchanged()
. After some substantial debugging, I have determined that ordering post revisions by post_date DESC
is completely unreliable when posts are added within seconds or milliseconds of each other, even when post_date
is explicitly set to increment on wp_update_post()
because the revisions still occur with milliseconds of each other, regardless of what the post's post_date
is.
When all revisions occur in nearly unison, ordering becomes moot. Sometime the revisions return DESC
. Sometimes they awesomely return in ASC
order, which makes the $last_revision
that $post
is compared against the first revision.
The only way to combat this is to sort in wp_get_post_revisions()
by ID DESC
. All of the unit tests pass with this change. Is there any reason we can't rely on sequential posts DESC
for ordering?
Attachments (7)
Change History (40)
#2
follow-up:
↓ 14
@
11 years ago
We could sort by both, 'orderby' => 'date ID'
should be fine, since we want DESC for both. We do this in some other places.
Great find, this has been bothering a number of us for months.
#4
@
11 years ago
The problem seems to be in wp-includes/revision.php (around line 95) in that last_revision is wrongly set when posts are added within milliseconds of each other:
if ( $revisions = wp_get_post_revisions( $post_id ) ) { // grab the last revision, but not an autosave foreach ( $revisions as $revision ) { if ( false !== strpos( $revision->post_name, "{$revision->post_parent}-revision" ) ) { $last_revision = $revision; break; } }
How about grabbing the last element of the revisions array as the last_revision, wouldn't that work?
#6
@
11 years ago
No, it wouldn't, because you have no idea what order they returned, and it isn't consistent - ordering by post_date, ID DESC would work
#7
@
11 years ago
- Cc kurtpayne added
Confirmed this is correct and sane. Fix addresses the correct problem in the correct way. This test passes now for me on windows and linux, in isolation and in aggregate.
#9
@
11 years ago
- Owner set to wonderboymusic
- Resolution set to fixed
- Status changed from new to closed
In 27184:
This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.
11 years ago
#12
@
11 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Per the IRC discussion linked above, the revisions order has changed in the Revisions metabox and the Revisions browser, from displaying the newest first to displaying the oldest first in latest trunk.
I tracked down the behavior change to r27184. Removing the ID reverted the order back to newest first for both.
I also checked 3.8 to confirm that displaying newest first for both was the expected behavior for revisions.
#13
@
11 years ago
Also, revision tests (specifically, test_revision_dont_save_revision_if_unchanged()
and test_revision_restore_updates_edit_last_post_meta()
) were still intermittently failing even after [27184]:
https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2014-02-27&sort=asc#m800788
#14
in reply to:
↑ 2
@
11 years ago
Replying to nacin:
We could sort by both,
'orderby' => 'date ID'
should be fine, since we want DESC for both. We do this in some other places.
FWIW, I don't see 'orderby' => 'date ID'
anywhere else in core.
I don't think it works as expected, due to #17065, as noted in comment:1:ticket:23923.
The clause it generates is:
ORDER BY trunk_posts.post_date, trunk_posts.ID DESC
According to MySQL reference, DESC
only applies to trunk_posts.ID
here.
#17
@
11 years ago
26042.3.diff fixes both the revisions order in the UI and random unit test failures for me.
#18
@
11 years ago
- Keywords commit added
26042.4.diff includes kovshenin's test from #23923, which consistently fails without the change in 26042.3.diff and passes with it.
This ticket was mentioned in IRC in #wordpress-dev by wonderboymusic. View the logs.
11 years ago
#20
in reply to:
↑ 19
@
11 years ago
Replying to ircbot:
This ticket was mentioned in IRC in #wordpress-dev by wonderboymusic. View the logs.
So the root cause of this issue is that the query isn't including all of the sort orders passed? Does this also correct the unit tests discussed above? I will do some testing and report back, thanks!
This ticket was mentioned in IRC in #wordpress-dev by kimparsell. View the logs.
11 years ago
#22
@
11 years ago
- Severity changed from normal to blocker
[27184] breaks "Preview Changes" for published posts as revisions are not in reverse chronological order any more (as explained by @SergeyBiryukov in comment 14).
Unless we fix wp_query, there's no way to order by two fields DESC. At this point (beta2) I'd rather revert [27184] than touch wp_query :)
#23
@
11 years ago
FWIW, I agree on reverting [27174] this late in the cycle. We can always circle back in 4.0-early.
#25
@
11 years ago
A note: wp_get_post_revisions()
calls get_children()
while calls get_posts()
which suppresses filters. So, even if we wanted to, we couldn't filter the query here, which it what I was gonna try to do.
#26
follow-up:
↓ 27
@
11 years ago
- Keywords 4.0-early added
- Milestone changed from 3.9 to Future Release
- Severity changed from blocker to major
We need to attack the assigning of order
properly to each orderby
in WP_Query
when multiple are specified. To big of a change for now, punting. Reverted the offending change in [27683].
#27
in reply to:
↑ 26
@
11 years ago
Replying to wonderboymusic:
We need to attack the assigning of
order
properly to eachorderby
inWP_Query
when multiple are specified.
See #24133 (closed a duplicate of #17065, but might be worth reopening and handling separately).
#31
@
10 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Now that WP_Query
generates correct SQL, we don't need the array_reverse()
workaround: 26042.6.diff.
ID could cause problems with some multi-master configurations for a start (where auto_incr is different between the two).
I'm also fairly certain this has come up before and there's another ticket, but all my searching is turning up blanks (but I've certainly heard this exact issue, and suggested fix elsewhere)