Opened 7 years ago
Closed 6 years 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: |
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)
Change History (22)
#2
in reply to:
↑ 1
@
7 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.
#4
@
7 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
@
7 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.
#6
@
7 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
@
7 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.
#10
@
7 years 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:
↓ 12
@
7 years ago
What's the benefit of WP_Term_Query::populate_terms()
over array_filter( array_map( 'get_term', $terms ) )
?
#12
in reply to:
↑ 11
@
7 years ago
Replying to swissspidy:
What's the benefit of
WP_Term_Query::populate_terms()
overarray_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
@
7 years 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.
#15
@
7 years ago
- Keywords fixed-major added
- Milestone changed from 4.9.8 to 4.9.7
Moving back to 4.9.7 for backport consideration.
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 inWP_Term_Query
, the term IDs pulled from the database or from the cache should always correspond to an existing term (and thusget_term()
should *not* returnnull
). 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.