Make WordPress Core

Opened 11 years ago

Closed 11 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's profile technical_mastermind Owned by: boonebgorges's profile boonebgorges
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.1
Component: Taxonomy Keywords: has-unit-tests has-patch
Focuses: Cc:

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 11 years ago.
Bug patch
29859.unit-tests.diff (6.9 KB) - added by technical_mastermind 11 years ago.
Unit tests that I added.
29859.patch (25.6 KB) - added by boonebgorges 11 years ago.
29859.2.patch (13.3 KB) - added by boonebgorges 11 years ago.

Download all attachments as: .zip

Change History (15)

@technical_mastermind
11 years ago

Bug patch

@technical_mastermind
11 years ago

Unit tests that I added.

#1 @technical_mastermind
11 years ago

  • Keywords has-unit-tests has-patch added

#2 @markjaquith
11 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
11 years ago

  • Status changed from assigned to accepted

#4 @boonebgorges
11 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
11 years ago

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

  • Keywords reporter-feedback removed

#10 in reply to: ↑ 8 @boonebgorges
11 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
11 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.