WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#31174 closed defect (bug) (fixed)

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

Reported by: F J Kaiser Owned by: DrewAPicture
Milestone: 4.2 Priority: normal
Severity: normal Version: 4.1
Component: Taxonomy Keywords: has-patch needs-codex
Focuses: docs Cc:

Description

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

Attachments (1)

31174.diff (1.1 KB) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (10)

#1 @boonebgorges
6 years ago

  • Keywords close added

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.

This ticket was mentioned in Slack in #core by kaiser. View the logs.


5 years ago

#3 @F J Kaiser
5 years ago

  • Keywords needs-patch added; close removed

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.

Last edited 5 years ago by F J Kaiser (previous) (diff)

#4 @DrewAPicture
5 years ago

  • Focuses docs added

@boonebgorges
5 years ago

#5 follow-up: @boonebgorges
5 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 4.2

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?

This ticket was mentioned in Slack in #core by drew. View the logs.


5 years ago

#7 @DrewAPicture
5 years ago

  • Owner set to DrewAPicture
  • Status changed from new to reviewing

#8 in reply to: ↑ 5 @F J Kaiser
5 years ago

  • Keywords needs-codex added

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.

#9 @DrewAPicture
5 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

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.

Note: See TracTickets for help on using tickets.