Make WordPress Core

Opened 18 months ago

Last modified 7 weeks ago

#63256 reviewing defect (bug)

Unnecessary array_map() call in get_terms() when object_ids is not set

Reported by: dilipbheda Owned by: audrasjb
Priority: lowest Milestone: Future Release
Component: Taxonomy Version: 4.7
Severity: normal Keywords: has-patch has-unit-tests needs-testing has-test-info early
Cc: Focuses:

Description

The get_terms() function always triggers the else block because the result is cast to an array and sent to array_map(), even when object_ids isn't set. This is unnecessary since the default value of object_ids is null.

The default value here is null: ​https://github.com/WordPress/wordpress-develop/blob/ac648a15245df2261c0782f7e1c618911f392601/src/wp-includes/class-wp-term-query.php#L199C1-L199C4

The else block runs here: ​https://github.com/WordPress/wordpress-develop/blob/ac648a15245df2261c0782f7e1c618911f392601/src/wp-includes/class-wp-term-query.php#L596-L600

Attachments (2)

before-63256.png​ (123.5 KB ) - added by ozgursar 5 months ago.
Before applying PR 11534
after-63256.png​ (121.2 KB ) - added by ozgursar 5 months ago.
After applying PR 11534

Download all attachments as: .zip

Change History (46)

This ticket was mentioned in ​PR #8668 on ​WordPress/wordpress-develop by ​@dilipbheda.


18 months ago
#1

#2 @audrasjb
18 months ago

  • Milestone Awaiting Review → 6.9
  • Owner set to audrasjb
  • Status new → reviewing

Makes sense, thanks.

#3 @rollybueno
13 months ago

  • Keywords needs-testing added

Adding needs testing keyword. While the code update is quite simple, we still need to be sure since get_terms() might be used extensively by some themes and plugins.

#4 @staurand
12 months ago

The only edge case I can see is the integer 0.
From a quick search on Github it's quite common to pass an integer as value for "object_ids".
In that case it would be a breaking change.
Before 0 => array(0) => potentially no result
After 0 => array() => no filter

Moreover the doc says:
"Object ID, or array of object IDs. Results will be limited to terms associated with these objects."
So 0 should be a valid value and it should filter results for terms associated with the objects with the ID equals to 0.

#5 @westonruter
12 months ago

@staurand A term ID cannot actually be zero though, can it? Term IDs are auto-incremented integers starting at 1, right?

#6 @staurand
12 months ago

@westonruter
The ticket is about the "object_ids" param, so I don‘t think it's related to term ID.

It’s in practice related to post ids or other kind of "object IDs".
But as the doc states, "Object ID, or array of object IDs. Results will be limited to terms associated with these objects.", we can’t presume the kind of object it will be.

So 0 should be a valid value and, as you stated, if the object is using an auto-incremented integer that will start at 1, this function should return no result (no object id will match 0, so no object with the id 0 is linked to a term).

With the proposed patch it will return all terms (no filter).

I hope the logic above makes sense :)

#7 @westonruter
12 months ago

@staurand You're right.

The object_ids param was made agnostic to the object type so that it could be used to relate posts as well as users, both of which have numeric IDs. It is running intval over all of the object IDs. Additionally, the object_ids are being ultimately used as part of the SQL here:

$this->sql_clauses['where']['object_ids'] = "tr.object_id IN ($object_ids)";

The tr table here is term_relationships (​schema), where the object_id field has this type:

object_id bigint(20) unsigned NOT NULL default 0,

So one thing is for sure is that it should be an array of integers.

So what if someone passes 0 as object_ids? That seems to be the problem here that you've identified. Passing array( 0 ) won't be a problem, since empty( array( 0 ) ) is false. So with the current patch:

get_terms( array( 'object_ids' => 0 ) ); // FAIL
get_terms( array( 'object_ids' => array( 0 ) ) ); // OK

So it seems we can address the problem with a slight tweak to the patch:

