get_adjacent_post() should use WP_Query rather than build its own SQL query

[ethitter]

With the introduction of the WP_Date_Query through r25139, get_adjacent_post() no longer needs to build its own SQL to retrieve adjacent posts. By switching to WP_Query, we gain the benefit of its performance improvements, including native caching.

The trickiest part of this change is maintaining support for the get_{$adjacent}_post_join and get_{$adjacent}_post_where filters currently applied to the SQL built in get_adjacent_post().

[ethitter]

26937.patch​ switches get_adjacent_post() to use WP_Query. It does not address the filters issue noted in the original ticket.

The relevant unit tests introduced as part of #17807 are passing with 26937.patch​ applied.

[ethitter]

26937.2.patch​ includes the following:

• Rewrites get_adjacent_post() to leverage WP_Query.
• Incorporates support for filters previously applied to get_adjacent_post()'s SQL clauses as discussed in the previous IRC conversation linked from this ticket. Supported filters are get_previous_post_where, get_previous_post_join, get_previous_post_sort, and their "next" counterparts.
• Adds unit tests for the filter backwards compatibility. The conversion to WP_Query is covered by the existing tests from #17807.

The included tests, plus those introduced as part of #17807, are passing at this time.

[nacin]

This is awesome. Most excellent work.

The only thing I see is remove() should probably occur immediately, within the post_clauses callback. This isn't just for academic or style reasons — if someone else runs a WP_Query on a hook of WP_Query (careful to avoid recursion, etc.), this would stomp over that.

I also could picture each clause manipulation being its own protected method, with filter() handling the has_filter() checks. Just to split up the logic a bit. If it doesn't make it easier to read/grok though, please push back on it.

[ethitter]

Replying to nacin:

This is awesome. Most excellent work.

Thanks!

The only thing I see is remove() should probably occur immediately, within the post_clauses callback. This isn't just for academic or style reasons — if someone else runs a WP_Query on a hook of WP_Query (careful to avoid recursion, etc.), this would stomp over that.

Done in 26937.4.patch​. Nice catch!

I also could picture each clause manipulation being its own protected method, with filter() handling the has_filter() checks. Just to split up the logic a bit. If it doesn't make it easier to read/grok though, please push back on it.

I went ahead and split the manipulations into their own methods in 26937.4.patch​. I'm not sure it improves readability, but I'm also not convinced it makes things less comprehensible. If no one expresses any strong feelings either way, I'm happy with leaving the methods split as they are in the latest patch.

[ethitter]

Replying to ethitter:

As I've thought about this more, I like the separation provided by splitting the manipulation from the checks for the need for manipulation. Given the relative complexity of the sort handling compared to the join and where cases, the approach reflected in 26937.4.patch​ seems better.

[ethitter]

Following discussions with Nacin, I've completely overhauled the approach to adjacent post handling. 26937.5.patch​ introduces a class that handles the adjacent post determination and makes get_adjacent_post() nothing more than a wrapper for said class. The new class allows for much greater flexibility, as it accepts a post object/ID/array for which to determine the desired adjacent post, which wasn't possible previously. As part of this patch, get_adjacent_post() gains the ability to specify a post, thanks to the class's capabilities.

For those keeping track at home, the WP_Get_Adjacent_Post::get_post() method introduced in 26937.5.patch​ is comprised of the functionality previously found in get_adjacent_post() in 26937.4.patch​. Some input validation is moved to the class constructor from get_adjacent_post(), but otherwise, the code is unchanged, just rearranged.

[nacin]

In 27286:

Make get_adjacent_post() wrap a new WP_Get_Adjacent_Post object that uses WP_Query.

See [27285] for the actual commit, which misfired.

props ethitter!
fixes #26937.

[nacin]

In 27287:

We shall call it WP_Adjacent_Post instead. see #26937.

[nacin]

In 27288:

One more change. see #26937.

[danielbachhuber]

Should we make it final, per #24672?

[nacin]

