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: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (15)
#2
@
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?
#4
@
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.
#5
follow-up:
↓ 6
@
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:
↓ 7
@
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
@
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:
↓ 10
@
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.
#10
in reply to:
↑ 8
@
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 :)
Bug patch