-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set default post type in class-wp-query.php #6393
Conversation
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.
Hi @jonnynews! 👋 Thank you for your contribution to WordPress! 💖 It looks like this is your first pull request to 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 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 Unlinked AccountsThe 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:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe 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
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good to me.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I left some nit-picks. Peter's feedback needs to be addressed before merging.
Co-authored-by: Mukesh Panchal <[email protected]>
Co-authored-by: Mukesh Panchal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of notes but looks good generally.
$reflection->setAccessible( true ); | ||
|
||
$cache_key_1 = $reflection->invoke( $query1, $query_vars1, $request1 ); | ||
$cache_key_2 = $reflection->invoke( $query1, $query_vars2, $request1 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not yet defined but shouldn't this be using the request value from the second query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Thanks for the feedback @peterwilsoncc . Actioned it here 55026bc |
@spacedmonkey Sorry mate, I was thinking this block here. But it was good to catch the other spot. wordpress-develop/src/wp-includes/class-wp-query.php Lines 2539 to 2547 in 55026bc
|
In the class-wp-query.php file, the queried post types are now sorted to ensure consistency in cache key generation. Similarly, post statuses are also sorted for the same reason. This change also impacted the related unit tests in cacheResults.php, which are updated to reflect this new sorting logic.
Good catch @peterwilsoncc. I have actioned your feedback found some more cases where keys would not match.
I also improved the tests to use second query object and use the second query objects request. This highlighted a weird bug, where spaces in the query resulted in none match sql, meaning the keys do not match. Tests are now passing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, just a couple of notes to fix a typo and improve a comment.
Pre-approving as neither of the suggestions related to code.
Co-authored-by: Peter Wilson <[email protected]>
6390404
to
8d848b0
Compare
Committed in https://core.trac.wordpress.org/changeset/58122 |
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.