#24672 is pretty specific to allowing our all-important models to continue to evolve without worrying about breaking plugins. Other classes are case-by-case. I don't think there is much of a use case in extending WP_Adjacent_Post, but if someone wanted to, it doesn't matter to me that much.

[DrewAPicture]

Working on docs fixes for this.

[DrewAPicture]

Docs fixes

[DrewAPicture]

In 27291:

Add inline documentation for properties and other inline docs fixes for WP_Adjacent_Post.

See #26937.

[DrewAPicture]

In 27292:

Remove now-moot vanity spacing for WP_Adjacent_Post property definitions.

See #26937.

[DrewAPicture]

In 27293:

Document the $taxonomy property in WP_Adjacent_Post.

See #26937.

[nacin]

In 27296:

Set the taxonomy property in the WP_Adjacent_Post class. fixes #26937.

[Funkatronic]

I was wondering if the query args filter could have a post type-specific equivilent? IE get_adjacent_post_query_args_{$post_type} or get_adjacent_{$post_type}_query_args

[kovshenin]

Noticing some breakage here with themes and plugins.

While the get_*_post_* filters seem to be backwards compatible, they're a little different than what some plugins expect: the whole WHERE statement is filtered pre r27285, so plugins that write their own WHERE statements (as opposed to appending conditions to the clause) will end up with an invalid SQL.

Also, the former SQL statement declared a p alias for $wpdb->posts, but WP_Query doesn't do that, so any references to fields in where and join filters are broken.

Here's some sample code that illustrates both problems:

add_filter( 'get_previous_post_where', function( $where ) {
	global $wpdb;
	$post = get_post();
	$where = $wpdb->prepare( " WHERE p.post_date < %s AND p.post_type IN ( 'post', 'page' ) ", $post->post_date );
	return $where;
});

[jcastaneda]

It appears that when using next/previous_post_link it actually directs to a sticky post if there is one.

[afercia]

Inspecting DB queries, I can count several additional ones related to adjacent links.
In my local test environment, using _s as theme, theme unit test data, no plugins etc. I usually have just 4 queries related to prev/next links called by adjacent_posts_rel_link_wp_head(), previous_post_link() and next_post_link().

But now I can count 16 queries related to next/prev links, 8 of them just for adjacent_posts_rel_link_wp_head().

Then, when I mark a post (doesn't matter which one, just a random post but not the one I'm currently inspecting) as "sticky" I can see 8 more queries... a total of 16.
Moreover, both previous_post_link() and next_post_link() will print out a link to the sticky post. If more than one sticky post, the most recent one will be printed out.

I fear this new class runs multiple times and does something even on the retrieved prev/next post objects, just my 2 cents but I really don't have time to investigate deeper.

To recap, in my usual test post which is, guess it, "Post Format: Standard", using _s theme with theme unit test data with no menus, no widgets etc. nothing else on the post other than post content, now I have a total of 29 queries.
As soon as I revert back to previous 'link-template.php' version, I have a total of 13 queries, just 4 of them related to prev/next links.

[BinaryMoon]

Replying to jcastaneda:

It appears that when using next/previous_post_link it actually directs to a sticky post if there is one.

I'm seeing this behaviour on my dev install as well. get_previous_post and get_next_post direct me to sticky posts.

[nacin]

26937.2.diff​ reduces the number of queries to what they were in 3.8. It should also fix sticky post issues, though that is untested.

• Still need to investigate if there are any additional issues for posts published at the same time, or something else triggering looping etc.

• Still need to investigate filter BC, per kovshenin.

[jcastaneda]

Does fix the sticky post issues, and I scheduled two posts to be published at the same time to see if it would create an issue. Didn't happen. I haven't tried to filter yet.

[afercia]

please correct me if I'm wrong
with 'ignore_sticky_posts' => true are you completely excluding sticky posts from posts navigation? when I revert back, I have my sticky posts in the post navigation and that make sense since if a theme makes use of prev/next links, you want to navigate through posts chronologically.

Maybe it would be better to let theme authors make a decision whether to include or not sticky posts in posts navigation?

