Opened 6 months ago

Closed 6 months ago

#22560 closed defect (bug) (fixed)

Cache invalidation in wp_set_object_terms

Reported by: batmoo Owned by: ryan
Priority: normal Milestone: 3.5
Component: Taxonomy Version: 3.5
Severity: normal Keywords: has-patch commit dev-reviewed
Cc: batmoo

Description

In [21981], cache invalidation was added to wp_set_post_terms. Should this not actually go in wp_set_object_terms instead?

The current cache invalidation routine means that a call to wp_set_object_terms and then a subsequent call to get_the_terms will result in stale cache entry returned.

Attachments (2)

22560.diff (851 bytes) - added by ryan 6 months ago.
22560.2.diff (1.0 KB) - added by ryan 6 months ago.

Download all attachments as: .zip

Change History (16)

Previous discussion that led to the change: http://core.trac.wordpress.org/ticket/21309#comment:72

  • Version set to trunk

The $taxonomy . '_relationships' cache is set in get_the_terms(), which only works for posts.

So, the cache is cleared in wp_set_post_terms(), which only works for posts.

comment:4 follow-up: ↓ 9   nacin6 months ago

  • Keywords close added

I could see why this can be an issue. Many developers use wp_set_object_terms() for posts, probably not realizing the higher level function exists. I don't think there is any harm in clearing the cache at a lower point in the stack.

Correct me if I'm wrong, but this also isn't a regression? We didn't add caching, just invalidation. Or does the caching apply in new places that could cause issues?

  • Keywords close removed
  • Milestone Awaiting Review deleted
  • Status changed from new to closed
  • Milestone set to Awaiting Review
  • Status changed from closed to reopened
  • Owner set to ryan
  • Status changed from reopened to assigned

comment:9 in reply to: ↑ 4   batmoo6 months ago

Replying to nacin:

Correct me if I'm wrong, but this also isn't a regression? We didn't add caching, just invalidation. Or does the caching apply in new places that could cause issues?

Not a regression but currently misleading since the expectation is that it's perfectly okay to use wp_set_object_terms in place of wp_set_post_terms and get the same outcome.

  • Keywords dev-feedback added

I don't think there is any harm in clearing the cache at a lower point in the stack.

I'll re-iterate my earlier statement.

  • Milestone changed from Awaiting Review to 3.5

ryan6 months ago

ryan6 months ago

  • Keywords has-patch commit dev-reviewed added; dev-feedback removed

If a taxonomy was assigned to completely different objects — say, a post and a user — and the same ID in both had a relationship cache, one would clear the other. This is acceptable, especially because that combination doesn't work anyway, as term relationships don't have a type of object — just an object ID and a tt ID.

Also, there is one early return in wp_set_object_terms() for db errors. We weren't doing a cache delete in wp_set_post_terms() for WP_Error either, so this also seems good.

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

In 22878:

Clear the object term relationships cache in wp_set_object_terms() rather than wp_set_post_terms(). This should be done lower in the stack than wp_set_post_terms().

Props batmoo
fixes #22560

Note: See TracTickets for help on using tickets.