#31086 closed defect (bug) (fixed)
Uniform results for getting terms
Reported by: | joshlevinson | Owned by: | boonebgorges |
---|---|---|---|
Milestone: | 4.2 | Priority: | normal |
Severity: | normal | Version: | 4.2 |
Component: | Taxonomy | Keywords: | dev-feedback has-patch |
Focuses: | Cc: |
Description
Currently, get_the_terms
gets data from two places: from get_object_term_cache
, or from wp_get_object_terms
.
When it does not pull from the cache, the resulting array is numerically zero-indexed.
However, when it does pull from cache (cases where update_post_caches
has been called before the call to get_the_terms
will make this occur), the resulting array uses term ID as the index for each term object.
This makes it so that workarounds are necessary to reliably get the first term from the resulting terms array.
I see two approaches:
- Update
get_object_term_cache
so it always returns a zero-indexed array - Update
wp_get_object_terms
so it always returns a term_id indexed array
The logic that is present in wp_get_object_terms
pretty much explicitly makes it so that a zero-indexed array is always returned. Also, upon altering it to return the term_id indexes, the following unit tests failed:
Tests_Post_Objects::test_get_tags_input_property
Tests_Term_WpGetObjectTerms::test_should_not_filter_out_duplicate_terms_assoc iated_with_different_objects
and sensibly so—they expected to get zero-indexed arrays and instead got term_id indexed arrays.
This makes me believe that a much simpler change is in order—altering get_object_term_cache
so it always returns a zero-indexed array.
I've got a simple patch for this coming; I ran the unit tests that ship with trunk and they all passed.
Attachments (3)
Change History (11)
#1
@
10 years ago
- Keywords needs-patch needs-unit-tests added
- Milestone changed from Awaiting Review to 4.2
#2
@
10 years ago
Glad I could finally get around to contributing (in however small a way)!
I agree with your assertion that the fix should go to the source. My reasoning for not initially doing this was lack of knowledge of whether or not the term_id index is used anywhere else in caching. I'll investigate a bit more.
I'll also try my hand at whipping up a unit test that asserts the two results are always equivalent.
#3
@
10 years ago
- Keywords dev-feedback added; needs-patch needs-unit-tests removed
Finally got around to doing this.
31086.diff
takes care of the indexing where it occurs, as suggested.
In order to ensure the results are truly equal, a modification to get_the_terms
was also required (passing in all_with_object_id
to the fields
parameter, since update_term_cache
does this).
I also wrote a unit test, though it's my first so I'm sure it could use review for both convention and logic.
#5
@
10 years ago
Excellent - thanks for this, joshlevinson.
Regarding the 'all_with_object_id' change: I'm not sure this really qualifies as a bug, but if it does, let's handle it independently.
I'm going to tweak the tests a bit, to make sure that we are testing only the array indexes (ie, the bug that you've raised here).
#6
@
10 years ago
- Owner set to boonebgorges
- Resolution set to fixed
- Status changed from new to closed
In 31287:
joshlevinson - Many thanks for the ticket, and for the detective work. Normally, I'd be a bit nervous about removing meaningful array keys like this, but the fact that
get_the_terms()
is returning inconsistent results for cached vs uncached terms makes me think that no one could be using these indexes in any meaningful way - their debugging would quickly lead them to ditch it in favor ofwp_list_pluck( $terms, 'term_id' )
or whatever.term_id indexing in the taxonomy cache was introduced in [5555], when term caching was first introduced. (It was called
category_id
at the time :) )The
array_values()
fix you've suggested doesn't really go to the heart of the problem. I'd suggest instead changingupdate_object_term_cache()
so that it doesn't index by term_id in this loop https://core.trac.wordpress.org/browser/trunk/src/wp-includes/taxonomy.php#L3737. This won't fix the problem for data that is already cached, but that's a temporary issue. For completeness's sake, it'd be nice to have unit tests that show that results ofget_the_terms()
are zero-indexed for both cached and uncached terms.