Btw, using _s theme, I still can count 2 additional queries, now on my test page I have a total of 15 queries, when I revert back they're 13. Testing with different themes, I get even more queries related to adjacent posts, I suppose this varies depending on the posts navigation function used.

With _s, I can see 6 queries related to adjacent posts:

• 2 called by adjacent_posts_rel_link_wp_head
• 4 called by _s_post_nav

seems to me the 4 ones called by _s_post_nav are duplicated, called a first time by WP_Adjacent_Post and then again by previous_post_link/next_post_link which end up calling again WP_Adjacent_Post which calls again get_posts...
Not a SQL expert here sorry I can't help more. See attachment for duplicated queries details.

[BinaryMoon]

Replying to afercia:

please correct me if I'm wrong
with 'ignore_sticky_posts' => true are you completely excluding sticky posts from posts navigation? when I revert back, I have my sticky posts in the post navigation and that make sense since if a theme makes use of prev/next links, you want to navigate through posts chronologically.

The way it was (incorrectly) setup the next and previous links always pointed to the same post on every post because the sticky post was being pushed to the front of the que in both instances.

The correct way would be to ignore the stickiness and just sort things chronologically. Sticky posts will still show, they just won't be at the front every time.

[afercia]

Replying to BinaryMoon:

The correct way would be to ignore the stickiness and just sort things chronologically. Sticky posts will still show, they just won't be at the front every time.

thx BinaryMoon, to be honest I forgot that :)

[ethitter]

26937.7.patch​ builds on Nacin's patch from 26937.2.diff​, adding the aliasing of wp_posts as p in the resulting SQL. 26937.7.patch​ also updates the related unit tests, which are passing.

Nacin's patch fixed the sticky post issue in my testing.

I'm reviewing the backwards compatibility issues with the WHERE filter noted by Kovshenin.

I haven't yet checked into the potential repetition or looping when posts are published with identical timestamps, but since the date restriction in the query just uses WP_Date_Query, I fear the problem may exist outside of the WP_Adjacent_Post class's use of WP_Query.

[ethitter]

Replying to ethitter:

I'm reviewing the backwards compatibility issues with the WHERE filter noted by Kovshenin.

I can't believe I overlooked this detail. :(

The old filter passed the entire clause (ref), with the WHERE prefixed, whereas WP_Query doesn't include the WHERE in the filters I used since it's added later on in the process of building the final query (ref).

One option is to use the posts_request filter instead, which is applied to the final query, to split the query out to the three pieces needed for the old filters: JOIN, WHERE (including GROUP BY), and ORDER BY (including LIMIT). I'm not keen on the additional string manipulation that this would require, as it increases the room for error when mapping the legacy filters' return values back to the relevant WP_Query filters.

Another possible solution, and the one I'm inclined towards, is to simply prepend WHERE 1=1 to the relevant clause as it goes to the legacy filter, and strip out any WHERE that the filter returns, while also ensuring the filter's return value starts with AND as it does initially in WP_Query. This approach requires considerably-less string manipulation, which seems a plus given there's already enough of that in the filter_sort() method.

[obenland]

Ignore sticky posts

[obenland]

Currently sticky posts aren't excluded which causes adjacent posts to be sticky posts mostly. See 26937.8.patch​.

[ethitter]

Replying to obenland:

Currently sticky posts aren't excluded which causes adjacent posts to be sticky posts mostly. See 26937.8.patch​.

Right, my original implementation missed the ignore_sticky_posts query option. Nacin's patch, which my latest patch incorporates, includes it.

[obenland]

#27461 was marked as a duplicate.

[ethitter]

Replying to ethitter:

Another possible solution, and the one I'm inclined towards, is to simply prepend WHERE 1=1 to the relevant clause as it goes to the legacy filter, and strip out any WHERE that the filter returns, while also ensuring the filter's return value starts with AND as it does initially in WP_Query. This approach requires considerably-less string manipulation, which seems a plus given there's already enough of that in the filter_sort() method.

26937.10.patch​ implements the above-described approach to the WHERE clause backcompat. I've added an additional unit test for this particular situation.

