Uniform results for getting terms

[joshlevinson]

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.

[boonebgorges]

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.

[joshlevinson]

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.

[joshlevinson]

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]

Unit tests for 31086.patch

[boonebgorges]

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).

[boonebgorges]

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.

[joshlevinson]

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

[boonebgorges]

Thanks for your contribution!