WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 17 months ago

#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:
PR Number:

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 2 years ago.
42691.2.diff (3.3 KB) - added by boonebgorges 2 years ago.

Download all attachments as: .zip

Change History (22)

@GM_Alex
2 years ago

#1 follow-up: @boonebgorges
2 years 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
2 years 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
2 years ago

Probably related to #42771

#4 @boonebgorges
2 years 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
2 years 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.

@boonebgorges
2 years ago

#6 @boonebgorges
2 years 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
2 years 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 2 years ago by GM_Alex (previous) (diff)

#8 @lllor
20 months ago

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

Cheers

#9 @boonebgorges
20 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
20 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
20 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
20 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
20 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
19 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
18 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
18 months ago

#44428 was marked as a duplicate.

#17 @ocean90
17 months ago

  • Milestone changed from 4.9.7 to 4.9.8

4.9.7 has been released, moving to next milestone.

#18 @pbiron
17 months ago

Any chance this can be backported for 4.9.8?

#19 @SergeyBiryukov
17 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
17 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.