WP_Term_Query get_terms generates invalid sql queries

[GM_Alex]

get_terms at WP_Term_Query uses $terms = array_map( 'get_term', $terms ); but get_term can return null by definition and so we creating an array like the following one

array(
  0 => null,
  1 => null
)

As result get_sql_for_clause at WP_Tax_Query generates the invalid query, calling $this->clean_query() > $this->transform_query() > WP_Term_Query::query() and then $terms = implode( ',', $terms ) at get_sql_for_clause.

[boonebgorges]

Hi @GM_Alex - Thanks for the ticket and for the patch.

Can you walk through the steps needed to reproduce this? It's true that get_term() can return a null value, but in WP_Term_Query, the term IDs pulled from the database or from the cache should always correspond to an existing term (and thus get_term() should *not* return null). I assume that you are working with a corrupted cache, or a plugin is filtering the term functions in an inconsistent way. This doesn't mean it's not something we should fix, but I want to understand the conditions for the error before throwing a band-aid over it.

[GM_Alex]

Hi @boonebgorges,

you are right, this issue is the result of filtering the terms. I'm the author of the User Access Manager (​https://wordpress.org/plugins/user-access-manager/) and I filter terms with no access. If they are hidden completely I return null, but within the plugin I also filtering the queries. The issue exists only in combination with an other plugin, Ultimate Category Excluder (​https://wordpress.org/plugins/ultimate-category-excluder/). This plugin uses the pre_get_posts filter and sets category__not_in, while the User Access Manager returns null for the get_term function if the category is restricted. And that results in a broken query (see ​https://github.com/GM-Alex/user-access-manager/issues/185). But anyway since null is a valid return type and also the cache could be corrupted, we should filter the array for null values to prevent the generation of invalid queries.

[markjaquith]

Probably related to #42771

[boonebgorges]

@GM_Alex If possible, could you have a look at #42771 to see if it was the cause of this regression for you? If so, we can probably fold these tickets together.

[GM_Alex]

@boonebgorges Reverting the changes of https://core.trac.wordpress.org/changeset/40979 doesn't fixes the issue here, so it seems not related for me.

[boonebgorges]

Thanks very much for verifying, @GM_Alex. 42691.2.diff​ brings your original patch a bit closer in line with naming conventions for WP, and also adds some tests. Can you give it another look please?

(For the record, while it's perhaps true that the bad SQL in transform_query() dates from 4.9, the get_term() mapping has happened for a lot longer, so this bug could have - and probably did - arise in other contexts before 4.9.)

[GM_Alex]

@boonebgorges Your patch works, but since $term_ids can be false we should add an extra check, like

protected function populate_terms( $term_ids ) {
    if ( is_array($term_ids) === false ) {
        return array();
    }

    $terms = array();

    foreach ( $term_ids as $key => $term_id ) {
        $term = get_term( $term_id );

        if ( $term instanceof WP_Term ) {
            $terms[ $key ] = $term;
        }
    }

    return $terms;
}

Otherwise we get a warning.

Edit: We have to return an empty array otherwise more warnings will be shown.

[lllor]

by any chance, is it possible to include the fix before version 5.0?

Cheers

[boonebgorges]

In 43049:

Taxonomy: Ensure that invalid term objects are discarded in WP_Term_Query.

The get_term() mapping may result in term objects that are null or
WP_Error when plugins use get_term or a related filter. Since null
and error objects are not valid results for a term query, we discard
them.

Props GM_Alex.
See #42691.

[boonebgorges]

I'm leaving this ticket open for backporting after the 4.9.6 release.

[swissspidy]

What's the benefit of WP_Term_Query::populate_terms() over array_filter( array_map( 'get_term', $terms ) )?

[boonebgorges]

Replying to swissspidy:

What's the benefit of WP_Term_Query::populate_terms() over array_filter( array_map( 'get_term', $terms ) )?

That won't work if $terms is not an array. But yes, array_filter() could be used to shorten:

if ( is_array( $terms ) {
  $terms = array_filter( array_map( 'get_term', $terms ) );
} else {
  $terms = array();
}

The separate method is to avoid duplicating the logic, but if there's concern about adding the method, it's not hugely problematic to repeat it.

[boonebgorges]

Sorry, I was mistaken in my previous comment. The populate_terms() check also accounts for the fact that get_term() can return WP_Error objects. This can't easily be compressed into a array_filter() call, unless we have a function that checks for non-emptiness as well as non-WP_Error-ness.

[desrosj]

Moving all tickets in 4.9.7 to 4.9.8.

[desrosj]

Moving back to 4.9.7 for backport consideration.

[boonebgorges]

#44428 was marked as a duplicate.

[ocean90]

4.9.7 has been released, moving to next milestone.

[pbiron]

Any chance this can be backported for 4.9.8?

[SergeyBiryukov]

In 43491:

Docs: Change @since entry for WP_Term_Query::populate_terms() added in [43049] to 4.9.8.

See #42691.

[SergeyBiryukov]

In 43492:

Taxonomy: Ensure that invalid term objects are discarded in WP_Term_Query.

The get_term() mapping may result in term objects that are null or
WP_Error when plugins use get_term or a related filter. Since null
and error objects are not valid results for a term query, we discard
them.

Props GM_Alex.
Merges [43049] and [43491] to the 4.9 branch.
Fixes #42691.