[wp-trac] [WordPress Trac] #36814: get_the_terms() caches term count property

[WordPress Trac]

#36814: get_the_terms() caches term count property
-------------------------------------+---------------------------
 Reporter:  ocean90                  |       Owner:  boonebgorges
     Type:  defect (bug)             |      Status:  reviewing
 Priority:  normal                   |   Milestone:  4.6
Component:  Taxonomy                 |     Version:
 Severity:  normal                   |  Resolution:
 Keywords:  has-patch needs-testing  |     Focuses:
-------------------------------------+---------------------------
Changes (by boonebgorges):

 * keywords:  needs-patch needs-unit-tests => has-patch needs-testing


Comment:

 > Invalidate term cache when a post is updated that has a
 term_relationship with that term.

 On further review, more invalidation does *not* appear to be the right
 answer. Here's the problem. Term data (classes with properties `term_id`,
 `taxonomy`, `count`, etc) are currently cached in *two* places:

 1. `wp_cache_set( $term_id, 'terms' )` (the single-term cache)
 2. `wp_cache_set( $object_id, $terms, "{$taxonomy}_relationships" )` (the
 object-term-relationships cache - `$terms` is an array of term objects)

 When a term's `count` changes, cache 1 is invalidated correctly. Cache 2
 is not, which is what causes the current bug. But to invalidate it
 properly would mean busting the object relationship cache for *every
 single object* associated with the term. In other words: A category `foo`
 is associated with 100 posts (`count=100`). It's added to a new post.
 Fixing the current bug by invalidating cache 2 means that *all 100 posts*
 must have their relationship caches dumped. This is a lousy system.

 There is a better way! The `{$taxonomy}_relationships` cache should be an
 array of term IDs, rather than an array of term objects. Term objects can
 then be pulled from the single-term cache. See [attachment:36814.diff].

 A couple back compat concerns. First, to maintain the same return value
 for `get_object_term_cache()` - an array of term objects - I had to move a
 bunch of logic into there. In some rare cases, this may mean that calling
 `get_object_term_cache()` results in a database query, in order to prime
 the cache for the individual terms. It's very rare, because the query only
 takes places when the `{$taxonomy}_relationships` cache is primed, but one
 or more of the matching single-term caches is not. But this hardly ever
 happens, because `get_object_term_cache()` is usually called in
 conjunction with `wp_get_object_terms()` or another function that sets all
 of these caches. I don't think this is actually a concern, but it's worth
 mentioning for the record.

 Second, the change means that the value stored in
 `{$taxonomy}_relationships` is of a different format. Anyone directly
 reading that value and expecting it to be term objects will have their
 stuff break. I've done a search of the plugin repo and have found a very,
 very small number of cases that would ever be affected by this. To be
 extra safe, I've maintained support for plugins that *update* the
 `{$taxonomy}_relationships` cache with term objects - this happens a bit
 more frequently in the wild.

 Anyone see problems with the approach I'm suggesting? I think it's far
 more elegant than what we've currently got. (And it will make the cache
 schema more compatible with a split term query, if we ever get around to
 that; see #35381.)

--
Ticket URL: <https://core.trac.wordpress.org/ticket/36814#comment:4>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform