Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 3 years ago

#26042 closed defect (bug) (fixed)

wp_save_post_revision() can compare against the wrong $last_revision post

Reported by: wonderboymusic's profile wonderboymusic Owned by: wonderboymusic's profile 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)

26042.diff (570 bytes) - added by wonderboymusic 10 years ago.
revisions.php.patch (891 bytes) - added by nunomorgadinho 10 years ago.
26042.2.diff (575 bytes) - added by nunomorgadinho 10 years ago.
26042.3.diff (742 bytes) - added by SergeyBiryukov 10 years ago.
26042.4.diff (1.8 KB) - added by SergeyBiryukov 10 years ago.
26042.5.diff (1.5 KB) - added by wonderboymusic 10 years ago.
26042.6.diff (1.3 KB) - added by SergeyBiryukov 10 years ago.

Download all attachments as: .zip

Change History (40)

#1 follow-up: @dd32
10 years ago

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)

#2 follow-up: @nacin
10 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.

#3 @ocean90
10 years ago

#26219 was marked as a duplicate.

#4 @nunomorgadinho
10 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?

#5 @nunomorgadinho
10 years ago

  • Cc nuno.morgadinho@… added

#6 @wonderboymusic
10 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 @kurtpayne
10 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.

#8 @wonderboymusic
10 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 3.9

#9 @wonderboymusic
10 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 27184:

wp_get_post_revisions() should have a default orderby of date ID to tie-break rapidly added revisions - namely, when unit tests are run. There is a test that occasionally fails when running all tests, and always fails when running phpunit tests/phpunit/tests/post/revisions.php . This fixes that.

Fixes #26042.

#10 @wonderboymusic
10 years ago

Forgot to props nunomorgadinho on this. I don't know the protocol here...

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


10 years ago

#12 @kpdesign
10 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 @SergeyBiryukov
10 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

Last edited 10 years ago by SergeyBiryukov (previous) (diff)

#14 in reply to: ↑ 2 @SergeyBiryukov
10 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.

Last edited 10 years ago by SergeyBiryukov (previous) (diff)

#15 in reply to: ↑ 1 @SergeyBiryukov
10 years ago

Replying to dd32:

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)

Sounds like #23923.

#16 @SergeyBiryukov
10 years ago

#23923 was marked as a duplicate.

#17 @SergeyBiryukov
10 years ago

26042.3.diff fixes both the revisions order in the UI and random unit test failures for me.

#18 @SergeyBiryukov
10 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.


10 years ago

#20 in reply to: ↑ 19 @adamsilverstein
10 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.


10 years ago

#22 @azaozz
10 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 :)

Last edited 10 years ago by azaozz (previous) (diff)

#23 @DrewAPicture
10 years ago

FWIW, I agree on reverting [27174] this late in the cycle. We can always circle back in 4.0-early.

#24 @wonderboymusic
10 years ago

In 27683:

Revert [27184] until WP_Query is patched to apply order to each space-delimited orderby value properly.

See #26042.

#25 @wonderboymusic
10 years ago

A note: wp_get_post_revisions() calls get_children() which 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.

Last edited 10 years ago by wonderboymusic (previous) (diff)

#26 follow-up: @wonderboymusic
10 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 @SergeyBiryukov
10 years ago

Replying to wonderboymusic:

We need to attack the assigning of order properly to each orderby in WP_Query when multiple are specified.

See #24133 (closed a duplicate of #17065, but might be worth reopening and handling separately).

#28 @wonderboymusic
10 years ago

#28241 was marked as a duplicate.

#29 @wonderboymusic
10 years ago

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

In 28541:

Apply order to each passed value for orderby in WP_Query:

  • Since orderby in WP_Query can accept space-delimited sets, yet only one order value: when multiple values are passed (and DESC is the order), the default sort order ASC is being applied to all values before the last in the set.
  • There is a unit test that sporadically fails since 3.6 in tests/post/revision due to multiple posts having the same post_date from being added so rapidly
  • When ordering revisions in wp_get_post_revisions(), order by post_date ID
  • Change the order value in wp_get_post_revisions() to ASC. This will produce SQL like: ORDER BY $wpdb->posts.post_date ASC, $wpdb->posts.ID ASC. Previously, this would have produced SQL like: ORDER BY $wpdb->posts.post_date DESC, and with the addition of ID: ORDER BY $wpdb->posts.post_date ASC, $wpdb->posts.ID DESC. Clearly, wrong. The original SQL produced: ORDER BY $wpdb->posts.post_date DESC. As such, return the reversions in reverse order using array_reverse(). Not doing so would break "Preview Changes."
  • Add unit tests to assert that all of this works.
  • All existing unit tests pass with the change to ordering multiple orderbys in WP_Query.
  • In the future, we should support independent order for each orderby, see #17065.

Props SergeyBiryukov, wonderboymusic.
Fixes #26042.

#30 @wonderboymusic
10 years ago

  • Milestone changed from Future Release to 4.0

#31 @SergeyBiryukov
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.

#32 @SergeyBiryukov
10 years ago

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

In 28543:

Remove unnecessary array_reverse() from wp_get_post_revisions().

WP_Query properly handles multiple 'orderby' values since [28541].

fixes #26042.

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.