- if ( 'term_order' === $_orderby && empty( $this->query_vars['object_ids'] ) ) { 
+ if ( empty( $args['object_ids'] ) && ! is_numeric( $args['object_ids'] ) ) {

I don't know how common it is to have term_relationships rows which have an object_id set to 0.

#8 @westonruter
12 months ago

  • Keywords needs-unit-tests added

​@westonruter commented on ​PR #8668:


12 months ago
#9

Let's also include a unit test for this change.

#10 @staurand
12 months ago

@westonruter

We are overiding "$this->query_vars['object_ids']" later in the code (that was what the patch changed).

So we may check earlier if the param is valid then re-use the logic in the two places where we need it.

		Index: src/wp-includes/class-wp-term-query.php
===================================================================
--- src/wp-includes/class-wp-term-query.php	(revision 61066)
+++ src/wp-includes/class-wp-term-query.php	(working copy)
@@ -439,9 +439,11 @@
 			}
 		}
 
+		$is_object_ids_param_valid = ! empty( $this->query_vars['object_ids'] ) || is_numeric( $this->query_vars['object_ids'] );
+
 		// 'term_order' is a legal sort order only when joining the relationship table.
 		$_orderby = $this->query_vars['orderby'];
-		if ( 'term_order' === $_orderby && empty( $this->query_vars['object_ids'] ) ) {
+		if ( 'term_order' === $_orderby && ! $is_object_ids_param_valid ) {
 			$_orderby = 'term_id';
 		}
 
@@ -590,7 +592,7 @@
 			);
 		}
 
