Opened 8 years ago
Last modified 4 years ago
#26937 reopened enhancement
get_adjacent_post() should use WP_Query rather than build its own SQL query
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 3.7 |
Component: | Query | Keywords: | |
Focuses: | Cc: |
Description
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()
.
Attachments (18)
Change History (80)
This ticket was mentioned in IRC in #wordpress-dev by ethitter. View the logs.
8 years ago
#3
@
8 years ago
- Keywords has-patch added
26937.2.patch includes the following:
- Rewrites
get_adjacent_post()
to leverageWP_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 areget_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.
#4
follow-up:
↓ 5
@
8 years ago
- Milestone changed from Awaiting Review to 3.9
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.
#5
in reply to:
↑ 4
;
follow-up:
↓ 6
@
8 years ago
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.
#6
in reply to:
↑ 5
@
8 years ago
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.
#7
@
8 years ago
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.
#8
@
8 years ago
- Owner set to nacin
- Resolution set to fixed
- Status changed from new to closed
In 27286:
This ticket was mentioned in IRC in #wordpress-dev by jessepollak. View the logs.
8 years ago
#13
@
8 years ago
#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.
#20
@
8 years ago
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
#21
@
8 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
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; });
This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.
8 years ago
#23
follow-up:
↓ 25
@
8 years ago
It appears that when using next/previous_post_link it actually directs to a sticky post if there is one.
#24
@
8 years ago
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.
#25
in reply to:
↑ 23
@
8 years ago
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.
#26
@
8 years ago
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.
#27
@
8 years ago
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.
#28
follow-up:
↓ 29
@
8 years ago
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.
#29
in reply to:
↑ 28
;
follow-up:
↓ 30
@
8 years ago
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.
#30
in reply to:
↑ 29
@
8 years ago
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 :)
#31
follow-up:
↓ 32
@
8 years ago
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
.
#32
in reply to:
↑ 31
@
8 years ago
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.
#33
follow-up:
↓ 34
@
8 years ago
Currently sticky posts aren't excluded which causes adjacent posts to be sticky posts mostly. See 26937.8.patch.
#34
in reply to:
↑ 33
@
8 years ago
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.
#36
@
8 years ago
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 anyWHERE
that the filter returns, while also ensuring the filter's return value starts withAND
as it does initially inWP_Query
. This approach requires considerably-less string manipulation, which seems a plus given there's already enough of that in thefilter_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.
#37
@
8 years ago
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.
This ticket was mentioned in IRC in #wordpress-dev by ethitter. View the logs.
8 years ago
#42
@
8 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
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.
#43
follow-up:
↓ 45
@
8 years ago
26937.11.patch looks good. Doesn't appear to be a regression in [27635], but in the original commit, [27285].
#45
in reply to:
↑ 43
@
8 years ago
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. :)
#46
@
7 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
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.
This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.
7 years ago
#48
follow-up:
↓ 49
@
7 years ago
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.
#49
in reply to:
↑ 48
;
follow-up:
↓ 50
@
7 years ago
- Keywords has-patch removed
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; } );
#50
in reply to:
↑ 49
;
follow-up:
↓ 51
@
7 years ago
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.
#51
in reply to:
↑ 50
@
7 years ago
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.
#52
@
7 years ago
#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.
This ticket was mentioned in IRC in #wordpress-dev by wpdavis. View the logs.
7 years ago
#59
follow-up:
↓ 62
@
6 years ago
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
This ticket was mentioned in Slack in #core by obenland. View the logs.
6 years ago
#62
in reply to:
↑ 59
@
4 years ago
+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
26937.patch switches
get_adjacent_post()
to useWP_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.