WP_Query: sort more arrays to improve cache hits.#9
Conversation
Introduces normalization a number of arguments passed to `WP_Query` to increase cache hits for equivalent requests. For example `author__in => [ 1, 2 ]` and `author__in => [ 2, 1 ]` will now hit the same cache. Prior to generating the SQL request and cache key, the following are sorted, made unique and type cast as appropriate. * `post_type` when passed as an array * `post_status` when passed as an array * `term_query`s containing `terms` * `cat` * `category__in` * `category__not_in` * `category__and` * `tag_slug__in` * `tag__in` * `tag__not_in` * `tag__and` * `tag_slug__in` * `tag_slug__and` * `post_parent__not_in` * `author` * `author__not_in` * `author__in` The following are sorted for the purposes of generating the cache key and SQL `WHERE` clause but unmodified for use in the `ORDER BY` SQL clause: * `post_name__in` * `post__in` * `post_parent__in` This commit includes changes to unrelated tests, assertions in `Tests_Query_ParseQuery::test_parse_query_cat_array_mixed()` and `WP_Test_REST_Posts_Controller::test_get_items_not_sticky_with_exclude()` have been modified to account for the sorting of the items above. Props thekt12, peterwilsoncc, spacedmonkey, joemcgill, flixos90, mukesh27, pbearne, swissspidy. Fixes #59516. git-svn-id: https://develop.svn.wordpress.org/trunk@59766 602fd350-edb4-49c9-b593-d223f7449a82
|
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: @zeyu2001. 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. |
📝 WalkthroughWalkthrough
ChangesWP_Query normalization and cache behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e6932e65c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| $q['author__not_in'] = array_unique( array_map( 'absint', $q['author__not_in'] ) ); | ||
| sort( $q['author__not_in'] ); | ||
| } | ||
| $author__not_in = implode( ',', (array) $q['author__not_in'] ); |
There was a problem hiding this comment.
When author__not_in is supplied as a scalar string (which the previous code handled by casting to an array and applying absint()), this path now skips the sanitizer and implode() returns the raw string that is interpolated into post_author NOT IN (...) on the next line. Callers that pass a request value through to WP_Query can therefore inject SQL via author__not_in; normalize with absint() regardless of whether the input is already an array.
Useful? React with 👍 / 👎.
| $cat_array = preg_split( '/[,\s]+/', urldecode( $q['cat'] ) ); | ||
| $cat_array = array_map( 'intval', $cat_array ); | ||
| $q['cat'] = implode( ',', $cat_array ); | ||
| sort( $cat_array ); |
There was a problem hiding this comment.
For multi-category queries such as cat=5,2, sorting here rewrites $q['cat'] before the later compatibility code stores the first queried category back into the query vars. The result set is unchanged, but get_queried_object() and conditionals like is_category( 5 ) now refer to the lowest ID instead of the first category the request named, which is a backward-compatibility regression for category archive templates.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if ( ! empty( $q['author__not_in'] ) ) { | ||
| $author__not_in = implode( ',', array_map( 'absint', array_unique( (array) $q['author__not_in'] ) ) ); | ||
| if ( is_array( $q['author__not_in'] ) ) { | ||
| $q['author__not_in'] = array_unique( array_map( 'absint', $q['author__not_in'] ) ); | ||
| sort( $q['author__not_in'] ); | ||
| } | ||
| $author__not_in = implode( ',', (array) $q['author__not_in'] ); | ||
| $where .= " AND {$wpdb->posts}.post_author NOT IN ($author__not_in) "; |
There was a problem hiding this comment.
🟡 Excluded-author list can break the query when supplied as a plain string
The list of authors to exclude is inserted into the database query without converting its values to integers (implode( ',', (array) $q['author__not_in'] ) at src/wp-includes/class-wp-query.php:2397) whenever it is provided as a non-array value, so a non-numeric string exclusion value produces a broken query instead of being safely ignored.
Impact: A request that passes the author-exclusion value as a plain string can cause the posts query to fail and return no results.
Loss of absint coercion on the non-array path
Previously this branch always ran array_map( 'absint', array_unique( (array) $q['author__not_in'] ) ), guaranteeing every value became an integer before being placed in NOT IN (...). The new code only applies absint/array_unique inside the is_array() guard (src/wp-includes/class-wp-query.php:2393-2396); when author__not_in is a string it falls through to implode( ',', (array) $q['author__not_in'] ) at src/wp-includes/class-wp-query.php:2397 with no integer coercion. For a value like 'foo' the generated SQL becomes NOT IN (foo), which is invalid. The old code coerced this to NOT IN (0), a valid (if no-op) clause. The parallel author__in branch at src/wp-includes/class-wp-query.php:2404 still re-applies absint on its final implode, so only author__not_in loses this protection.
| if ( ! empty( $q['author__not_in'] ) ) { | |
| $author__not_in = implode( ',', array_map( 'absint', array_unique( (array) $q['author__not_in'] ) ) ); | |
| if ( is_array( $q['author__not_in'] ) ) { | |
| $q['author__not_in'] = array_unique( array_map( 'absint', $q['author__not_in'] ) ); | |
| sort( $q['author__not_in'] ); | |
| } | |
| $author__not_in = implode( ',', (array) $q['author__not_in'] ); | |
| $where .= " AND {$wpdb->posts}.post_author NOT IN ($author__not_in) "; | |
| if ( ! empty( $q['author__not_in'] ) ) { | |
| if ( is_array( $q['author__not_in'] ) ) { | |
| $q['author__not_in'] = array_unique( array_map( 'absint', $q['author__not_in'] ) ); | |
| sort( $q['author__not_in'] ); | |
| } | |
| $author__not_in = implode( ',', array_map( 'absint', (array) $q['author__not_in'] ) ); | |
| $where .= " AND {$wpdb->posts}.post_author NOT IN ($author__not_in) "; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if ( ! empty( $q['author__not_in'] ) ) { | ||
| $author__not_in = implode( ',', array_map( 'absint', array_unique( (array) $q['author__not_in'] ) ) ); | ||
| if ( is_array( $q['author__not_in'] ) ) { | ||
| $q['author__not_in'] = array_unique( array_map( 'absint', $q['author__not_in'] ) ); | ||
| sort( $q['author__not_in'] ); | ||
| } | ||
| $author__not_in = implode( ',', (array) $q['author__not_in'] ); | ||
| $where .= " AND {$wpdb->posts}.post_author NOT IN ($author__not_in) "; |
There was a problem hiding this comment.
🟨 Author exclusion value inserted into SQL without integer coercion for non-array input
When author__not_in is provided as a non-array value, the new code path builds $author__not_in = implode( ',', (array) $q['author__not_in'] ) at src/wp-includes/class-wp-query.php:2397 and concatenates it directly into the WHERE clause NOT IN (...) without applying absint. Previously the code always ran array_map( 'absint', array_unique( (array) $q['author__not_in'] ) ), guaranteeing every value was coerced to an integer. Removing this coercion for the non-array branch means an attacker-controlled string value flows unsanitized into the SQL string, creating a SQL injection surface. The parallel author__in branch still re-applies absint on its final implode, so only author__not_in loses this protection.
Was this helpful? React with 👍 or 👎 to provide feedback.
Greptile SummaryThis PR normalises
Confidence Score: 4/5Safe to merge with minor clean-up; the correctness of query results is not affected, only the completeness of the cache normalisation for a couple of edge cases. The main logic is sound and well-tested for all the src/wp-includes/class-wp-query.php — specifically the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[WP_Query input args] --> B[parse_query]
B --> C{Array-type var?}
C -->|post_type, post_status, category__in/not_in/and, tag__in/not_in/and, author__in/not_in| D[array_unique + sort in-place]
C -->|post__in, post_parent__in, post_name__in| E[sort deferred to get_posts - original order preserved for orderby]
D --> F[get_posts]
E --> F
F --> G[Build WHERE clause]
G --> H{post__in / post_parent__in / post_name__in?}
H -->|yes| I[Copy array → array_unique + sort copy, original untouched for ORDER BY]
H -->|no: post__not_in, post_parent__not_in| J[sort in-place directly - NO array_unique]
I --> K[Assemble SQL into this.request]
J --> K
K --> L[generate_cache_key q + SQL]
L --> M{Cache hit?}
M -->|yes| N[Return cached results]
M -->|no| O[Execute SQL → store in cache]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[WP_Query input args] --> B[parse_query]
B --> C{Array-type var?}
C -->|post_type, post_status, category__in/not_in/and, tag__in/not_in/and, author__in/not_in| D[array_unique + sort in-place]
C -->|post__in, post_parent__in, post_name__in| E[sort deferred to get_posts - original order preserved for orderby]
D --> F[get_posts]
E --> F
F --> G[Build WHERE clause]
G --> H{post__in / post_parent__in / post_name__in?}
H -->|yes| I[Copy array → array_unique + sort copy, original untouched for ORDER BY]
H -->|no: post__not_in, post_parent__not_in| J[sort in-place directly - NO array_unique]
I --> K[Assemble SQL into this.request]
J --> K
K --> L[generate_cache_key q + SQL]
L --> M{Cache hit?}
M -->|yes| N[Return cached results]
M -->|no| O[Execute SQL → store in cache]
Reviews (1): Last reviewed commit: "chore: trigger CI" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -1314,6 +1335,7 @@ public function parse_tax_query( &$q ) { | |||
| } else { | |||
| $q['tag'] = sanitize_term_field( 'slug', $q['tag'], 0, 'post_tag', 'db' ); | |||
| $q['tag_slug__in'][] = $q['tag']; | |||
| sort( $q['tag_slug__in'] ); | |||
There was a problem hiding this comment.
sort() inside loop
sort( $q['tag_slug__in'] ) is called on every iteration of the loop (line 1327) and again immediately after the single-element append in the else branch (line 1338). Both are redundant because the tag_slug__in processing block at line 1380 unconditionally re-applies array_unique(), sanitize_title_for_query, and then sort(). Moving the sort to after the loop (or removing it here entirely) would match the pattern used elsewhere in this PR.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| } elseif ( $q['post__not_in'] ) { | ||
| sort( $q['post__not_in'] ); | ||
| $post__not_in = implode( ',', array_map( 'absint', $q['post__not_in'] ) ); | ||
| $where .= " AND {$wpdb->posts}.ID NOT IN ($post__not_in)"; | ||
| } |
There was a problem hiding this comment.
array_unique() on post__not_in
Unlike their __in counterparts (which use array_unique() before building the WHERE clause), post__not_in is only sorted without deduplication. This means post__not_in => [1, 1, 2] produces NOT IN (1,1,2) while post__not_in => [1, 2] produces NOT IN (1,2) — two different SQL strings that yield identical result sets but different cache keys, defeating the PR's normalization goal for duplicated inputs.
| } elseif ( $q['post__not_in'] ) { | |
| sort( $q['post__not_in'] ); | |
| $post__not_in = implode( ',', array_map( 'absint', $q['post__not_in'] ) ); | |
| $where .= " AND {$wpdb->posts}.ID NOT IN ($post__not_in)"; | |
| } | |
| } elseif ( $q['post__not_in'] ) { | |
| $q['post__not_in'] = array_unique( array_map( 'absint', $q['post__not_in'] ) ); | |
| sort( $q['post__not_in'] ); | |
| $post__not_in = implode( ',', $q['post__not_in'] ); | |
| $where .= " AND {$wpdb->posts}.ID NOT IN ($post__not_in)"; | |
| } |
| } elseif ( $q['post_parent__not_in'] ) { | ||
| sort( $q['post_parent__not_in'] ); | ||
| $post_parent__not_in = implode( ',', array_map( 'absint', $q['post_parent__not_in'] ) ); | ||
| $where .= " AND {$wpdb->posts}.post_parent NOT IN ($post_parent__not_in)"; | ||
| } |
There was a problem hiding this comment.
array_unique() on post_parent__not_in
Same issue as post__not_in: the sort without deduplication means duplicate values produce a different SQL string (and therefore a different cache key) compared to the deduplicated equivalent, even though the result set is identical.
| } elseif ( $q['post_parent__not_in'] ) { | |
| sort( $q['post_parent__not_in'] ); | |
| $post_parent__not_in = implode( ',', array_map( 'absint', $q['post_parent__not_in'] ) ); | |
| $where .= " AND {$wpdb->posts}.post_parent NOT IN ($post_parent__not_in)"; | |
| } | |
| } elseif ( $q['post_parent__not_in'] ) { | |
| $q['post_parent__not_in'] = array_unique( array_map( 'absint', $q['post_parent__not_in'] ) ); | |
| sort( $q['post_parent__not_in'] ); | |
| $post_parent__not_in = implode( ',', $q['post_parent__not_in'] ); | |
| $where .= " AND {$wpdb->posts}.post_parent NOT IN ($post_parent__not_in)"; | |
| } |
| * | ||
| * These arrays are sorted in the query generator for the purposes of the | ||
| * WHERE clause but the arguments are not modified as they can be used for | ||
| * the orderby clase. |
There was a problem hiding this comment.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/wp-includes/class-wp-query.php`:
- Around line 2392-2398: Update the author__not_in handling in WP_Query so
scalar values are sanitized with absint before constructing the NOT IN clause,
while preserving the existing array normalization, deduplication, and sorting
behavior. Ensure the value assigned to $author__not_in is always safe for SQL
interpolation.
- Around line 1114-1124: In the post_type and post_status array normalization
branches of WP_Query, sanitize each value before applying array_unique(), then
sort the resulting values. Preserve the existing scalar sanitization behavior
and ensure equivalent sanitized queries produce the same normalized arrays and
cache key.
- Around line 2238-2256: Canonicalize both exclusion arrays before sorting in
the post__not_in and post_parent__not_in branches: map values through absint,
remove duplicates, then sort and build the SQL list from the canonical arrays.
Preserve the existing NOT IN behavior while ensuring equivalent ID inputs
produce identical SQL and cache keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 288036cb-6fc6-4c29-983d-a3932ea1180d
📒 Files selected for processing (4)
src/wp-includes/class-wp-query.phptests/phpunit/tests/query/cacheResults.phptests/phpunit/tests/query/parseQuery.phptests/phpunit/tests/rest-api/rest-posts-controller.php
| $qv['post_type'] = array_map( 'sanitize_key', array_unique( $qv['post_type'] ) ); | ||
| sort( $qv['post_type'] ); | ||
| } else { | ||
| $qv['post_type'] = sanitize_key( $qv['post_type'] ); | ||
| } | ||
| } | ||
|
|
||
| if ( ! empty( $qv['post_status'] ) ) { | ||
| if ( is_array( $qv['post_status'] ) ) { | ||
| $qv['post_status'] = array_map( 'sanitize_key', $qv['post_status'] ); | ||
| $qv['post_status'] = array_map( 'sanitize_key', array_unique( $qv['post_status'] ) ); | ||
| sort( $qv['post_status'] ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Deduplicate after sanitization.
array_unique() runs before sanitize_key(), so values such as ['foo bar', 'foobar'] become duplicate keys and produce a different cache key from the equivalent unique query.
Proposed fix
- $qv['post_type'] = array_map( 'sanitize_key', array_unique( $qv['post_type'] ) );
+ $qv['post_type'] = array_unique( array_map( 'sanitize_key', $qv['post_type'] ) );
sort( $qv['post_type'] );
...
- $qv['post_status'] = array_map( 'sanitize_key', array_unique( $qv['post_status'] ) );
+ $qv['post_status'] = array_unique( array_map( 'sanitize_key', $qv['post_status'] ) );
sort( $qv['post_status'] );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $qv['post_type'] = array_map( 'sanitize_key', array_unique( $qv['post_type'] ) ); | |
| sort( $qv['post_type'] ); | |
| } else { | |
| $qv['post_type'] = sanitize_key( $qv['post_type'] ); | |
| } | |
| } | |
| if ( ! empty( $qv['post_status'] ) ) { | |
| if ( is_array( $qv['post_status'] ) ) { | |
| $qv['post_status'] = array_map( 'sanitize_key', $qv['post_status'] ); | |
| $qv['post_status'] = array_map( 'sanitize_key', array_unique( $qv['post_status'] ) ); | |
| sort( $qv['post_status'] ); | |
| $qv['post_type'] = array_unique( array_map( 'sanitize_key', $qv['post_type'] ) ); | |
| sort( $qv['post_type'] ); | |
| } else { | |
| $qv['post_type'] = sanitize_key( $qv['post_type'] ); | |
| } | |
| } | |
| if ( ! empty( $qv['post_status'] ) ) { | |
| if ( is_array( $qv['post_status'] ) ) { | |
| $qv['post_status'] = array_unique( array_map( 'sanitize_key', $qv['post_status'] ) ); | |
| sort( $qv['post_status'] ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/wp-includes/class-wp-query.php` around lines 1114 - 1124, In the
post_type and post_status array normalization branches of WP_Query, sanitize
each value before applying array_unique(), then sort the resulting values.
Preserve the existing scalar sanitization behavior and ensure equivalent
sanitized queries produce the same normalized arrays and cache key.
| } elseif ( $q['post__not_in'] ) { | ||
| sort( $q['post__not_in'] ); | ||
| $post__not_in = implode( ',', array_map( 'absint', $q['post__not_in'] ) ); | ||
| $where .= " AND {$wpdb->posts}.ID NOT IN ($post__not_in)"; | ||
| } | ||
|
|
||
| if ( is_numeric( $q['post_parent'] ) ) { | ||
| $where .= $wpdb->prepare( " AND {$wpdb->posts}.post_parent = %d ", $q['post_parent'] ); | ||
| } elseif ( $q['post_parent__in'] ) { | ||
| $post_parent__in = implode( ',', array_map( 'absint', $q['post_parent__in'] ) ); | ||
| // Duplicate array before sorting to allow for the orderby clause. | ||
| $post_parent__in_for_where = $q['post_parent__in']; | ||
| $post_parent__in_for_where = array_unique( array_map( 'absint', $post_parent__in_for_where ) ); | ||
| sort( $post_parent__in_for_where ); | ||
| $post_parent__in = implode( ',', array_map( 'absint', $post_parent__in_for_where ) ); | ||
| $where .= " AND {$wpdb->posts}.post_parent IN ($post_parent__in)"; | ||
| } elseif ( $q['post_parent__not_in'] ) { | ||
| sort( $q['post_parent__not_in'] ); | ||
| $post_parent__not_in = implode( ',', array_map( 'absint', $q['post_parent__not_in'] ) ); | ||
| $where .= " AND {$wpdb->posts}.post_parent NOT IN ($post_parent__not_in)"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Canonicalize exclusion IDs before sorting.
post__not_in and post_parent__not_in sort raw values, then cast only the SQL copy. Duplicates, numeric strings, and signed values can therefore generate different SQL/cache keys for the same effective ID set.
Proposed fix
} elseif ( $q['post__not_in'] ) {
+ $q['post__not_in'] = array_unique( array_map( 'absint', (array) $q['post__not_in'] ) );
sort( $q['post__not_in'] );
- $post__not_in = implode( ',', array_map( 'absint', $q['post__not_in'] ) );
+ $post__not_in = implode( ',', $q['post__not_in'] );
...
} elseif ( $q['post_parent__not_in'] ) {
+ $q['post_parent__not_in'] = array_unique( array_map( 'absint', (array) $q['post_parent__not_in'] ) );
sort( $q['post_parent__not_in'] );
- $post_parent__not_in = implode( ',', array_map( 'absint', $q['post_parent__not_in'] ) );
+ $post_parent__not_in = implode( ',', $q['post_parent__not_in'] );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } elseif ( $q['post__not_in'] ) { | |
| sort( $q['post__not_in'] ); | |
| $post__not_in = implode( ',', array_map( 'absint', $q['post__not_in'] ) ); | |
| $where .= " AND {$wpdb->posts}.ID NOT IN ($post__not_in)"; | |
| } | |
| if ( is_numeric( $q['post_parent'] ) ) { | |
| $where .= $wpdb->prepare( " AND {$wpdb->posts}.post_parent = %d ", $q['post_parent'] ); | |
| } elseif ( $q['post_parent__in'] ) { | |
| $post_parent__in = implode( ',', array_map( 'absint', $q['post_parent__in'] ) ); | |
| // Duplicate array before sorting to allow for the orderby clause. | |
| $post_parent__in_for_where = $q['post_parent__in']; | |
| $post_parent__in_for_where = array_unique( array_map( 'absint', $post_parent__in_for_where ) ); | |
| sort( $post_parent__in_for_where ); | |
| $post_parent__in = implode( ',', array_map( 'absint', $post_parent__in_for_where ) ); | |
| $where .= " AND {$wpdb->posts}.post_parent IN ($post_parent__in)"; | |
| } elseif ( $q['post_parent__not_in'] ) { | |
| sort( $q['post_parent__not_in'] ); | |
| $post_parent__not_in = implode( ',', array_map( 'absint', $q['post_parent__not_in'] ) ); | |
| $where .= " AND {$wpdb->posts}.post_parent NOT IN ($post_parent__not_in)"; | |
| } elseif ( $q['post__not_in'] ) { | |
| $q['post__not_in'] = array_unique( array_map( 'absint', (array) $q['post__not_in'] ) ); | |
| sort( $q['post__not_in'] ); | |
| $post__not_in = implode( ',', $q['post__not_in'] ); | |
| $where .= " AND {$wpdb->posts}.ID NOT IN ($post__not_in)"; | |
| } | |
| if ( is_numeric( $q['post_parent'] ) ) { | |
| $where .= $wpdb->prepare( " AND {$wpdb->posts}.post_parent = %d ", $q['post_parent'] ); | |
| } elseif ( $q['post_parent__in'] ) { | |
| // Duplicate array before sorting to allow for the orderby clause. | |
| $post_parent__in_for_where = $q['post_parent__in']; | |
| $post_parent__in_for_where = array_unique( array_map( 'absint', $post_parent__in_for_where ) ); | |
| sort( $post_parent__in_for_where ); | |
| $post_parent__in = implode( ',', array_map( 'absint', $post_parent__in_for_where ) ); | |
| $where .= " AND {$wpdb->posts}.post_parent IN ($post_parent__in)"; | |
| } elseif ( $q['post_parent__not_in'] ) { | |
| $q['post_parent__not_in'] = array_unique( array_map( 'absint', (array) $q['post_parent__not_in'] ) ); | |
| sort( $q['post_parent__not_in'] ); | |
| $post_parent__not_in = implode( ',', $q['post_parent__not_in'] ); | |
| $where .= " AND {$wpdb->posts}.post_parent NOT IN ($post_parent__not_in)"; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/wp-includes/class-wp-query.php` around lines 2238 - 2256, Canonicalize
both exclusion arrays before sorting in the post__not_in and post_parent__not_in
branches: map values through absint, remove duplicates, then sort and build the
SQL list from the canonical arrays. Preserve the existing NOT IN behavior while
ensuring equivalent ID inputs produce identical SQL and cache keys.
| if ( ! empty( $q['author__not_in'] ) ) { | ||
| $author__not_in = implode( ',', array_map( 'absint', array_unique( (array) $q['author__not_in'] ) ) ); | ||
| if ( is_array( $q['author__not_in'] ) ) { | ||
| $q['author__not_in'] = array_unique( array_map( 'absint', $q['author__not_in'] ) ); | ||
| sort( $q['author__not_in'] ); | ||
| } | ||
| $author__not_in = implode( ',', (array) $q['author__not_in'] ); | ||
| $where .= " AND {$wpdb->posts}.post_author NOT IN ($author__not_in) "; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Injection (CWE-89): Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection')
Sanitize scalar author__not_in before SQL interpolation.
Only arrays pass through absint; a scalar value is directly interpolated into the NOT IN clause. Normalize unconditionally before building SQL.
Proposed fix
if ( ! empty( $q['author__not_in'] ) ) {
- if ( is_array( $q['author__not_in'] ) ) {
- $q['author__not_in'] = array_unique( array_map( 'absint', $q['author__not_in'] ) );
- sort( $q['author__not_in'] );
- }
- $author__not_in = implode( ',', (array) $q['author__not_in'] );
+ $q['author__not_in'] = array_unique( array_map( 'absint', (array) $q['author__not_in'] ) );
+ sort( $q['author__not_in'] );
+ $author__not_in = implode( ',', $q['author__not_in'] );
$where .= " AND {$wpdb->posts}.post_author NOT IN ($author__not_in) ";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( ! empty( $q['author__not_in'] ) ) { | |
| $author__not_in = implode( ',', array_map( 'absint', array_unique( (array) $q['author__not_in'] ) ) ); | |
| if ( is_array( $q['author__not_in'] ) ) { | |
| $q['author__not_in'] = array_unique( array_map( 'absint', $q['author__not_in'] ) ); | |
| sort( $q['author__not_in'] ); | |
| } | |
| $author__not_in = implode( ',', (array) $q['author__not_in'] ); | |
| $where .= " AND {$wpdb->posts}.post_author NOT IN ($author__not_in) "; | |
| if ( ! empty( $q['author__not_in'] ) ) { | |
| $q['author__not_in'] = array_unique( array_map( 'absint', (array) $q['author__not_in'] ) ); | |
| sort( $q['author__not_in'] ); | |
| $author__not_in = implode( ',', $q['author__not_in'] ); | |
| $where .= " AND {$wpdb->posts}.post_author NOT IN ($author__not_in) "; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/wp-includes/class-wp-query.php` around lines 2392 - 2398, Update the
author__not_in handling in WP_Query so scalar values are sanitized with absint
before constructing the NOT IN clause, while preserving the existing array
normalization, deduplication, and sorting behavior. Ensure the value assigned to
$author__not_in is always safe for SQL interpolation.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| if ( ! empty( $q['author__not_in'] ) ) { | ||
| $author__not_in = implode( ',', array_map( 'absint', array_unique( (array) $q['author__not_in'] ) ) ); | ||
| if ( is_array( $q['author__not_in'] ) ) { | ||
| $q['author__not_in'] = array_unique( array_map( 'absint', $q['author__not_in'] ) ); | ||
| sort( $q['author__not_in'] ); | ||
| } | ||
| $author__not_in = implode( ',', (array) $q['author__not_in'] ); |
There was a problem hiding this comment.
🔴 The refactored author__not_in branch in WP_Query::get_posts() (class-wp-query.php:2392-2398) only applies absint()/array_unique()/sort() inside an is_array() guard, so a scalar value (e.g. set by a plugin/theme/pre_get_posts, since fill_query_vars() only defaults an unset key to array()) skips sanitization entirely and is imploded straight into the unprepared SQL WHERE clause. This reintroduces a SQL-injection sanitization gap that existed before this PR, since the old code ran absint() unconditionally via (array) $q['author__not_in']; the sibling author__in branch still keeps absint() on its final implode (line 2404) so only author__not_in regressed.
Extended reasoning...
The bug: In WP_Query::get_posts(), the author__not_in branch was refactored from a single unconditional sanitize-and-implode line into a guarded one:
if ( ! empty( $q['author__not_in'] ) ) {
if ( is_array( $q['author__not_in'] ) ) {
$q['author__not_in'] = array_unique( array_map( 'absint', $q['author__not_in'] ) );
sort( $q['author__not_in'] );
}
$author__not_in = implode( ',', (array) $q['author__not_in'] );
$where .= " AND {$wpdb->posts}.post_author NOT IN ($author__not_in) ";Before this PR, the line was implode( ',', array_map( 'absint', array_unique( (array) $q['author__not_in'] ) ) ) — the (array) cast normalized both scalars and arrays before absint() ran on every element, so a scalar string like '1) OR 1=1 --' became ['1) OR 1=1 --'] and then absint() reduced it to 0. In the new code, absint()/array_unique()/sort() are gated behind is_array( $q['author__not_in'] ). If the value is a scalar, that whole block is skipped, and the final line does implode( ',', (array) $q['author__not_in'] ), which for a scalar just wraps it in a one-element array and implodes it back out unchanged — i.e. the raw, unsanitized value.
Code path: That unsanitized $author__not_in string is concatenated directly into $where (... post_author NOT IN ($author__not_in) ...), which flows into $this->request, and is executed via $wpdb->get_results( $this->request ) — with no $wpdb->prepare() anywhere in this path. Anything landing in $where unsanitized is a direct SQL injection sink.
Why existing code doesn't prevent it: fill_query_vars() (around lines 617-640) only defaults author__not_in to array() when the key is unset — it does not coerce an explicitly-set scalar value into an array. author__not_in is documented as int[] but WP_Query does not enforce that type; nothing else in parse_query() normalizes this variable before get_posts() reaches this branch. So a plugin, theme, or a pre_get_posts callback that sets author__not_in to a scalar (string or int) — which is easy to do inadvertently while building query args — reaches this branch with the guard's is_array() check evaluating false, and the value flows through unsanitized.
Impact: This is a security regression: an unsanitized, attacker-influenceable value could be interpolated into a raw SQL WHERE clause with no escaping/preparation, whereas prior to this PR the same input was always safely reduced via absint(). It's also inconsistent with the sibling author__in branch just below (lines 2399-2404), which still runs array_unique( array_map( 'absint', $q['author__in'] ) ) unconditionally on its final implode line — confirming this asymmetry is an unintentional regression from the refactor, not an intended behavior change.
Proof (step-by-step):
- Some code (plugin/theme/
pre_get_posts) runsnew WP_Query( array( 'author__not_in' => '1) OR 1=1 --' ) )— a scalar, not wrapped in an array. fill_query_vars()sees the key is already set, so it leaves it as the scalar string unchanged.- In
get_posts(),! empty( $q['author__not_in'] )is true, so we enter the block. is_array( $q['author__not_in'] )is false (it's a string), so theabsint()/sort()sanitization block is skipped entirely.$author__not_in = implode( ',', (array) $q['author__not_in'] )— casting a scalar string to an array just wraps it (['1) OR 1=1 --']), and imploding a one-element array returns the string unchanged:'1) OR 1=1 --'.$where .= " AND {$wpdb->posts}.post_author NOT IN ($author__not_in) "inserts the raw string directly into the WHERE clause.- This is never passed through
$wpdb->prepare()before being run via$wpdb->get_results( $this->request ).
Fix: Mirror the author__in branch by keeping absint()/array_unique() unconditional on the final implode line, e.g. $author__not_in = implode( ',', array_map( 'absint', array_unique( (array) $q['author__not_in'] ) ) );, while still using the is_array() guard only to decide whether to write the sorted/deduped value back into $q['author__not_in'] for cache-key purposes.
| if ( $q['p'] ) { | ||
| $where .= " AND {$wpdb->posts}.ID = " . $q['p']; | ||
| } elseif ( $q['post__in'] ) { | ||
| $post__in = implode( ',', array_map( 'absint', $q['post__in'] ) ); | ||
| // Duplicate array before sorting to allow for the orderby clause. | ||
| $post__in_for_where = $q['post__in']; | ||
| $post__in_for_where = array_unique( array_map( 'absint', $post__in_for_where ) ); | ||
| sort( $post__in_for_where ); | ||
| $post__in = implode( ',', array_map( 'absint', $post__in_for_where ) ); | ||
| $where .= " AND {$wpdb->posts}.ID IN ($post__in)"; | ||
| } elseif ( $q['post__not_in'] ) { | ||
| sort( $q['post__not_in'] ); | ||
| $post__not_in = implode( ',', array_map( 'absint', $q['post__not_in'] ) ); | ||
| $where .= " AND {$wpdb->posts}.ID NOT IN ($post__not_in)"; | ||
| } |
There was a problem hiding this comment.
🟡 Both the post__not_in and post_parent__not_in branches call sort() but not array_unique() before building the NOT IN clause (class-wp-query.php around lines 2237-2242 and 2253-2258), unlike every sibling __in/__not_in variant added elsewhere in this PR. generate_cache_key()'s new normalization loop also only re-normalizes post__in/post_parent__in, not these two. As a result, e.g. post__not_in => [1,2,3] and post__not_in => [1,1,2,3] (functionally identical NOT IN clauses) produce different SQL and different cache keys, missing a cache hit that this PR otherwise aims to fix (ticket 59516). No correctness impact - just a one-line array_unique() fix for consistency.
Extended reasoning...
What the bug is: This PR's goal is to normalize equivalent WP_Query arguments so they produce the same cache key (Trac #59516). It does this for every list-style query var by calling array_unique() followed by sort() before the value is used to build the SQL WHERE clause, and by re-normalizing the corresponding entry in query_vars (or in generate_cache_key()'s copy of the args) so the serialized cache key matches too.
Two of the new call sites don't get the array_unique() treatment: post__not_in (around line 2239, sort( $q['post__not_in'] );) and post_parent__not_in (around line 2254, sort( $q['post_parent__not_in'] );). Both only sort in place - no dedup. Every sibling variant added in the same diff (post__in, post_parent__in, post_name__in, category__in/__not_in/__and, tag__in/__not_in/__and, tag_slug__in/__and, author__in/__not_in) calls array_unique() before (or as part of) sorting.
Code path: In WP_Query::get_posts(), when $q['post__not_in'] is set, the code does sort( $q['post__not_in'] ); $post__not_in = implode( ',', array_map( 'absint', $q['post__not_in'] ) );. If the caller passes duplicate IDs, they remain duplicated in both the SQL string and in $q['post__not_in'] itself (which feeds the serialized args used for the cache key in generate_cache_key()). The same pattern repeats for post_parent__not_in. Separately, generate_cache_key()'s new $sortable_arrays_with_int_values loop (which applies array_unique() + absint + sort()) only lists 'post__in' and 'post_parent__in' - the __not_in counterparts are absent from that list too, so there's no second chance to normalize them for the cache key.
Why existing code doesn't prevent it: The dedup step was added consistently to every other list var in this PR; these two were simply missed. There's no other normalization path - the only other array_unique-adjacent code near post__not_in is an unrelated array_diff used for sticky posts handling (around line 3573).
Step-by-step proof:
- Call new WP_Query( array( 'post__not_in' => array( 1, 2, 3 ) ) ). In get_posts(), sort() leaves it as [1,2,3]; SQL becomes ... NOT IN (1,2,3). generate_cache_key() serializes query_vars['post__not_in'] = [1,2,3] into the cache key.
- Call new WP_Query( array( 'post__not_in' => array( 1, 1, 2, 3 ) ) ). sort() leaves it as [1,1,2,3]; SQL becomes ... NOT IN (1,1,2,3). The serialized args contain [1,1,2,3].
- Both queries return exactly the same set of posts (NOT IN (1,2,3) and NOT IN (1,1,2,3) are semantically identical in SQL), but because the SQL string and the serialized query_vars differ, md5( serialize( $args ) . $sql ) produces two different cache keys - so the second query is a guaranteed cache miss even though it's asking for the same thing already cached by the first. This is exactly the scenario the PR's other list vars (e.g. post__in) are tested against in data_query_cache_duplicate() ('post__in non-unique'), but no equivalent 'post__not_in non-unique' / 'post_parent__not_in non-unique' case exists, confirming the gap.
Impact: None on correctness - a NOT IN clause with duplicate values returns identical results to one without. The only effect is a missed object-cache hit when a caller happens to pass duplicate IDs into post__not_in or post_parent__not_in, which is an uncommon but plausible input (e.g. IDs merged from multiple exclusion lists upstream).
Fix: Add $q['post__not_in'] = array_unique( $q['post__not_in'] ); (and the equivalent for post_parent__not_in) before the existing sort() calls, and add 'post__not_in' / 'post_parent__not_in' to the $sortable_arrays_with_int_values list in generate_cache_key() for consistency with post__in/post_parent__in. This is a one-line-per-site fix, matching the pattern already used everywhere else in the diff.
2 participants
Lighter touch alternative to WordPress/wordpress-develop#5347
What
Improves the cache hits for WP_Query
How
It sorts query variable whenever possible. This results in the SQL queries being normalised so that it doesn't produce different SQL for the same effective arguments. For example category__in => [1,2] and category__in => [2,1] produce the same IN() clause.
Why
Currently the SQL queries and the generated cache key differ for the same effective arguments.
Trac ticket: https://core.trac.wordpress.org/ticket/59516
Note
Medium Risk
Touches core query parsing and SQL WHERE generation across many code paths; behavior changes only argument ordering in query_vars/SQL, but incorrect normalization could theoretically affect edge-case queries or tests that assumed unsorted IDs.
Overview
Improves WP_Query object-cache hit rate by treating equivalent query arguments as the same request, so duplicate SQL and mismatched cache keys are less common (Trac #59516).
During parse and SQL building, list-style vars (
post_type,post_status, categories/tags, authors,post__in/post__not_in, etc.) are deduplicated and sorted so permutations likecategory__in => [2,1]vs[1,2]build the sameIN (...)clause. Fororderbyofpost__in,post_parent__in, orpost_name__in, copies are sorted only for the WHERE clause while the original argument order stays intact forFIELD(...)ordering and distinct cache keys when order matters.generate_cache_key()applies the same normalization on a copy of args (including sortedpost__in/post_parent__in/post_name__inwhen not order-sensitive) and stores the result on a new$query_cache_keyproperty. PHPUnit coverage adds equivalence cases and asserts order-by-array queries still diverge.Reviewed by Cursor Bugbot for commit 9e6932e. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests