The get_terms() argument "fields" renders "get_terms_fields" & "terms_clauses" filter callbacks useless

[F J Kaiser]

The get_terms( $tax, [ args ] ); function accepts an argument named fields. This allows to chose which columns should be fetched from either the wp_terms or wp_term_taxonomy table. Example:

$terms = get_terms( 'example_tax', [ 'fields' => 'names', ] );
var_dump( $names );
/* Result:
array (size=1)
  0 => string 'Code Review' (length=11)
*/

In this case, the resulting query inside the function would be the following:

SELECT t.term_id, tt.parent, tt.count, t.name 
FROM wp_terms AS t 
INNER JOIN wp_term_taxonomy AS tt 
    ON t.term_id = tt.term_id 
WHERE tt.taxonomy IN ('project_type') 
ORDER BY t.name ASC

In fact, fields does not really any combination of fields, but allows to chose from a list:

• all : $selects = array( 't.*', 'tt.*' )
• ids : $selects = array( 't.term_id', 'tt.parent', 'tt.count' )
• id=>parent : $selects = array( 't.term_id', 'tt.parent', 'tt.count' ) (the same as above)
• names : $selects = array( 't.term_id', 'tt.parent', 'tt.count', 't.name' )
• count : $selects = array( 'COUNT(*)' )
• id=>name : $selects = array( 't.term_id', 't.name', 'tt.count' )
• id=>slug : $selects = array( 't.term_id', 't.slug', 'tt.count' )

After this preselection ran, there are two filters available to alter the list:

$fields = implode( ', ', apply_filters( 'get_terms_fields', $selects, $args, $taxonomies ) );
$pieces = array( 'fields', 'join', 'where', 'orderby', 'order', 'limits' );

But no matter if you attached an filter to either of those or not: The result stays the same. This is due to the following filtering done in a final loop through all results:

• ids : $_terms[] = $term->term_id
• id=>parent : $_terms[$term->term_id] = $term->parent
• names : $_terms[] = $term->name
• id=>name : $_terms[$term->term_id] = $term->name
• id=>slug : $_terms[$term->term_id] = $term->slug

And that renders callbacks to both attached filters useless. The only cases where this does not happen are the argument values all and count.

I'm not sure if this should be called an "undocumented feature" or if this actually needs a patch. I'm in for later if there is support for changing this awkward behavior as this is not clear until one jumps and dumps in core.

Proposed fix: Only run the final filter-loop if ...

if ( ! ( 
    has_filter( 'get_terms_fields' ) or has_filter( 'terms_clauses' ) 
) )
    // do final filtering

[boonebgorges]

F J Kaiser - Thanks for the very detailed and helpful bug report.

The filter was introduced in [11037], and I'm not sure it's ever worked properly. See https://core.trac.wordpress.org/browser/trunk/wp-includes/taxonomy.php?annotate=blame&rev=11037#L773. Even then, passing 'ids' or 'names' for the value of 'fields' would override any changes applied in a 'get_terms_fields' filter, though back then there were fewer supported values of 'fields'.

I'm leaning toward wontfix here. IMO this is a pretty crummy filter anyway - it seems much better to have a more robust 'fields' parameter. Moreover, I think the 'fields' param - aside from 'names' and 'ids', which return flat arrays - are not very useful or helpful. If 'fields=all' gets you more data than you need, just ignore the excess. I guess people want to specify the fields in order to reduce the memory footprint of the query. But the savings are really marginal; and when you don't do 'fields=all', you don't get full cache support, so you're potentially shooting yourself in the foot anyway. If I could rip this filter out without breaking things, I would :) As it is, it's working fine for 'all' and 'count' (as you note), so I think we should just leave it as it is.

[F J Kaiser]

​As mentioned and ​as @boonebgorges has agreed, we need a "docblock update only" patch that explains the problem with those filters. Else later devs are forced to go down the same rabbit hole.

[boonebgorges]

In 31174.diff​, I remove the bit about the filter from the function docblock, and I add some additional info to the hook documentation explaining the issue (and discouraging use of the hook). @F J Kaiser, what do you think?

[F J Kaiser]

Replying to boonebgorges:

In 31174.diff​, I remove the bit about the filter from the function docblock, and I add some additional info to the hook documentation explaining the issue (and discouraging use of the hook). @F J Kaiser, what do you think?

Looks good for me. Thanks for the patch and your effort.

[DrewAPicture]

In 31855:

Supplement hook documentation for the get_terms_fields filter to more clearly explain the expected consequences of using it to modify the fields to select in a terms query.

Props boonebgorges.
Fixes #31174.