Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#22560 closed defect (bug) (fixed)

Cache invalidation in wp_set_object_terms

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

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 11 years ago.
22560.2.diff (1.0 KB) - added by ryan 11 years ago.

Download all attachments as: .zip

Change History (16)

#1 @batmoo
11 years ago

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

#2 @nacin
11 years ago

  • Version set to trunk

#3 @scribu
11 years ago

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.

#4 follow-up: @nacin
11 years 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?

#5 @nacin
11 years ago

  • Keywords close removed

#6 @nacin
11 years ago

  • Milestone Awaiting Review deleted
  • Status changed from new to closed

#7 @nacin
11 years ago

  • Milestone set to Awaiting Review
  • Status changed from closed to reopened

#8 @nacin
11 years ago

  • Owner set to ryan
  • Status changed from reopened to assigned

#9 in reply to: ↑ 4 @batmoo
11 years 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.

#10 @nacin
11 years ago

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

#12 @ryan
11 years ago

  • Milestone changed from Awaiting Review to 3.5

@ryan
11 years ago

@ryan
11 years ago

#13 @nacin
11 years 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.

#14 @ryan
11 years ago

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