Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#36814 closed defect (bug) (fixed)

get_the_terms() caches term count property

Reported by: ocean90's profile ocean90 Owned by: boonebgorges's profile boonebgorges
Milestone: 4.6 Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: has-patch needs-testing
Focuses: Cc:

Description

Noticed this on w.org see attached screenshot.

Call stack is get_the_tags( $id, 'post_tag' ); -> get_the_terms( $id, 'post_tag' ).

get_the_terms() caches the assigned terms which includes the count property too. But it seems like the cache gets only refreshed on post updates.

Attachments (2)

36814.png (342.9 KB) - added by ocean90 8 years ago.
36814.diff (8.4 KB) - added by boonebgorges 8 years ago.

Download all attachments as: .zip

Change History (11)

@ocean90
8 years ago

#1 @ocean90
8 years ago

  • Owner set to boonebgorges
  • Status changed from new to reviewing

#2 @boonebgorges
8 years ago

  • Keywords needs-unit-tests added

I was hopeful that we'd be able to solve this by switching to ID-only queries in #35381, but I've been working on that ticket, and it looks like it won't be possible to make that change at this time. So the bug described here will have to be addressed separately.

Two possible strategies:

  1. Stop caching count. There's some prior art here - in wp_get_object_terms(), term cache is handled in such a way that the object_id property is uncached.
  2. Invalidate term cache when a post is updated that has a term_relationship with that term.

I think the second technique is going to be a bit easier. Let me see what I can do about a patch.

#3 @boonebgorges
8 years ago

In 37480:

Move get_the_terms() tests to their own file.

See #36814.

@boonebgorges
8 years ago

#4 @boonebgorges
8 years ago

  • Keywords has-patch needs-testing added; needs-patch needs-unit-tests removed

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

#5 @boonebgorges
8 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 37573:

Store only term IDs in object term relationships caches.

Previously, objects containing all data about a term were stored in each
object's term cache. Besides being wasteful, this approach caused invalidation
issues, as when a modified term count required a flush for all objects
belonging to the term.

Backward compatibility is maintained for plugins that continue to put object
data directly into the {$taxonomy}_relationships cache bucket.

Fixes #36814.

#6 @DrewAPicture
8 years ago

In 37578:

Docs: Remove unnecessary backtick-escaping around two function references in the DocBlock for get_object_term_cache().

Known classes, methods, and functions are now auto-linked and formatted in the Code Reference. :-)

See [37573]. See #36814.

#7 @boonebgorges
8 years ago

In 37593:

Don't clear object relationship caches on term update.

Since [37573], object relationship caches ({$taxonomy}_relationships)
contain term IDs rather than term objects. See #36814. As such, it's no longer
necessary to clear these caches when a term is updated; none of the data that's
changed on update (name, description, count, etc) is stored in the relationship
cache.

Fixes #36251.

#8 @boonebgorges
8 years ago

In 38277:

In is_object_in_term(), return error object rather than caching it.

This change prevents an error object from being stored in the cache,
and prevents notices from being thrown when plucking term IDs to put
into the relationship cache.

See #32044, #36814.

Props rpayne7264.
Fixes #37721.

#9 @boonebgorges
8 years ago

In 38346:

In is_object_in_term(), return error object rather than caching it.

This change prevents an error object from being stored in the cache,
and prevents notices from being thrown when plucking term IDs to put
into the relationship cache.

See #32044, #36814.

Merges [38277] to the 4.6 branch.

Props rpayne7264.
Fixes #37721.

Note: See TracTickets for help on using tickets.