Set default post type in class-wp-query.php #6393

jonnynews · 2024-04-15T12:40:08Z

Post type handling in the WP_Query class has been improved. Now, if a post type is not defined, a default one will be set based on whether the post is an attachment, a page, or a regular post. This adjustment streamlines the code, improving both efficiency and readability.

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

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Post type handling in the WP_Query class has been improved. Now, if a post type is not defined, a default one will be set based on whether the post is an attachment, a page, or a regular post. This adjustment streamlines the code, improving both efficiency and readability.

github-actions · 2024-04-15T12:40:26Z

Hi @jonnynews! 👋

Thank you for your contribution to WordPress! 💖

It looks like this is your first pull request to wordpress-develop. Here are a few things to be aware of that may help you out!

No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description.

Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making.

More information about how GitHub pull requests can be used to contribute to WordPress can be found in this blog post.

Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook.

If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook.

The Developer Hub also documents the various coding standards that are followed:

Thank you,
The WordPress Project

github-actions · 2024-04-15T12:40:26Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @jonnynews.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

Core Committers: Use this line as a base for the props when committing in SVN:

Props spacedmonkey, joemcgill, pbearne, peterwilsoncc, rajinsharwar, mukesh27.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

github-actions · 2024-04-15T13:01:21Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

The post type handling in WP_Query has been refined to automatically set a default value when no specific post type is given. This change simplifies the defined arguments and enhances the code's efficiency. Additionally, test cases have been expanded to confirm the correct cache key generation related to the implemented changes.

Corrected the indentation in SQL query to improve code readability. The "test_generate_cache_key_normalize" method has been refactored to handle the fields and request variables accurately, ensuring proper testing of cache key generation across different queries and post types.

joemcgill

I think this is looking pretty good. A couple of nitpicks, but I really like this approach to handling this by normalizing the cache key generation, rather than modifying get_posts(). Nice!

src/wp-includes/class-wp-query.php

tests/phpunit/tests/query/cacheResults.php

The code has been updated to sort post types before the cache key generation to ensure consistency. Also, a default orderby value of 'date' has been added for the same purpose. Changes have been tested by adding a new test case to verify the query cache for differently ordered post types.

joemcgill

This is looking good to me.

joemcgill · 2024-04-18T14:04:39Z

src/wp-includes/class-wp-query.php

