WP_Term_Query::get_terms() - check return value of get_term()

[josephscott]

Deep inside of WP_Term_Query::get_terms() there is a get_term() call that assumes it will always get a WP_Term object back. Even though get_term() is documented - ​https://developer.wordpress.org/reference/functions/get_term/ - as being able to return WP_Term|array|WP_Error|null. I'm running into cases where it returns null, generating a warning like:

Attempt to read property "count" on null

Because it always tries to look at $child->count, without ever checking the return value - ​https://github.com/WordPress/wordpress-develop/blame/trunk/src/wp-includes/class-wp-term-query.php#L851-L854

We can avoid that warning, and keep the existing behavior, with an additional check. WP_Term_Query::populate_terms() for example uses instanceof, which could also be done here - ​https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/class-wp-term-query.php#L1140-L1142

Here is a simplified test of this condition - ​https://3v4l.org/E4uk7 - that confirms the same result for the if condition and that the warning no longer happens.

https://core.trac.wordpress.org/ticket/63877

Need to check the return value from the call to get_term().

[sabernhardt]

Related/duplicate: #57835

While tests are having trouble passing now in trunk, it would be nice to add a test for the case when something other than WP_Term is returned.

I thought about tests before submitting this, but wasn't sure what form a test would take. For example, I could generate a test that fails when get_term() returns null - but that test would then always fail. What would success look like in that scenario?

The inverse also seems not very helpful, if we test that we got null back in a way we expected, then it would always pass. Something like:

class Tests_Term_Null_Term_Handling extends WP_UnitTestCase {
    public function test_orphaned_hierarchy_causes_null_term_access() {
        global $wpdb;
        register_taxonomy( 'wptests_tax', 'post', array( 'hierarchical' => true ) );    
        $parent_term = self::factory()->term->create( array( 'taxonomy' => 'wptests_tax' ) );
        $non_existent_child_id = 99999;
        $hierarchy = array( $parent_term => array( $non_existent_child_id ) );
        update_option( 'wptests_tax_children', $hierarchy ); 
        $this->assertNull( get_term( $non_existent_child_id, 'wptests_tax' ), 'Non-existent term should return null' );
    }
}

If you are ok with a test like that, I'm happy to clean it up ( and add comments ) and add that to the patch.

Yeah, I mean whatever scenario caused the error to happen in the first place on your site would be good to add a test for.

I honestly don't know what happened, I noticed it on sites that have been running for more than 15 years and have probably been generating this warning for years.

[SergeyBiryukov]

#57835 was marked as a duplicate.

[SergeyBiryukov]

In 60661:

Taxonomy: Check the result of get_term() in WP_Term_Query::get_terms().

get_term() can return WP_Error or null on failure, so the result should be verified as a WP_Term instance before accessing the count property.

This commit prevents a PHP warning if get_term() returns null for a child term:

Warning: Attempt to read property "count" on null

Follow-up to [27458], [37572].

Props josephscott, coleatkinson1, kebbet, jakariaistauk, sabernhardt, westonruter, SergeyBiryukov.
Fixes #63877.