Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#31086 closed defect (bug) (fixed)

Uniform results for getting terms

Reported by: joshlevinson's profile joshlevinson Owned by: boonebgorges's profile 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:

  1. Update get_object_term_cache so it always returns a zero-indexed array
  2. 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)

taxonomy.php.patch (384 bytes) - added by joshlevinson 10 years ago.
31086.patch (1.0 KB) - added by joshlevinson 10 years ago.
31086.tests.patch (717 bytes) - added by joshlevinson 10 years ago.
Unit tests for 31086.patch

Download all attachments as: .zip

Change History (11)

#1 @boonebgorges
10 years ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.2

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 of wp_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 changing update_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 of get_the_terms() are zero-indexed for both cached and uncached terms.

#2 @joshlevinson
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 @joshlevinson
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.

@joshlevinson
10 years ago

@joshlevinson
10 years ago

Unit tests for 31086.patch

#4 @joshlevinson
10 years ago

  • Keywords has-patch added

#5 @boonebgorges
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 @boonebgorges
10 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 31287:

Don't use term IDs for array indexes when caching object terms.

Uncached results pulled from wp_get_object_terms() are zero-indexed (ie 0,
1, 2...). As a result, get_the_terms() was returning a strictly different
array when pulling from the cache and when the cache was empty.

Props joshlevinson.
Fixes #31086.

#7 @joshlevinson
10 years ago

Thanks for polishing this off! Gives me a better direction for writing future unit tests :-)

#8 @boonebgorges
10 years ago

Thanks for your contribution!

Note: See TracTickets for help on using tickets.