-		if ( '' === $args['object_ids'] ) {
+		if ( ! $is_object_ids_param_valid ) {
 			$args['object_ids'] = array();
 		} else {
 			$args['object_ids'] = array_map( 'intval', (array) $args['object_ids'] );


#11 @westonruter
12 months ago

@staurand Good idea.

@dilipbheda Could you apply this feedback?

#12 @westonruter
12 months ago

  • Priority normal → lowest
  • Version → 4.7

The object_ids arg was introduced in r38667 (#37198), but it doesn't seem the original code had the issue:

<?php
if ( ! empty( $args['object_ids'] ) ) {
        $object_ids = $args['object_ids'];
        if ( ! is_array( $object_ids ) ) {
                $object_ids = array( $object_ids );
        }

        $object_ids = implode( ', ', array_map( 'intval', $object_ids ) );
        $this->sql_clauses['where']['object_ids'] = "tr.object_id IN ($object_ids)";
}

I any case, "issue" is a strong word because it's not like wastefully calling array_map() with intval is going to have any noticeable performance impact in any way.

Last edited 12 months ago by westonruter (previous) (diff)

This ticket was mentioned in ​Slack in #core-test by jon_bossenger. ​View the logs.


11 months ago

This ticket was mentioned in ​Slack in #core-test by krupajnanda. ​View the logs.


11 months ago

#15 @wildworks
11 months ago

  • Keywords changes-requested added; needs-testing removed
  • Milestone 6.9 → 7.0

Since the 6.9 RC1 release is approaching, I'd like to punt this ticket to 7.0.

As I understand it, we need to address two things in order to move this ticket forward.

This ticket was mentioned in ​PR #10634 on ​WordPress/wordpress-develop by ​@abcd95.


10 months ago
#16

  • Keywords has-unit-tests added; needs-unit-tests removed

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

Adds unit test coverage for the fix implemented in #8668 that addresses unnecessary array_map() processing in WP_Term_Query::get_terms() when object_ids is not set.

This PR -

  • Verifies that when object_ids is null (default), all terms are returned without filtering
  • Ensures the optimization skips unnecessary array_map() processing
  • Validates that numeric zero (0) is treated as a valid object ID
  • Prevents regression where 0 might be incorrectly treated as empty

### Testing

npm run test:php -- --filter=Tests_Term_Query

Follows the patch of ​https://github.com/WordPress/wordpress-develop/pull/8668 by @dilipbheda

​@westonruter commented on ​PR #10634:


10 months ago
#17

I've added 3d7f52aa8d653a38ee624059c7e0137bdc5a6ca9 to account for the case for when someone may have supplied 'object_ids' => false. This will now preserve the original behavior of returning no terms. The same tests pass in trunk as in this branch.

#18 @westonruter
10 months ago

  • Keywords changes-requested removed

@staurand @dilipbheda @abcd95 I've further iterated on the PR a bit to handle a possible back-compat breakage where if someone had supplied 'object_ids' => false. Now it will ensure no terms are returned as before.

Please review.

​@westonruter commented on ​PR #10634:


10 months ago
#19

Alas, the Tests_Term_getTerms::test_cache_key_generation test is now failing with the data set "array object_ids vs string object_ids".

#20 @staurand
9 months ago

Hi @westonruter

That sounds odd to have a different behaviour between the values null and false, no?

They are both invalid values (expecting int|int[]).

Thanks.

This ticket was mentioned in ​Slack in #core by juanmaguitar. ​View the logs.


7 months ago

#22 @juanmaguitar
7 months ago

  • Keywords needs-testing added

From today's bug scrub

It seems ​https://github.com/WordPress/wordpress-develop/pull/10634 adds the required tests but also its own implementation of the solution suggested in ​https://github.com/WordPress/wordpress-develop/pull/8668

@westonruter as the person who added latest commits to ​https://github.com/WordPress/wordpress-develop/pull/10634, do you think we can move forward with the patch in time for WP 7.0 RC1

Also pinging @audrasjb as the owner of this ticket

I'm also adding "needs-testing" so the patch can be confirmed

#23 @westonruter
7 months ago

I'm not going to have time to get this over the finish line for 7.0.

#24 @audrasjb
7 months ago

  • Milestone 7.0 → 7.1

Thanks @juanmaguitar @westonruter, moving to 7.1.

This ticket was mentioned in ​Slack in #core-test by r1k0. ​View the logs.


5 months ago

@ozgursar
5 months ago

Before applying PR 11534

@ozgursar
5 months ago

After applying PR 11534

#27 @ozgursar
5 months ago

Test Report

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/11534

Environment

  • WordPress: 7.1-alpha-62161-src
  • Subdirectory: No
  • PHP: 8.2.29
  • Server: nginx/1.29.4
  • Database: mysqli (Server: 8.4.7 / Client: mysqlnd 8.2.29)
  • Browser: Chrome 148.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty-Five 1.5
  • MU Plugins: None activated
  • Plugins:
    • Code Snippets 3.9.6
    • Test Reports 1.3.0

Steps taken

  1. Add the following snippet to your active theme's functions.php or via Code Snippets plugin to see the results in Admin Notification section of the dashboard.
add_action( 'admin_notices', function () {
	if ( ! current_user_can( 'manage_options' ) ) return;

	$file       = file_get_contents( ABSPATH . 'wp-includes/class-wp-term-query.php' );
	$is_patched = $file && str_contains( $file, "null === \$args['object_ids']" );
	$cases      = [ null, '', 0, false, 1 ];

	echo '<div class="notice notice-info">';
	echo '<p><strong>Trac #63256 &mdash; array_map() behavior per object_ids value in get_terms()</strong></p>';
	echo '<table style="border-collapse:collapse">';
	echo '<tr>'
		. '<th style="padding:4px 8px;border:1px solid #ccc">object_ids</th>'
		. '<th style="padding:4px 8px;border:1px solid #ccc">Result</th>'
		. '</tr>';

	foreach ( $cases as $val ) {
		if ( $is_patched ) {
			$result = ( null === $val || '' === $val )
				? 'skip → []'
				: 'array_map → ' . var_export( array_map( 'intval', (array) $val ), true );
		} else {
			$result = ( '' === $val )
				? 'skip → []'
				: 'array_map → ' . var_export( array_map( 'intval', (array) $val ), true );
		}

		$bg = ( null === $val ) ? '#fff3cd' : '#fff';

		echo '<tr style="background:' . $bg . '">'
			. '<td style="padding:4px 8px;border:1px solid #ccc"><code>' . esc_html( var_export( $val, true ) ) . '</code></td>'
			. '<td style="padding:4px 8px;border:1px solid #ccc">' . esc_html( $result ) . '</td>'
			. '</tr>';
	}

	echo '</table></div>';
} );

  1. Observe object_ids NULL does not skip and results as array_map → array ( )
  2. Apply the patch
  3. NULL now correctly skips array_map(). All other values behave identically to unpatched version
  4. ✅ Patch is solving the problem

Expected result

  • When object_ids is not set (defaults to null), array_map() should be skipped.
  • All other object_ids values ('', 0, false, valid IDs) should behave unchanged.

Screenshots/Screencast with results

Before
https://core.trac.wordpress.org/raw-attachment/ticket/63256/before-63256.png

After
https://core.trac.wordpress.org/raw-attachment/ticket/63256/after-63256.png

This ticket was mentioned in ​Slack in #core by nimeshatxecurify. ​View the logs.


4 months ago

#29 @nimeshatxecurify
4 months ago

Refreshed the patch on ​PR #11534. Please review and let me know if any further adjustments are required.

#30 @vedantere
4 months ago

Test Report

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/11534
Environment

WordPress: 7.1-alpha-62161-src
PHP: 8.5.6 (Homebrew)
OS: macOS (Apple Silicon, arm64)
Docker: linux/amd64 image via Rosetta emulation

Steps taken

Fetched PR #11534 via git fetch upstream pull/11534/head:pr-11534
Checked out the branch with git checkout pr-11534
Ran the full Tests_Term_Query suite: npm run test:php -- --filter=Tests_Term_Query
Ran the two new tests individually to confirm they pass

Results

Full suite: 44/44 tests passed, 109 assertions ✅
test_object_ids_null_returns_all_terms: 1/1 passed, 3 assertions ✅
test_object_ids_zero_is_treated_as_numeric: 1/1 passed, 1 assertion ✅

✅ Patch is working as expected

Use of AI: Yes
Yes, used Claude (claude.ai) to understand the ticket's change history, identify the relevant edge case discussion around object_ids values, and structure the test report format following project conventions.

#31 @jamesbregenzer
4 months ago

Tested ​PR 11534 against current trunk.

Applied the patch locally and verified the change in src/wp-includes/class-wp-term-query.php.

Ran the relevant taxonomy PHPUnit tests:

npm run test:php -- --filter Tests_Term

Result:

OK (630 tests, 1628 assertions)

Also performed a basic admin smoke test by creating a category, assigning a post to that category, confirming the post appeared correctly in the admin post list, and viewing both the category archive and post on the front end.

The patch applied cleanly, tests passed, and I did not observe any regressions in the tested taxonomy workflows.

#32 in reply to: ↑ description @shailesh7714
3 months ago

Thanks for reporting this. I checked the flow in get_terms() and the issue is valid.

The else block always executes because object_ids is cast to an array before being passed into array_map(). So even when object_ids is null, it becomes an empty array and still triggers array_map(), which is unnecessary.

Current behavior (unnecessary call):
$object_ids = null;
$object_ids = (array) $object_ids; []
$result = array_map( 'intval', $object_ids );
still runs

Expected / optimized behavior:
if ( ! empty( $object_ids ) ) {

$object_ids = array_map( 'intval', (array) $object_ids );

}

#33 @monzuralam
3 months ago

Testing Report

Environment

  • OS: Windows 11
  • Web Server: Linux 6.8.0-100-generic x86_64
  • PHP: 8.5.3 (Supports 64bit values)
  • WordPress: 7.1-beta1-62761
  • Browser: Chrome 150.0.7871.115
  • Theme: Astra 4.13.6
  • Active Plugins:
    • WordPress Beta Tester 4.0.0

Steps to Reproduce

  1. Edit theme functions.php
  2. Paste code
add_action( 'admin_notices', function () {
	if ( ! current_user_can( 'manage_options' ) ) return;

	$file       = file_get_contents( ABSPATH . 'wp-includes/class-wp-term-query.php' );
	$is_patched = $file && str_contains( $file, "null === \$args['object_ids']" );
	$cases      = [ null, '', 0, false, 1 ];

	echo '<div class="notice notice-info">';

	echo '<p><strong>Trac #63256 &mdash; array_map() behavior per object_ids value in get_terms()</strong></p>';

	echo '<table style="border-collapse:collapse">';

	echo '<tr>'
		. '<th style="padding:4px 8px;border:1px solid #ccc">object_ids</th>'
		. '<th style="padding:4px 8px;border:1px solid #ccc">Result</th>'
		. '</tr>';

	foreach ( $cases as $val ) {
		if ( $is_patched ) {
			$result = ( null === $val || '' === $val )
				? 'skip → []'
				: 'array_map → ' . var_export( array_map( 'intval', (array) $val ), true );
		} else {
			$result = ( '' === $val )
				? 'skip → []'
				: 'array_map → ' . var_export( array_map( 'intval', (array) $val ), true );
		}

		$bg = ( null === $val ) ? '#fff3cd' : '#fff';

		echo '<tr style="background:' . $bg . '">'
			. '<td style="padding:4px 8px;border:1px solid #ccc"><code>' . esc_html( var_export( $val, true ) ) . '</code></td>'
			. '<td style="padding:4px 8px;border:1px solid #ccc">' . esc_html( $result ) . '</td>'
			. '</tr>';
	}

	echo '</table></div>';

} );
  1. Observe that when object_ids is NULL, it does not skip the array_map() call and instead results in array_map() → array().
  1. Apply the patch.
  1. Verify that NULL now correctly skips the array_map() call, while all other object_ids values continue to behave identically to the unpatched implementation.

Expected Results

✅ The patch successfully resolves the issue.

Screenshot

[Before apply the patch](​https://postimg.cc/TKk3wF70)
[After apply the](​https://postimg.cc/XXcGWHWM)

​@wildworks commented on ​PR #10634:


2 months ago
#34

@himanshupathak95, Can you investigate and fix the failing unit tests?

This ticket was mentioned in ​Slack in #core-test by noruzzaman. ​View the logs.


2 months ago

This ticket was mentioned in ​Slack in #core-test by softglaze. ​View the logs.


2 months ago

#37 @mikekipruto
2 months ago

Testing Report

Environment:

  • WordPress: 7.1-alpha-src / trunk
  • PHP: 8.2.29 / 8.3.0
  • MySQL: 8.4
  • OS: macOS
  • PHPUnit: 9.6.34

Verification Summary:

  1. Patch & PR Inspection:
    • Pulled PR #11534 (src/wp-includes/class-wp-term-query.php & tests/phpunit/tests/term/query.php).
    • Verified that null === $args['object_ids'] || '' === $args['object_ids'] skips the unnecessary array_map('intval') call when object_ids is not set (defaulting to null).
    • Confirmed that numeric 0 continues to be treated as a numeric value rather than unset.
  1. Automated Unit Testing:
    • Executed PHPUnit test suite: vendor/bin/phpunit tests/phpunit/tests/term/query.php.
    • Result: OK (44 tests, 109 assertions) — 100% passing.
    • test_object_ids_null_returns_all_terms and test_object_ids_zero_is_treated_as_numeric passed cleanly.
  1. Coding Standards:
    • Ran vendor/bin/phpcs on src/wp-includes/class-wp-term-query.php and tests/phpunit/tests/term/query.php.
    • Result: 0 errors and 0 warnings.

Verdict: ✅ Patch tested & verified. Fix functions as expected without regressions.

#38 @softglaze
2 months ago

  • Keywords has-test-info added

Test Report

The existing reports confirm the array_map() skip. This one compares actual get_terms() behaviour across trunk and both open implementation PRs, since PR 10634 and PR 11534 fix the same thing differently and only one of them can be committed.

Environment

WordPress 7.1-beta4-62899-src
Trunk 4eaba0e054 (30 Jul 2026)
PR 10634 head 7363d68881
PR 11534 head 62ee55d490
PHP 8.3.32
Database MariaDB 11.8.6
Environment wordpress-develop Docker serving src/, WSL2, Windows 11 host

Behaviour matrix

WP_Term_Query with fields => ids, hide_empty => false, and a unique cache_domain per query so caching cannot affect the comparison. One post assigned to term A; term B left unassigned.

object_ids trunk PR 10634 PR 11534
null all terms all terms all terms
'' all terms all terms all terms
0 [] [] []
false [] [] []
valid post ID that term only that term only that term only
array( 0 ) [] [] []

Both patches are behaviour-preserving on every input tested. That addresses two concerns raised earlier on this ticket:

  • comment:4 — 0 does not become "no filter" under either patch. (array) 0 is array( 0 ), which is non-empty, so the tr.object_id IN (0) clause is still applied and no terms match.
  • comment:17 / comment:18 — the same holds for false.

Test runs

Suite trunk PR 10634 PR 11534
Tests_Term_Query OK (42, 105) OK (45, 110) OK (44, 109)
Tests_Term OK (628, 1622) OK (631, 1627) OK (630, 1628)
test_cache_key_generation OK (20, 44) OK (20, 44) OK (20, 44)
test_cache_key_generation (multisite) OK (20, 44) OK (20, 44) OK (20, 44)

On the failing test in comment:34

At head 7363d68881 the test passes locally in both single-site and multisite. The branch contains a commit titled "Fix failing test test_cache_key_generation for object_ids", so this appears to have been addressed already. @wildworks — worth re-checking CI against the current head rather than an earlier run.

The difference between the two implementations

The two patches leave a different value in $args['object_ids'] when the input is null.

PR 10634:

if ( '' === $args['object_ids'] ) {
        $args['object_ids'] = array();
} elseif ( null !== $args['object_ids'] ) {
        $args['object_ids'] = array_map( 'intval', (array) $args['object_ids'] );
}

With null, neither branch is entered, so the value stays null.

PR 11534:

if ( null === $args['object_ids'] || '' === $args['object_ids'] ) {
        $args['object_ids'] = array();
} else {
        $args['object_ids'] = array_map( 'intval', (array) $args['object_ids'] );
}

This normalises to array(), which is what trunk produces via array_map( 'intval', (array) null ).

Query results are identical either way, since empty() treats null and array() the same at every point the value is used downstream. But $args also feeds cache key generation, and there the two are not equivalent.

Measured by counting queries for null, then null again as a control, then '', all sharing one cache_domain after a wp_cache_flush():

Sequence trunk PR 10634 PR 11534
null (cold) 2 2 2
null again (control) 0 0 0
'' 0 1 0

The control row is identical everywhere, so the measurement is sound. The single difference is that under PR 10634 the '' query does not reuse the cache entry written by the null query — it generates a separate cache key and re-runs the term ID lookup. The term objects themselves stay cached by ID, which is why the cost is one query rather than two.

On that basis PR 11534 looks like the safer of the two. It leaves the normalised value identical to trunk for every input, so no cache key changes.

Note on a second location

The term_order fallback is unchanged by both patches:

if ( 'term_order' === $_orderby && empty( $this->query_vars['object_ids'] ) ) {
        $_orderby = 'term_id';
}

With object_ids => 0, empty() is true so term_order is downgraded to term_id, even though the WHERE clause is applied and the relationship table is joined. This is the second location raised in comment:10. It is pre-existing rather than introduced here, so it may be better handled as a separate ticket than by widening this one.

The question in comment:20 — whether null and false should behave differently when both are invalid for int|int[] — is still open, and it is what decides which implementation is correct. I'm not answering it here.

Tested at the head hashes above. Happy to retest whichever PR is chosen.

This ticket was mentioned in ​Slack in #core-test by softglaze. ​View the logs.


2 months ago

​@softglaze commented on ​PR #10634:


2 months ago
#40

Tested at head 7363d68881 — full report on Trac: https://core.trac.wordpress.org/ticket/63256

test_cache_key_generation passes locally in both single-site and multisite (PHP 8.3.32, MariaDB 11.8.6), so the failure raised on Trac looks like it predates the "Fix failing test test_cache_key_generation for object_ids" commit on this branch.

One thing worth flagging while the choice between this PR and #11534 is open. With object_ids => null, neither branch of the condition here is entered, so $args['object_ids'] stays null where trunk produces array(). Query results are unaffected, but $args feeds cache key generation — measured with a shared cache_domain after a flush, querying null then '' costs one extra query on this branch and zero on both trunk and #11534, because the two inputs no longer share a cache entry.

Numbers and the full behaviour matrix are in the Trac report.

​@ugyensupport commented on ​PR #11534:


2 months ago
#41

# Test Report — PR #11534

Ticket: #63256 — Unnecessary `array_map()` call in `get_terms()` when `object_ids` is not set
PR: ​WordPress/wordpress-develop#11534 — "63256 null check added" by @nimesh-xecurify
Component: Taxonomy · Milestone: 7.1 · CI: All checks passing

---

## What the patch does

WP_Term_Query::get_terms() normalizes the object_ids argument before use. The default value of object_ids is null, but the guard only tested for empty string:

// before
if ( '' === $args['object_ids'] ) {
    $args['object_ids'] = array();
} else {
    $args['object_ids'] = array_map( 'intval', (array) $args['object_ids'] );
}

Because '' === null is false, the default null fell through to the else branch and ran array_map( 'intval', (array) null ) — an unnecessary call that produces the same array() the if branch would have set directly. The patch adds a null check so the default takes the cheap branch:

// after (PR 11534)
if ( null === $args['object_ids'] || '' === $args['object_ids'] ) {
    $args['object_ids'] = array();
} else {
    $args['object_ids'] = array_map( 'intval', (array) $args['object_ids'] );
}

Files changed (+46 / -1):

  • src/wp-includes/class-wp-term-query.php — the condition above.
  • tests/phpunit/tests/term/query.php — two new tests: test_object_ids_null_returns_all_terms() and test_object_ids_zero_is_treated_as_numeric().

---

## Environment

Verification method Static review + isolated PHP reproduction of the branch logic
PHP 7.4.33 (isolated logic harness)
PR head reviewed against the diff on GitHub, CI green

Scope note: I verified the normalization logic in isolation and reviewed the patch, tests, and ticket history. I did not stand up a full wordpress-develop + DB instance to run the live PHPUnit suite here — that part is corroborated by the two independent reports already on the ticket (comment:37 and comment:38), cited below.

---

## Behaviour verification

I reproduced the exact if/else normalization for both trunk and the patch across every input raised on the ticket. The question that matters is: does the patch change the value stored in $args['object_ids'] for any input? It must not — only skip the redundant call for the unset case.

object_ids input trunk output PR 11534 output array_map() skipped?
null (default) [] [] ✔ yes — this is the fix
'' [] [] ✔ yes
0 (int) [0] [0] no — still filtered
false [0] [0] no
array(0) [0] [0] no
5 (valid ID) [5] [5] no
array(5, 6) [5, 6] [5, 6] no

Result: byte-identical output for every input. The only change is that null and '' now bypass array_map(). This directly addresses the two regression concerns from the ticket discussion:

  • comment:4 — the 0 edge case. 0 is not caught by the new null/'' guard (both use strict ===), so it still falls to the else branch and becomes array( 0 ) → tr.object_id IN (0). object_ids => 0 remains a real filter that returns no terms, not "no filter." No breaking change.
  • comment:17/18 — false. Same path as 0; normalizes to array( 0 ), behaviour preserved.

---

## Unit tests (in the PR)

Both added tests match the verified behaviour:

  • test_object_ids_null_returns_all_terms() — object_ids => null returns all terms (no filtering). ✔
  • test_object_ids_zero_is_treated_as_numeric() — object_ids => 0 returns no terms (treated as a numeric filter, not unset). ✔

Independent full-suite runs already posted on the ticket:

  • comment:37 — vendor/bin/phpunit tests/phpunit/tests/term/query.php → OK (44 tests, 109 assertions); PHPCS clean.
  • comment:38 — cross-PR behaviour matrix on WP 7.1-beta4, PHP 8.3.32 / MariaDB 11.8.6: Tests_Term_Query OK (44, 109), Tests_Term OK (630, 1628), cache-key tests green.

---

## Observations / notes for the committer

  1. Cache-key parity (in this PR's favour). Per comment:38, PR 11534 normalizes null → array(), which is exactly what trunk produces. The competing PR #10634 leaves null as null, which diverges from trunk in cache-key generation (an extra query when null then '' are queried under a shared cache_domain). PR 11534 keeps the stored value identical to trunk for every input, so no cache key changes — the safer of the two implementations.
  1. term_order fallback is untouched (pre-existing, out of scope). The second location flagged in comment:10 —
    if ( 'term_order' === $_orderby && empty( $this->query_vars['object_ids'] ) ) {
           $_orderby = 'term_id';
       }
    

still uses empty(), so object_ids => 0 downgrades term_order to term_id even though the WHERE clause is applied. This is pre-existing behaviour, not introduced by this PR; better handled as a separate ticket than by widening this one.

  1. Open design question (comment:20). Whether null and false *should* behave differently as invalid values for int|int[] is still open and is orthogonal to this specific fix — this PR keeps both aligned with trunk.

---

## Verdict

✅ Works as intended, no regressions found.

The patch removes the redundant array_map() call for the default/unset case while producing output identical to trunk for every tested input, including the 0, false, and array(0) edge cases that earlier reviews were concerned about. Added unit tests are correct and cover the fix. CI is green and two independent full-suite reports on the ticket confirm passing tests. Recommend for commit consideration; the cache-key parity note gives it an edge over the alternative PR #10634.

---
<sub>🤖 This test report was prepared with the help of Claude (Claude Code). Logic verified in isolation; full-suite results cited from existing Trac reports rather than re-run locally.</sub>

This ticket was mentioned in ​Slack in #core by adrianduffell. ​View the logs.


2 months ago

#43 @adrianduffell
2 months ago

  • Keywords early added
  • Milestone 7.1 → Future Release

This was discussed in today's bug scrub. I’d like to punt it with 7.1 RC 1 due in the upcoming hours.

It looks like it is testing well, so tagging with early for consideration in 7.2.

#44 @parthjoginyusoft
7 weeks ago

Tested PR #11534 locally.

Environment:

  • WordPress Core local Docker development environment
  • Single-site installation
  • PHPUnit 9.6.36

Results:

  • Tests_Term_Query: 44 tests, 109 assertions — PASS
  • test_object_ids_null_returns_all_terms: 1 test, 3 assertions — PASS
  • test_object_ids_zero_is_treated_as_numeric: 1 test, 1 assertion — PASS

I also verified the changes in WP_Term_Query. The change correctly treats null and an empty object_ids value as unset, while numeric values continue through array_map().

No test failures were observed.

Note: See TracTickets for help on using tickets.