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)
Change History (46)
This ticket was mentioned in PR #8668 on WordPress/wordpress-develop by @dilipbheda.
18 months ago
#1
#3
@
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
@
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
@
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
@
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
@
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.
@westonruter commented on PR #8668:
12 months ago
#9
Let's also include a unit test for this change.
#10
@
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'] );
#12
@
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.
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
@
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.
- Add a unit test: https://github.com/WordPress/wordpress-develop/pull/8668#issuecomment-3448722852
- Changes based on feedback: https://core.trac.wordpress.org/ticket/63256#comment:10
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_idsisnull(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
0might 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
@
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
@
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
@
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
This ticket was mentioned in PR #11534 on WordPress/wordpress-develop by @nimeshatxecurify.
6 months ago
#25
This ticket was mentioned in Slack in #core-test by r1k0. View the logs.
5 months ago
#27
@
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
- Add the following snippet to your active theme's
functions.phpor 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 — 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>';
} );
- Observe object_ids
NULLdoes not skip and results asarray_map → array ( ) - Apply the patch
NULLnow correctly skipsarray_map(). All other values behave identically to unpatched version- ✅ Patch is solving the problem
Expected result
- When
object_idsis not set (defaults tonull),array_map()should be skipped. - All other
object_idsvalues ('',0,false, valid IDs) should behave unchanged.
Screenshots/Screencast with results
This ticket was mentioned in Slack in #core by nimeshatxecurify. View the logs.
4 months ago
#29
@
4 months ago
Refreshed the patch on PR #11534. Please review and let me know if any further adjustments are required.
#30
@
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
@
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
@
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
@
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
- Edit theme functions.php
- 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 — 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>';
} );
- Observe that when
object_idsisNULL, it does not skip thearray_map()call and instead results inarray_map() → array().
- Apply the patch.
- Verify that
NULLnow correctly skips thearray_map()call, while all otherobject_idsvalues 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
@
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:
- 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 unnecessaryarray_map('intval')call whenobject_idsis not set (defaulting tonull). - Confirmed that numeric
0continues to be treated as a numeric value rather than unset.
- Pulled PR #11534 (
- 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_termsandtest_object_ids_zero_is_treated_as_numericpassed cleanly.
- Executed PHPUnit test suite:
- Coding Standards:
- Ran
vendor/bin/phpcsonsrc/wp-includes/class-wp-term-query.phpandtests/phpunit/tests/term/query.php. - Result: 0 errors and 0 warnings.
- Ran
Verdict: ✅ Patch tested & verified. Fix functions as expected without regressions.
#38
@
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 —
0does not become "no filter" under either patch.(array) 0isarray( 0 ), which is non-empty, so thetr.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()andtest_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
0edge case.0is not caught by the newnull/''guard (both use strict===), so it still falls to theelsebranch and becomesarray( 0 )→tr.object_id IN (0).object_ids => 0remains a real filter that returns no terms, not "no filter." No breaking change. - comment:17/18 —
false. Same path as0; normalizes toarray( 0 ), behaviour preserved.
---
## Unit tests (in the PR)
Both added tests match the verified behaviour:
test_object_ids_null_returns_all_terms()—object_ids => nullreturns all terms (no filtering). ✔test_object_ids_zero_is_treated_as_numeric()—object_ids => 0returns 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_QueryOK (44, 109),Tests_TermOK (630, 1628), cache-key tests green.
---
## Observations / notes for the committer
- 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 leavesnullasnull, which diverges from trunk in cache-key generation (an extra query whennullthen''are queried under a sharedcache_domain). PR 11534 keeps the stored value identical to trunk for every input, so no cache key changes — the safer of the two implementations.
term_orderfallback 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(), soobject_ids => 0downgradesterm_ordertoterm_ideven 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.
- Open design question (comment:20). Whether
nullandfalse*should* behave differently as invalid values forint|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
@
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
@
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.
![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png)


Makes sense, thanks.