Skip to content

Code Modernization: Fix null to non-nullable deprecation in WP_Term_Query::parse_orderby()#12626

Open
itzmekhokan wants to merge 1 commit into
WordPress:trunkfrom
itzmekhokan:fix/65679-term-query-orderby-null-deprecation
Open

Code Modernization: Fix null to non-nullable deprecation in WP_Term_Query::parse_orderby()#12626
itzmekhokan wants to merge 1 commit into
WordPress:trunkfrom
itzmekhokan:fix/65679-term-query-orderby-null-deprecation

Conversation

@itzmekhokan

Copy link
Copy Markdown

WP_Term_Query::parse_orderby() passes the raw orderby query var straight to strtolower(). When that value is null, 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 ) ) emitted strtolower(): Passing null to parameter #1 ($string) of type string is deprecated on PHP 8.1 and above, at wp-includes/class-wp-term-query.php:921.
  • 'orderby' defaults to 'name', but wp_parse_args() lets a caller override it with an explicit null, so the value reaches strtolower() unguarded.
  • The method already handles a falsy value correctly further down — the empty( $_orderby ) branch orders by t.term_id. Only the notice was wrong.

What the fix does:

  • Casts the raw value to string before lowercasing: strtolower( (string) $orderby_raw ).
  • Adds a regression test covering a null orderby.

Approach and why:

  • One line, at the exact point of failure. This matches how Core has fixed the same class of issue in sibling query classes — see [51806], which guarded WP_Comment_Query::get_comment_ids() at its call site rather than reworking the method.
  • A (string) cast was chosen over an is_string() guard (the idiom used by the neighbouring parse_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.
  • An is_string() guard would not be behaviour-preserving: parse_orderby_meta() resolves the value via array_key_exists( $orderby_raw, $meta_clauses ), and unnamed meta_query clauses receive integer keys. PHP normalises numeric string keys to integers, so array_key_exists( '5', array( 5 => ... ) ) is true — meaning a numeric orderby can legitimately reference a meta clause today. Discarding non-strings would silently break that ordering.
  • The @param string docblock 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.

…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.
Copilot AI review requested due to automatic review settings July 21, 2026 13:41
@github-actions

github-actions Bot commented Jul 21, 2026

Copy link
Copy Markdown

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.

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

Props khokansardar, soean.

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

@github-actions

Copy link
Copy Markdown

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

  • 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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 orderby value before lowercasing in WP_Term_Query::parse_orderby().
  • Adds a term query test asserting that orderby => null falls 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.

Comment on lines 920 to 922
protected function parse_orderby( $orderby_raw ) {
$_orderby = strtolower( $orderby_raw );
$_orderby = strtolower( (string) $orderby_raw );
$maybe_orderby_meta = false;
@Soean

Soean commented Jul 21, 2026

Copy link
Copy Markdown
Member

orderby should be a string, why should we allow null? What is the usecase?

https://developer.wordpress.org/reference/classes/wp_term_query/__construct/#parameters

@itzmekhokan

Copy link
Copy Markdown
Author

orderby should be a string, why should we allow null? What is the usecase?

https://developer.wordpress.org/reference/classes/wp_term_query/__construct/#parameters

Fair question — you're right that nothing in core passes null here. Every core call site into WP_Term_Query passes a string literal, so this can only originate in plugin or theme code. There's no legitimate use case for null, and I don't think there should be one.

One things that made me think it's still worth hardening, the sibling query var is already hardened in this same class. $order is documented as @type string exactly like $orderby, yet parse_order() has guarded against non-strings since WP_Term_Query was introduced in [37572]:

if ( ! is_string( $order ) || empty( $order ) ) {
     return 'DESC';
}

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 class-wp-term-query.php:921 with nothing pointing at the actual caller.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants