Make WordPress Core

Opened 19 months ago

Last modified 15 months ago

#56689 reopened enhancement

Use WP_Query in get_page_by_path

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

Screenshot 2022-12-08 at 14.36.03.png (11.2 KB) - added by spacedmonkey 17 months ago.
Before patch
Screenshot 2022-12-08 at 14.35.59.png (10.9 KB) - added by spacedmonkey 17 months ago.
After patch
56689.diff (2.4 KB) - added by SergeyBiryukov 15 months ago.
56689.2.diff (2.7 KB) - added by SergeyBiryukov 15 months ago.

Download all attachments as: .zip

Change History (38)

This ticket was mentioned in PR #3365 on WordPress/wordpress-develop by spacedmonkey.


19 months ago
#1

  • Keywords has-patch added

#2 @spacedmonkey
17 months ago

  • Milestone changed from Future Release to 6.2

#3 @spacedmonkey
17 months ago

  • Owner set to spacedmonkey
  • Status changed from new to assigned

#4 @spacedmonkey
17 months 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.

#5 @spacedmonkey
15 months ago

  • Keywords has-unit-tests added

Unit tests are added.

#6 @costdev
15 months ago

  • Keywords has-testing-info added

#9 @SergeyBiryukov
15 months ago

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

#10 follow-up: @iandunn
15 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 @mukesh27
15 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 @spacedmonkey
15 months ago

Replying to iandunn:

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?

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 @iandunn
15 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: @spacedmonkey
15 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: @iandunn
15 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 to get_page_by_path. Add a comment explaining why it can't use WP_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 with suppress_filters. That doesn't actually avoid pre_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 @Otto42
15 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.

#18 @spacedmonkey
15 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.

#19 @spacedmonkey
15 months ago

In 55215:

Query: Revert [55169].

Revert [55169] and mark #56689 as wontfix. get_page_by_path can not use WP_Query, as is results in fatel errors for those using get_page_by_path in the pre_get_posts action or posts_pre_query filter. This sadly means that WP_Query can never be used in get_page_by_path.

Props spacedmonkey, iandunn, mukesh27, joemcgill, adamsilverstein, otto42.
See #56689.

#20 follow-up: @spacedmonkey
15 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 @iandunn
15 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 @joemcgill
15 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: @SergeyBiryukov
15 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: @Otto42
15 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 @SergeyBiryukov
15 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 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.

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 @SergeyBiryukov
15 months ago

Replying to iandunn:

Here's a few other potential solutions:

  • Solve the performance issue without WP_Query, like by manually add caching to get_page_by_path. Add a comment explaining why it can't use WP_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 @peterwilsoncc
15 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 ;)

#29 @spacedmonkey
15 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.

#30 @spacedmonkey
15 months ago

  • Milestone set to 6.2

#31 @spacedmonkey
15 months ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

#32 @joemcgill
15 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?

Last edited 15 months ago by joemcgill (previous) (diff)

#34 @peterwilsoncc
15 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.

Note: See TracTickets for help on using tickets.