WordPress.org

Make WordPress Core

#42691 closed defect (bug) (fixed)

WP_Term_Query get_terms generates invalid sql queries

Reported by: GM_Alex Owned by: SergeyBiryukov
Milestone: 4.9.8 Priority: normal
Severity: normal Version: 4.9
Component: Taxonomy Keywords: has-patch fixed-major
Focuses: Cc:

Description

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.

Attachments (2)

42691.diff (1.3 KB) - added by GM_Alex 22 months ago.
42691.2.diff (3.3 KB) - added by boonebgorges 22 months ago.

Download all attachments as: .zip

Change History (22)

@GM_Alex
22 months ago

#1 follow-up: @boonebgorges
22 months ago

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.

#2 in reply to: ↑ 1 @GM_Alex
22 months ago

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.

#3 @markjaquith
22 months ago

Probably related to #42771

#4 @boonebgorges
22 months ago

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

#5 @GM_Alex
22 months ago

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

#6 @boonebgorges
22 months ago

  • Component changed from General to Taxonomy
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 5.0

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

#7 @GM_Alex
22 months ago

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

Last edited 22 months ago by GM_Alex (previous) (diff)

#8 @lllor
18 months ago

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

Cheers

#9 @boonebgorges
17 months ago

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.

#10 @boonebgorges
17 months ago

  • Milestone changed from 5.0 to 4.9.7

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

#11 follow-up: @swissspidy
17 months ago

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

#12 in reply to: ↑ 11 @boonebgorges
17 months ago

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.

#13 @boonebgorges
17 months ago

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.

#14 @desrosj
16 months ago

  • Milestone changed from 4.9.7 to 4.9.8

Moving all tickets in 4.9.7 to 4.9.8.

#15 @desrosj
16 months ago

  • Keywords fixed-major added
  • Milestone changed from 4.9.8 to 4.9.7

Moving back to 4.9.7 for backport consideration.

#16 @boonebgorges
15 months ago

#44428 was marked as a duplicate.

#17 @ocean90
15 months ago

  • Milestone changed from 4.9.7 to 4.9.8

4.9.7 has been released, moving to next milestone.

#18 @pbiron
14 months ago

Any chance this can be backported for 4.9.8?

#19 @SergeyBiryukov
14 months ago

In 43491:

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

See #42691.

#20 @SergeyBiryukov
14 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.