@@ -2542,6 +2542,8 @@ public function get_posts() {
 				$post_type_where = " AND {$wpdb->posts}.post_type IN ('" . implode( "', '", array_map( 'esc_sql', $in_search_post_types ) ) . "')";
 			}
 		} elseif ( ! empty( $post_type ) && is_array( $post_type ) ) {
+			// Sort post types to ensure same cache key generation.
+			sort( $post_type );


I assume this needs to happen here as well as sorting the post_type args in generate_cache_key() in order to normalize the SQL as well?

I assume this needs to happen here as well as sorting the post_type args in generate_cache_key() in order to normalize the SQL as well?

Correct. A query for post_type IN ( 'page', 'post' ) would get the same results as post_type IN ( 'post', 'page' )

It's outside the diff by in the any block above this one:

$in_search_post_types = get_post_types( array( 'exclude_from_search' => false ) ); sort( $in_search_post_types );

As with this change, it will normalize the SQL

src/wp-includes/class-wp-query.php

The WP_Query has been updated to correctly handle 'any' as a valid post type argument. This includes a change where 'any' will now return all post types excluded from search. New tests have also been introduced to ensure cache keys behave as expected when post types are registered, unregistered, and clarified under 'any' classification.

A new test scenario 'any and post types' has been added to the `cacheResults.php` testing module. This change aims to improve test coverage by checking query variables against different types of posts such as 'post', 'page', 'attachment' under the 'any' post type.

mukeshpanchal27

Thanks for the PR! I left some nit-picks. Peter's feedback needs to be addressed before merging.

tests/phpunit/tests/query/cacheResults.php

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

tests/phpunit/tests/query/cacheResults.php

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

peterwilsoncc

A couple of notes but looks good generally.

peterwilsoncc · 2024-04-29T00:31:58Z

src/wp-includes/class-wp-query.php

@@ -2542,6 +2542,8 @@ public function get_posts() {
 				$post_type_where = " AND {$wpdb->posts}.post_type IN ('" . implode( "', '", array_map( 'esc_sql', $in_search_post_types ) ) . "')";
 			}
 		} elseif ( ! empty( $post_type ) && is_array( $post_type ) ) {
+			// Sort post types to ensure same cache key generation.
+			sort( $post_type );


It's outside the diff by in the any block above this one:

$in_search_post_types = get_post_types( array( 'exclude_from_search' => false ) ); sort( $in_search_post_types );

As with this change, it will normalize the SQL

peterwilsoncc · 2024-04-29T00:47:41Z

tests/phpunit/tests/query/cacheResults.php

+		$reflection->setAccessible( true );
+
+		$cache_key_1 = $reflection->invoke( $query1, $query_vars1, $request1 );
+		$cache_key_2 = $reflection->invoke( $query1, $query_vars2, $request1 );


It's not yet defined but shouldn't this be using the request value from the second query.

I tried to do this in a early versions of the patch. It just made the tests harder to follow. I think this is fine. We can improve the tests later.

An update was made to the WP_Query class to ensure that when multiple post types are in use, they are sorted to generate a consistent cache key. This helps avoid potential issues with cache key mismatch and optimizes the performance of WordPress' cache system.

spacedmonkey · 2024-04-30T13:32:15Z

Thanks for the feedback @peterwilsoncc . Actioned it here 55026bc

jonnynews added 3 commits April 15, 2024 17:32

Fix lints.

43b0bbf

joemcgill requested changes Apr 16, 2024

View reviewed changes

src/wp-includes/class-wp-query.php Outdated Show resolved Hide resolved

src/wp-includes/class-wp-query.php Show resolved Hide resolved

tests/phpunit/tests/query/cacheResults.php Show resolved Hide resolved

jonnynews added 2 commits April 16, 2024 18:43

Fix lint error

694b385

jonnynews requested a review from joemcgill April 17, 2024 07:56

WordPress deleted a comment from drhsrushb Apr 18, 2024

joemcgill approved these changes Apr 18, 2024

View reviewed changes

pbearne approved these changes Apr 19, 2024

View reviewed changes

peterwilsoncc reviewed Apr 22, 2024

View reviewed changes

src/wp-includes/class-wp-query.php Show resolved Hide resolved

jonnynews added 2 commits April 22, 2024 10:05

mukeshpanchal27 reviewed Apr 22, 2024

View reviewed changes

tests/phpunit/tests/query/cacheResults.php Outdated Show resolved Hide resolved

tests/phpunit/tests/query/cacheResults.php Show resolved Hide resolved

Apply suggestions from code review

fa3a5af

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

Rajinsharwar reviewed Apr 22, 2024

View reviewed changes

tests/phpunit/tests/query/cacheResults.php Outdated Show resolved Hide resolved

mukeshpanchal27 reviewed Apr 23, 2024

View reviewed changes

tests/phpunit/tests/query/cacheResults.php Outdated Show resolved Hide resolved

Apply suggestions from code review

4c95d7e

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>

jonnynews requested review from mukeshpanchal27 and peterwilsoncc April 23, 2024 10:40

mukeshpanchal27 approved these changes Apr 25, 2024

View reviewed changes

peterwilsoncc reviewed Apr 29, 2024

View reviewed changes

spacedmonkey added 2 commits April 30, 2024 13:49

Merge branch 'trunk' into fix/cache-key-generation

900df75

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set default post type in class-wp-query.php #6393

Set default post type in class-wp-query.php #6393

jonnynews commented Apr 15, 2024

github-actions bot commented Apr 15, 2024

github-actions bot commented Apr 15, 2024 •

edited

github-actions bot commented Apr 15, 2024

joemcgill left a comment

joemcgill left a comment

joemcgill Apr 18, 2024

jonnynews Apr 18, 2024

peterwilsoncc Apr 29, 2024

mukeshpanchal27 left a comment

peterwilsoncc left a comment

peterwilsoncc Apr 29, 2024

peterwilsoncc Apr 29, 2024

jonnynews Apr 30, 2024

spacedmonkey commented Apr 30, 2024

Set default post type in class-wp-query.php #6393

Are you sure you want to change the base?

Set default post type in class-wp-query.php #6393

Conversation

jonnynews commented Apr 15, 2024

github-actions bot commented Apr 15, 2024

github-actions bot commented Apr 15, 2024 • edited

Unlinked Accounts

github-actions bot commented Apr 15, 2024

Test using WordPress Playground

Some things to be aware of

joemcgill left a comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

joemcgill Apr 18, 2024

Choose a reason for hiding this comment

jonnynews Apr 18, 2024

Choose a reason for hiding this comment

peterwilsoncc Apr 29, 2024

Choose a reason for hiding this comment

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

peterwilsoncc left a comment

Choose a reason for hiding this comment

peterwilsoncc Apr 29, 2024

Choose a reason for hiding this comment

peterwilsoncc Apr 29, 2024

Choose a reason for hiding this comment

jonnynews Apr 30, 2024

Choose a reason for hiding this comment

spacedmonkey commented Apr 30, 2024

github-actions bot commented Apr 15, 2024 •

edited