wp_save_post_revision() can compare against the wrong $last_revision post

[wonderboymusic]

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?

[dd32]

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)

[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.

Great find, this has been bothering a number of us for months.

[ocean90]

#26219 was marked as a duplicate.

[nunomorgadinho]

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?

[wonderboymusic]

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

[kurtpayne]

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.

[wonderboymusic]

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.

[wonderboymusic]

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

[kpdesign]

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.

[SergeyBiryukov]

Also, revision tests were still intermittently failing even after [27184]:
​https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2014-02-27&sort=asc#m800788

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

#23923 was marked as a duplicate.

[SergeyBiryukov]

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

[SergeyBiryukov]

26042.4.diff​ includes kovshenin's test from #23923, which consistently fails without the change in 26042.3.diff​ and passes with it.

[adamsilverstein]

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!

[azaozz]

[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 :)

[DrewAPicture]

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

[wonderboymusic]

In 27683:

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

See #26042.

[wonderboymusic]

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.

[wonderboymusic]

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].

[SergeyBiryukov]

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).

[wonderboymusic]

#28241 was marked as a duplicate.

[wonderboymusic]

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.

[SergeyBiryukov]

Now that WP_Query generates correct SQL, we don't need the array_reverse() workaround: 26042.6.diff​.

[SergeyBiryukov]

In 28543:

Remove unnecessary array_reverse() from wp_get_post_revisions().

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

fixes #26042.