Opened 2 years ago
Last modified 21 months ago
#56689 reopened enhancement
Use WP_Query in get_page_by_path
Reported by: | spacedmonkey | Owned by: | spacedmonkey |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | |
Component: | Query | Keywords: | needs-testing has-patch has-unit-tests has-testing-info 2nd-opinion |
Focuses: | performance | Cc: |
Description
Use WP_Query
in get_page_by_path
, this this correctly primes caches and is cached itself.
Attachments (4)
Change History (38)
This ticket was mentioned in PR #3365 on WordPress/wordpress-develop by spacedmonkey.
2 years ago
#1
- Keywords has-patch added
#4
@
2 years ago
- Keywords needs-testing added
Tests to test.
Create a page.
Create a sub page of first page.
Create a sub sub page on second page.
Load page of sub sub page on FE end. See all parent posts primed and less database queries.
@spacedmonkey commented on PR #3365:
22 months ago
#7
#10
follow-up:
↓ 12
@
22 months ago
- Resolution fixed deleted
- Status changed from closed to reopened
There's code in the wild that calls get_page_by_path()
in pre_get_posts
callbacks. In that scenario it looks like this causes an infinite loop fatal error.
For example, handbooks.php:109 and template-tags.php:132 on w.org.
xref https://wordpress.slack.com/archives/C02QB8GMM/p1675204296499039
Is there something Core should do to handle that situation gracefully?
#11
@
22 months ago
- Type changed from enhancement to defect (bug)
- Version set to trunk
This ticket was discussed in the recent performance bug scrub.
We should investigate the issue or revert this commit.
Props to @joemcgill and @adamsilverstein
#12
in reply to:
↑ 10
@
21 months ago
Replying to iandunn:
There's code in the wild that calls
get_page_by_path()
inpre_get_posts
callbacks. In that scenario it looks like this causes an infinite loop fatal error.
For example, handbooks.php:109 and template-tags.php:132 on w.org.
xref https://wordpress.slack.com/archives/C02QB8GMM/p1675204296499039
Is there something Core should do to handle that situation gracefully?
This comment is a little unclear. What is the worry here? Why do we need to revert?
It worth noting that there is already a nested WP_Query in WP_Query. See this example.
pre_get_posts
is a really powerful hook, if it is missed it can generate errors. Looks like a brital implementation.
#13
@
21 months ago
- Keywords 2nd-opinion added
Why do we need to revert?
What I heard was that we should investigate or revert. Since it was mentioned in a bug scrub and Beta 1 is next week, I'm assuming they were thinking of a temporary revert that would give us time to explore a permanent fix.
What is the worry here?
My worry is simply that r55169 will introduce fatal errors into an unknown number of sites if we do nothing. IMO that's a significant problem and needs to be addressed in some way.
Looks like a brittle implementation
The w.org code was written by an experienced dev, and it doesn't seem obviously "wrong" to me at a glance. Even if it is, though, it seems like something that a lot of plugin devs might be doing. I don't think we can expect folks to be aware of the internals of Core functions.
I feel like back-compat is one of our foundational values and commitments -- because it makes Core less brittle from the user's perspective -- so we'd need a compelling reason to break it. Writing code that works today, only to get a fatal error 6 years later is the kind of situation that back-compat is intended to prevent.
I do think there's a reasonable balance to strike, though, and obviously the performance improvement is a good thing. I feel like there needs to be a sincere exploration of the impact and potential solutions, rather than accepting that sites will break.
#14
follow-up:
↓ 16
@
21 months ago
Looking into this issue, it comes from the fact that pre_get_posts
callbacks calls get_page_by_path
. Now that get_page_by_path
calls WP_Query
, it calls which calls pre_get_posts
. This ends up in a recursive loop. This feels pretty edge case, but if this does not get reverted, it means this 100% needs a dev note.
I don't see a way around this issue. This change results in calling WP_Query
in get_page_by_path
, has the caching and performance benefit, it comes along with the filters and actions of WP_Query
. We either role with that or revert and mark this as a wontfix. :(
Does any body have else have any thoughts on this one?
#15
follow-up:
↓ 26
@
21 months ago
pre_get_posts
has a generic purpose, and there's an infinite number of things that a plugin's business logic might need to do there. If we were talking about new code, I'd be fine with saying a plugin just needs to work around the inherent problem with recursion, but I feel like the main issue here is back-compat.
Here's a few other potential solutions:
- Solve the performance issue without
WP_Query
, like by manually add caching toget_page_by_path
. Add a comment explaining why it can't useWP_Query
. - Keep the
WP_Query
in most cases, but fallback to the old direct queries when a potential infinite loop is detected.
...and a few half-baked ones in case brainstorming sparks something better:
- Maybe something around calling
WP_Query
withsuppress_filters
. That doesn't actually avoidpre_get_posts
, but maybe a similar concept could be applied somewhere. - Detect the infinite loop, throw a
doing_it_wrong()
notice, and return early with an empty array. This would still need a dev note, since it's breaking the functionality. It's probably better to cause a fatal than to trigger some unpredictable behavior, though.
Manually adding caching to the function seems like the simplest to me.
#16
in reply to:
↑ 14
@
21 months ago
Replying to spacedmonkey:
We either role with that or revert and mark this as a wontfix. :(
Then this should be wontfix.
Backwards compatibility is more important than performance changes.
This has been in place, and broken, for three days now. It should have been reverted three days ago.
This ticket was mentioned in PR #3978 on WordPress/wordpress-develop by @spacedmonkey.
21 months ago
#17
Trac ticket: https://core.trac.wordpress.org/ticket/56689
#18
@
21 months ago
I am leave at the moment, so not had a chance to really look into this.
I have created a revert PR. This leaves the unit tests in, as are still valid and pass. I have assigned @Otto42 @iandunn as they have shown interest. I wait a while for feedback, revert and mark this ticket as wontfix.
#20
follow-up:
↓ 23
@
21 months ago
- Milestone 6.2 deleted
- Resolution set to wontfix
- Status changed from reopened to closed
Making as wontfix, as this patch generates fatal errors. I can't see anyway around this, so I marking as wontfix for now. We can reopen in the future if can work out another way around this issue.
#21
@
21 months ago
Thanks for the revert, I know it's painful and demoralizing to have to do that after working hard on something :(
Hopefully in the future we can reopen with an idea on how to get the performance gains in a different way.
#22
@
21 months ago
I'm totally fine with the resolution here, but wanted to comment a bit on the process to make sure that when folks like @spacedmonkey are working on performance enhancements in some of our core APIs during the alpha period of a WP Core release cycle that we have time to adequately identify and address issues like this that are discovered early in the cycle.
We noticed during the bug scrub earlier week that this enhancement ticket had been reopened by @iandunn due to the discovery of the pre_get_posts
bug.
When an enhancement is merged during the alpha period of a release cycle, my assumption is that the beta period of that cycle should be used to investigate and fix any bugs that are reported. If the bug can't be adequately addressed and the breakage is large enough that it warrants a revert, then the revert should be made as the final decision (at least for that release cycle) as it has been done here.
Additionally, I strongly disagree with @Otto42's opinion stated here:
This has been in place, and broken, for three days now. It should have been reverted three days ago.
The ability to collect feedback and fix bugs introduced during a release cycle is exactly what the alpha/beta periods are for, and it's an unrealistic expectation that people will be able to investigate and address feedback immediately. Anyone with systems that are dependent on running alpha software in production are assuming some risk in doing so.
#23
in reply to:
↑ 20
;
follow-up:
↓ 24
@
21 months ago
Replying to spacedmonkey:
We can reopen in the future if can work out another way around this issue.
What if we replace the direct SQL query, but also keep the function's own caching? Then the infinite loop should not be triggered if my understanding is correct.
56689.diff illustrates the idea. The tests still pass with this patch.
#24
in reply to:
↑ 23
;
follow-up:
↓ 25
@
21 months ago
Replying to SergeyBiryukov:
What if we replace the direct SQL query, but also keep the function's own caching? Then the infinite loop should not be triggered if my understanding is correct.
56689.diff illustrates the idea. The tests still pass with this patch.
I believe that would still trigger the loop.
The problem is the pre_get_posts
filter is one that may be used for a lot of different things. In this particular circumstance, it is being called and then calling get_page_by_path
. If this function calls a new WP_Query, that's going to, once again, call the pre_get_posts
filter. And that's where the loop is.
Making this a wrapper around WP_Query is going to be essentially a problem, no matter what, when it can be called by a filter that is called by WP_Query.
Now it is arguable that a pre_get_posts
filter should not be used to call this function. But that's an entirely new restraint on the filter. And given the age of the function and its use in countless possible plugins or themes or what have you... it's a very big backward compatibility break.
One way around it would be to disable the filter on creation of the WP_Query. However, there's not natively a way to do that, and potentially other filters could produce similar results as well.
Ultimately, I think this function should simply be deprecated if that is desired. If a replacement is desired for it that uses WP_Query, it should have a new name.
#25
in reply to:
↑ 24
@
21 months ago
Replying to Otto42:
I believe that would still trigger the loop.
The problem is the
pre_get_posts
filter is one that may be used for a lot of different things. In this particular circumstance, it is being called and then callingget_page_by_path
. If this function calls a new WP_Query, that's going to, once again, call thepre_get_posts
filter. And that's where the loop is.
Ah yes, you're right, it never gets to the point where wp_cache_set()
is called, so wp_cache_get()
doesn't work.
One way around it would be to disable the filter on creation of the WP_Query. However, there's not natively a way to do that, and potentially other filters could produce similar results as well.
Yeah, this avoids the loop in my testing, though having a WP_Query
argument to skip pre_get_posts
and any other hooks that still run with 'suppress_filters' => true
would be much better:
if ( doing_action( 'pre_get_posts' ) ) { $original_wp_filter = $wp_filter; unset( $wp_filter['pre_get_posts'] ); } $query = new WP_Query( $args ); $posts = $query->get_posts(); if ( isset( $original_wp_filter ) ) { $wp_filter = $original_wp_filter; }
#26
in reply to:
↑ 15
@
21 months ago
Replying to iandunn:
Here's a few other potential solutions:
- Solve the performance issue without
WP_Query
, like by manually add caching toget_page_by_path
. Add a comment explaining why it can't useWP_Query
.- Keep the
WP_Query
in most cases, but fallback to the old direct queries when a potential infinite loop is detected.
Played with the latter idea too in 56689.2.diff, just to see what the patch would look like.
Not exactly ideal, but seems better than manipulating the $wp_filter
global :)
#27
@
21 months ago
The advantage of using WP_Query
is that it hits the lower level cache, ideally increasing cache hits for various purposes.
The approaches in the most recent patches end up double caching, so the advantage is lost and using the simplest code seems the best approach. That being the code in it's current, committed form.
I experimented a little with a cache key collision before realising that was a terrible idea because it would be too easy for a future developer miss updating cache objects in multiple locations. Not all my ideas can be winners ;)
This ticket was mentioned in PR #4006 on WordPress/wordpress-develop by @spacedmonkey.
21 months ago
#28
Trac ticket: https://core.trac.wordpress.org/ticket/56689
#29
@
21 months ago
I have created a PR for backwards compat. See #4006.
This does a number of things.
- If it detects posts_pre_query or pre_get_posts filter / actions are in use, then it fallbacks to the old logic.
- Passes surpress filters as true.
This should that if someone is using a filter, that it should not effect this WP_Query. There are lots of filters in that class that are not covered by surpress filters, but I think should cover most of the breaking use cases.
#32
@
21 months ago
- Type changed from defect (bug) to enhancement
- Version trunk deleted
Since the original commit was reverted, the bug no longer exists in trunk
so this needs to be set back to enhancement
and committed by the 6.2 beta 1 milestone. Moving for visibility.
@iandunn or @Otto42 could either of you test out the new proposed patch and confirm that the back-compat logic here avoids the issue you reported earlier?
#33
@
21 months ago
Both 56689.2.diff and PR 4006 @ 3f64e1fe8 worked 👍🏻
#34
@
21 months ago
- Milestone changed from 6.2 to Future Release
As noted on the PR, I am not keen on the forked approach of sometimes using WP_Query
, sometimes using an SQL statement.
The effect is that any changes to this function will need to be made twice, which can easily be missed by future contributor (possibly those following this ticket).
Instead of rushing a fix for the beta, I think it best to come back to this and see how it can be improved without the forked code.
Trac ticket: https://core.trac.wordpress.org/ticket/56689