Code Modernization: Fix null to non-nullable deprecation in WP_Term_Query::parse_orderby()#12626
Conversation
…Query::parse_orderby()`. Passing `null` as the `orderby` query var triggered a `strtolower(): Passing null to parameter WordPress#1 ($string) of type string is deprecated` notice on PHP 8.1 and above. Cast the raw value to string before lowercasing. This preserves the existing behaviour for every scalar type, including numeric values that may reference an unnamed `meta_query` clause key, while a `null` value continues to fall through to the existing `empty()` check and order by term ID. Adds a regression test. Fixes #65679.
|
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 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. |
There was a problem hiding this comment.
Pull request overview
This PR updates WP_Term_Query::parse_orderby() to avoid PHP 8.1+ deprecation notices caused by passing null to strtolower() when orderby is explicitly set to null, and adds a PHPUnit regression test for that scenario.
Changes:
- Casts the raw
orderbyvalue before lowercasing inWP_Term_Query::parse_orderby(). - Adds a term query test asserting that
orderby => nullfalls back to ordering by term ID.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/phpunit/tests/term/getTerms.php | Adds a regression test ensuring orderby => null orders by term ID. |
| src/wp-includes/class-wp-term-query.php | Adjusts parse_orderby() to prevent PHP 8.1+ null-to-string deprecation in strtolower(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| protected function parse_orderby( $orderby_raw ) { | ||
| $_orderby = strtolower( $orderby_raw ); | ||
| $_orderby = strtolower( (string) $orderby_raw ); | ||
| $maybe_orderby_meta = false; |
|
https://developer.wordpress.org/reference/classes/wp_term_query/__construct/#parameters |
Fair question — you're right that nothing in core passes One things that made me think it's still worth hardening, the sibling query var is already hardened in this same class. So order is defended and orderby isn't — that asymmetry looks more like an oversight than a deliberate line. It's really about where the failure surfaces — the notice names If you'd rather not absorb caller type errors here, I'm happy to close this and have the ticket resolved as invalid — your call. |
WP_Term_Query::parse_orderby()passes the raworderbyquery var straight tostrtolower(). When that value isnull, PHP 8.1+ emits a deprecation notice. This casts the value to string first, which silences the notice without altering any existing behaviour.What the problem was:
get_terms( array( 'orderby' => null ) )emittedstrtolower(): Passing null to parameter #1 ($string) of type string is deprecatedon PHP 8.1 and above, atwp-includes/class-wp-term-query.php:921.'orderby'defaults to'name', butwp_parse_args()lets a caller override it with an explicitnull, so the value reachesstrtolower()unguarded.empty( $_orderby )branch orders byt.term_id. Only the notice was wrong.What the fix does:
strtolower( (string) $orderby_raw ).nullorderby.Approach and why:
WP_Comment_Query::get_comment_ids()at its call site rather than reworking the method.(string)cast was chosen over anis_string()guard (the idiom used by the neighbouringparse_order()) deliberately. The cast reproduces PHP's pre-8.1 implicit coercion exactly for every scalar type —null/false→'',5→'5',true→'1'— so behaviour is unchanged for all of them.is_string()guard would not be behaviour-preserving:parse_orderby_meta()resolves the value viaarray_key_exists( $orderby_raw, $meta_clauses ), and unnamedmeta_queryclauses receive integer keys. PHP normalises numeric string keys to integers, soarray_key_exists( '5', array( 5 => ... ) )istrue— meaning a numericorderbycan legitimately reference a meta clause today. Discarding non-strings would silently break that ordering.@param stringdocblock is left as-is; the cast is defensive against callers, not a signature change.Trac ticket: https://core.trac.wordpress.org/ticket/65679
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 4.8
Used for: Writting test case and PR desription. All changes were reviewed and tested by me.
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.