Of the issues that triggered this ticket's reopening, only the possibility of repetition or looping needs to be investigated still. The remaining issues are covered as part of 26937.10.patch​.

[ethitter]

Replying to nacin:

Still need to investigate if there are any additional issues for posts published at the same time, or something else triggering looping etc.

I created three posts with identical publish times. Loading any of the three posts didn't lead to any sort of loop or recursion in the queries. Comparing the order from the post list table (post-list.png​) to the display per the Twenty Thirteen theme (twenty-thirteen.png​), the order matches. I don't see any indication that the current WP_Query arguments are going to trigger any looping.

[nacin]

In 27633:

WP_Query: allow split_the_query = false to avoid a split.

see #26937.

[nacin]

In 27634:

Return a bool from wp_using_ext_object_cache(), never a null.

see #26937.

[nacin]

In 27635:

Fix various issues with WP_Adjacent_Post:

• Performance / number of queries.
• Incorrect results caused by sticky posts.
• Back compat for filters, which had used "WHERE" while WP_Query does not; and fixing table references.

props ethitter.
fixes #26937.

[ethitter]

Fix adjacency for non-"post" post types

[ethitter]

26937.11.patch​ ensures that, by default, the current post object's post type is respected when determining the adjacent post. This corrects a regression introduced in r27635.

Tests are updated accordingly, using the page post type to test post-type specific adjacency.

[nacin]

26937.11.patch​ looks good. Doesn't appear to be a regression in [27635], but in the original commit, [27285].

[nacin]

In 27692:

Use the current post's post type when determining post adjacency.

props ethitter.
fixes #26937.

[ethitter]

Replying to nacin:

26937.11.patch​ looks good. Doesn't appear to be a regression in [27635], but in the original commit, [27285].

Ah, yes. Copy-paste fail. :)

[kovshenin]

This is now much slower than it used to be.

3.8	No ext object caching	350ms
3.8	Memcached object cache	310ms
trunk	No ext object caching	1200ms
trunk	Memcached object cache	1300ms

I used a real import with about 600 posts from my personal blog and ​this snippet to time them. I dug a bit deeper into how we use WP_Query and it seems like we have two separate issues.

The first issue is WP_Query itself, it looks like most of the time is spent parsing the query rather than querying the database. I'm not sure how to address this problem.

The second issue is the query itself. In 3.8 we do:

SELECT p.ID FROM wp_posts ...
SELECT * FROM wp_posts WHERE ID = 123 LIMIT 1

But in trunk we currently run:

SELECT wp_posts.* FROM wp_posts ...

If you put persistent object caching in the mix then the second query in 3.8 has a very high chance of being served from cache. With the above test and with 26937.3.diff​ I was able to get a 300ms reduction in trunk with Memcached.

[nacin]

In 3.8 we did two separate queries always. In 3.9 I made it a bit more clever by bypassing the query splitting if an external object cache was off. See 'split_the_query' => wp_using_ext_object_cache(). If an object cache is on, then it should be two separate queries. That's what I'm seeing with APC.

[kovshenin]

