Make WordPress Core

Opened 9 months ago

Closed 9 months ago

Last modified 8 months ago

#59224 closed defect (bug) (fixed)

get_pages: duplicate `WP_Query::get_posts` call

Reported by: davidbinda's profile david.binda Owned by: joemcgill's profile joemcgill
Milestone: 6.3.2 Priority: normal
Severity: normal Version: 6.3
Component: Posts, Post Types Keywords: has-patch commit fixed-major
Focuses: performance Cc:

Description (last modified by SergeyBiryukov)

The internal usage of WP_Query in get_pages (introduced in r55569 ) performs the WP_Query::get_posts() call twice ( see source:tags/6.3/src/wp-includes/post.php#L6094 ).

When query args are passed to the WP_Query constructor, it returns the result of the WP_Query::get_posts method call (proxied through WP_Query::query). All the fetched posts are then already present in the WP_Query object and the correct way to access those is via the WP_Query->posts property.

In case the WP_Query::get_posts() is used, a duplicate SQL query is performed, eventually leading to performance downgrade (yet the caching inside the WP_Query mitigates this to some degree, unless cache_results query param is set to true).

Anyway, I would propose to work with the WP_Query inside the get_pages in the similar way as the code does in the get_posts, where the query args are passed to the object through the WP_Query::query method it's return value (the found posts) is being used (see source:tags/6.3/src/wp-includes/post.php#L2440 ).

Attachments (1)

59224.diff (536 bytes) - added by david.binda 9 months ago.

Download all attachments as: .zip

Change History (16)

@david.binda
9 months ago

#1 @SergeyBiryukov
9 months ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 6.4

#2 @azaozz
9 months ago

  • Milestone changed from 6.4 to 6.3.2

Uh, yea, I always suspected that [55569] will backfire eventually.

WP pages are not "just another post type", they have some specifics that all current code is expecting. Throwing away 14 years of incremental fixes and improvements for pages is not a good decision imho.

Changing the milestone to 6.3.2 as it would be best to fix this soon.

Last edited 9 months ago by azaozz (previous) (diff)

#3 @mukesh27
9 months ago

Thank you, @davidbinda, for providing the ticket and patch.

As you mentioned, the lack of extra query requests due to the wp_query cache explains why this went unnoticed. Well done on catching that detail.

The proposed changes are looks good to me.

#4 @spacedmonkey
9 months ago

  • Keywords commit added

Good catch @davidbinda.

Patch looks good to me. If other are happy, I will commit.

This ticket was mentioned in Slack in #core-performance by clarkeemily. View the logs.


9 months ago

#6 @flixos90
9 months ago

Thanks @davidbinda, great catch! Fix looks good to me as well.

#7 @SergeyBiryukov
9 months ago

The patch looks good to me as well.

This ticket was mentioned in PR #5114 on WordPress/wordpress-develop by joemcgill.


9 months ago
#8

This is a direct copy of 59224.diff from the original ticket. Opening the PR to allow CI to run before committing.

Trac ticket: https://core.trac.wordpress.org/ticket/59224

#9 @joemcgill
9 months ago

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

#10 @joemcgill
9 months ago

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

In 56491:

Posts, Post Types: Avoid redundant SQL query in get_pages().

This avoids an additional query by passing the query args directly to the WP_Query::query() method, rather than to the constructor and calling get_posts(), following [55569].

Props david.binda, azaozz, spacedmonkey, mukesh27, flixos90, SergeyBiryukov, joemcgill.
Fixes #59224.

#11 @joemcgill
9 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to be backported to the 6.3 branch.

joemcgill commented on PR #5114:


9 months ago
#12

Committed in 56491.

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


9 months ago

#14 @flixos90
9 months ago

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

In 56516:

Posts, Post Types: Avoid redundant SQL query in get_pages().

This avoids an additional query by passing the query args directly to the WP_Query::query() method, rather than to the constructor and calling get_posts(), following [55569].

Props david.binda, azaozz, spacedmonkey, mukesh27, flixos90, SergeyBiryukov, joemcgill.
Merges [56491] to the 6.3 branch.
Fixes #59224.

#15 @dilip2615
8 months ago

As per your description, it sounds like there is a potential for optimizing the get_pages function to avoid unnecessary database queries. If you're looking to optimize and make changes to a WordPress Core function, there are a few general steps you'll want to follow:

  1. Create a Child Theme: Always create a [child theme](https://developer.wordpress.org/themes/advanced-topics/child-themes/) to ensure that updates to the main theme do not erase your customizations.
  1. Duplicate and Modify: In your child theme, duplicate the function that you want to change, renaming it to avoid function naming conflicts.
  1. Make Your Changes: Edit the new function to optimize it as per your needs.
  1. Use Your Custom Function: Replace calls to the original function with calls to your new, optimized function where needed.

However, considering your question, let's look at a more specific approach for optimizing the get_pages function based on your description:

  1. Use WP_Query Efficiently: Avoid calling WP_Query::get_posts() twice, by utilizing the posts that are already fetched and available through the WP_Query->posts property.
  1. Modify or Create a Function: Write a new function or filter that uses the optimized method of querying posts/pages.

Here's a rough, theoretical example, as a basic reference to give you a starting point. This is not meant to be a direct solution, but rather a guideline:

`php
function optimized_get_pages( $args = array() ) {

... your extra code/logic ...

$query = new WP_Query( $args );
No need to call $query->get_posts() as we'll use $query->posts directly.

... possibly more of your code/logic ...

return $query->posts;

}
`

Now you would use optimized_get_pages() instead of get_pages() in your theme or plugin, to utilize the more optimized version of the function.

Note: The exact modification would greatly depend on the specific logic and requirements of your project, so adapt as necessary.

If you believe that this optimization should be part of the WordPress Core, consider creating a ticket on the [WordPress Trac](https://core.trac.wordpress.org/), or contribute to the [WordPress Core](https://make.wordpress.org/core/) directly. Always remember to test thoroughly to ensure that your changes do not introduce new issues or break existing functionality.

Version 0, edited 8 months ago by dilip2615 (next)
Note: See TracTickets for help on using tickets.