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

[dilipbheda]

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

[audrasjb]

Makes sense, thanks.

[rollybueno]

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.

[staurand]

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.

[westonruter]

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

[staurand]

@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 :)

[westonruter]

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

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

[staurand]

@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'] );

[westonruter]

@staurand Good idea.

@dilipbheda Could you apply this feedback?

[westonruter]

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.

[wildworks]

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

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

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.

[westonruter]

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

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

[staurand]

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.