Nacin is right. It does indeed split the query with external object cache so my patch is pretty useless. However, after a little more debugging it turned out that string replacement in alias_posts_table() breaks the "$wpdb->posts.*" == $fields assumption in WP_Query, thus explicitly setting $split_the_query to false :(

To reproduce add the following before running a next post query with external object caching:

add_filter( 'get_next_post_where', function( $where ) { return $where; } );

[ethitter]

Replying to kovshenin:

Nacin is right. It does indeed split the query with external object cache so my patch is pretty useless. However, after a little more debugging it turned out that string replacement in alias_posts_table() breaks the "$wpdb->posts.*" == $fields assumption in WP_Query, thus explicitly setting $split_the_query to false :(

Overcoming this is a matter of using the split_the_query filter to set the value, instead of passing it via the query arguments. Easy enough, it would seem.

Unfortunately, when the query is split, the string manipulation in alias_posts_table() results in invalid SQL, because WP_Query expects the posts table to be in the SQL as whatever $wpdb->posts is. The answer is to reverse the string manipulation after the legacy filters are applied. I am a bit concerned with the amount of string manipulation this requires, but on the upside, the manipulations are rather basic.

I'm working on a new patch.

[ethitter]

Replying to ethitter:

It was silly of me not to realize that by simply reversing the aliasing, the split_the_query issue resolves itself. At any rate, that's what I've done in 26937.12.patch​.

My concern at this point is that I've used simple str_replace()s so far, but needed a preg_replace() to reverse the aliasing without clobbering other table names. Perhaps some of the other string manipulations should be regex-based instead? I tend to think it's unnecessary, given the types of manipulations done and that any unintended replacements would've caused SQL errors without the manipulations, but it's possible I'm missing some case.

[nacin]

#27564 is another bug in this, caused by excluded terms. Fixing that also means our test coverage needs to be increased.

As indicated in IRC when talking with kovshenin, at this point, I'm really concerned about this for 3.9. Let's face it, it's been a rabbit hole trying to get this fully backwards compatible. ethitter and I did not expect it to get this bad, and things like table aliases probably should have been the final straw. But we're also not just dealing with those kinds of bugs; we're also dealing with performance. For obvious reasons, WP_Query is slower than a raw query, but there are still some things we could do here to offset some of our losses — like a non-persistent cache so each adjacent link doesn't get re-queried (in the head then in the body, for example), which has always bothered me.

Right now, though, this adds a solid 2-3ms to each request. Maybe this would be OK normally, but then there are also a number of late-blooming bugs. On top of that, this doesn't actually give us any benefit beyond the feel-good of using date queries of raw SQL. It allows for better manipulation via WP_Query filters but without query contexts (like #17019 / #23833), it's tough to actually target them. And, the original justification was to do this for performance, but without actually bringing advanced post cache into core (#22176) we didn't actually gain anything — in fact, we lost a step.

There are also other concerns like sites just breaking because some plugin didn't expect these WP_Query calls. (For example, poor use of pre_get_posts as cited in #27564, or something else.) That doesn't impress me much, but because we've been fighting with bugs for weeks on this, it's possible that developers didn't even notice these more subtle breakage points. That we only now noticed that excluded terms were broken definitely shakes my confidence that this will go smoothly in 3.9 final, given my past experience.

So given all of this, and with ticket still open on the eve of the final beta, only days before what I hope to be an RC, and less than three before final release, it's all a pretty clear indicator of a revert-and-retry.

ethitter and I agree it's time to revert this from 3.9 and move this ticket to future release. The good thing is, the long soak this got in 3.9 has, presumably, allowed us to work out all or nearly of the kinks. I am happy to try this again in the future if the circumstances improve.

I'd like to commend ethitter for working extra hard during beta to support this feature to get it to a shippable state. This ticket was a pretty herculean effort and we definitely didn't expect these challenges at the start, but I'm glad the whole effort was undertaken.

[nacin]

#27564 was marked as a duplicate.

[nacin]

In 27836:

Revert the conversion of adjacent post queries to WP_Query. Explanation on the ticket.

Reverts [27285], [27286], [27287], [27288], [27291], [27292], [27293], [27296], [27633], [27634], [27635], and [27692].

see #26937.

[wpdavis]

In the time being, might it make sense to at least cache the existing function?

[boonebgorges]

#33076 was marked as a duplicate.

[mark-k]

ok, if it is not practical to change how the code works why not deprecate the functions and write new ones with the same parameters just implemented with wp_query while dropping the old filters.
Performance is important but having a better code base is IMHO as important

[johnbillion]

Related: #39111

[LindsayBSC]

+1 to @mark-k's comment re: considering deprecating the functions and making a newer, better way to get adjacent posts. The ability to override and "co-opt" the adjacent post functionality w/ our own queries is extraordinarily wonky in the current situation IMHO.

Replying to mark-k:

ok, if it is not practical to change how the code works why not deprecate the functions and write new ones with the same parameters just implemented with wp_query while dropping the old filters.
Performance is important but having a better code base is IMHO as important