WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#29859 closed defect (bug) (fixed)

get_terms on hierarchical taxonomy fails when 'fields' arg contains 'id=>name' OR 'id=>slug'

Reported by: technical_mastermind Owned by: boonebgorges
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.1
Component: Taxonomy Keywords: has-unit-tests has-patch
Focuses: Cc:
PR Number:

Description

When using get_terms() and passing in 'id=>name' or 'id=>slug' as the value for the 'fields' argument, it fails to return the expected array if you are working with a hierarchical taxonomy.

For example, this code block works:

$terms = get_terms( 'post_tag', array( 'fields' => 'id=>name' ) );

This code block does not work:

$terms = get_terms( 'category', array( 'fields' => 'id=>name' ) );

The difference being that 'post_tag' is a non-hierarchical taxonomy while 'category' is a hierarchical taxonomy. I have tested this in several versions of WordPress, it is still present in 4.0. I have created a patch for it against trunk as well as written more in-depth unit tests to test the functionality of the get_terms function. Note that 4 of the unit tests I wrote fail before the get_terms patch is applied, none of the unit tests I wrote fail after the patch is applied.

Attachments (4)

29859.diff (550 bytes) - added by technical_mastermind 5 years ago.
Bug patch
29859.unit-tests.diff (6.9 KB) - added by technical_mastermind 5 years ago.
Unit tests that I added.
29859.patch (25.6 KB) - added by boonebgorges 5 years ago.
29859.2.patch (13.3 KB) - added by boonebgorges 5 years ago.

Download all attachments as: .zip

Change History (15)

@technical_mastermind
5 years ago

Bug patch

@technical_mastermind
5 years ago

Unit tests that I added.

#1 @technical_mastermind
5 years ago

  • Keywords has-unit-tests has-patch added

#2 @markjaquith
5 years ago

  • Owner set to boonebgorges
  • Status changed from new to assigned

Looks like a simple fix and has unit tests. @boonebgorges, you wanna take this one?

#3 @boonebgorges
5 years ago

  • Status changed from assigned to accepted

#4 @boonebgorges
5 years ago

  • Milestone changed from Awaiting Review to 4.1

Fix looks good at a glance - thanks, technical_mastermind. I'll review the patch in more detail in the upcoming days.

@boonebgorges
5 years ago

#5 follow-up: @boonebgorges
5 years ago

  • Keywords reporter-feedback added

29859.patch breaks up technical_mastermind's tests so that only one assertion happens per test. (The duplicated setup code required by the split tests is a bit annoying, but it's important to ensure that the assertions are actually independent of one another. Separating them out also means that one test can fail while the rest continue to run.)

Before going ahead with this fix: technical_mastermind, in your original post you mentioned that *4* of the tests fail without the fix to get_terms(). I'm only seeing two failures: test_get_terms_hierarchical_tax_hide_empty_true_fields_idslug() and test_get_terms_hierarchical_tax_hide_empty_true_fields_idname(). Your diagnosis of the problem - that the 'tt.count' column wasn't being selected when 'hierarchical=true' on hierarchical taxonomies - suggests that there should only be failures when 'hierarchical=true' *and* 'hide_empty=true', since when 'hide_empty=false' the count value is never used. So, 2 seems like the correct number of failing tests. Can you clarify where your 4 is coming from?

#6 in reply to: ↑ 5 ; follow-up: @SergeyBiryukov
5 years ago

Replying to boonebgorges:

The duplicated setup code required by the split tests is a bit annoying

Can we move it into a separate private function?

#7 in reply to: ↑ 6 @boonebgorges
5 years ago

Replying to SergeyBiryukov:

Replying to boonebgorges:

The duplicated setup code required by the split tests is a bit annoying

Can we move it into a separate private function?

Yes. 29859.2.patch

#8 follow-up: @technical_mastermind
5 years ago

Just looked at this again boonebgorges, you are correct that only 2 fail. That was an error on my part, I incorrectly stated the number of tests that fail. Sorry about the confusion. That's what I get for learning PHPUnit the same night I submit a patch.

#9 @technical_mastermind
5 years ago

  • Keywords reporter-feedback removed

#10 in reply to: ↑ 8 @boonebgorges
5 years ago

Replying to technical_mastermind:

Just looked at this again boonebgorges, you are correct that only 2 fail. That was an error on my part, I incorrectly stated the number of tests that fail. Sorry about the confusion. That's what I get for learning PHPUnit the same night I submit a patch.

Great - thanks for verifying. And really, thank you so much for these unit tests - I appreciate them more than the fix itself :)

#11 @boonebgorges
5 years ago

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

In 29845:

In get_terms(), select term taxonomy count for all values of 'fields'.

Not having the count caused queries with 'fields' values of 'id=>name' and
'id=>slug' to return incorrect results when querying a hierarchical taxonomy
with 'hide_empty=true'.

Includes unit tests for get_terms() when using various combinations of 'fields',
'hide_empty', and 'hierarchical' arguments.

Props technical_mastermind.
Fixes #29859.

Note: See TracTickets for help on using tickets.