WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 16 months ago

#22526 closed defect (bug) (fixed)

Changing category doesn't invalidate cache

Reported by: Whissi Owned by: wonderboymusic
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.4.2
Component: Cache API Keywords: has-patch needs-docs dev-feedback
Focuses: Cc:

Description

Hi,

I am using object-cache.php Memcached v2.0.2 (Dropins) with the current WordPress version, but I don't think this has anything to do with the dropin.

I noticed that category changes don't invalidate/update the cache.

For example:

  1. Create a category "Foo".
  1. Assign some postings to "Foo".
  1. View your postings in backend and frontend, so that results get cached.
  1. Rename "Foo" category to "Bar".
  1. Reload some postings, which are in that category. But they still display the old "Foo" category instead the new "Bar" category name.

Flushing memcached would "solve" this problem, that's why I think renaming categories don't invalidate/update the cache.

Attachments (4)

22526.diff (7.6 KB) - added by wonderboymusic 16 months ago.
22526.2.diff (892 bytes) - added by kovshenin 16 months ago.
22526.3.diff (3.0 KB) - added by wonderboymusic 16 months ago.
22526.4.diff (10.6 KB) - added by kovshenin 16 months ago.

Download all attachments as: .zip

Change History (21)

comment:1 @wonderboymusic22 months ago

  • Keywords reporter-feedback added

Still an issue?

comment:2 @Whissi21 months ago

  • Keywords reporter-feedback removed

Yes, still an issue with 3.6.

comment:3 @wonderboymusic16 months ago

  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 3.9

comment:4 @wonderboymusic16 months ago

In 27099:

Add a unit test demonstrating the failure to invalidate a post's term cache when the term is updated.

See #22526.

@wonderboymusic16 months ago

comment:5 @wonderboymusic16 months ago

  • Keywords has-patch commit added; needs-patch removed

Bugs like this give me nightmares. 22526.diff keeps the post terms cache in sync with changes to a given taxonomy.

Introduces 3 new functions:
get_taxonomy_last_changed( $taxonomy )
set_taxonomy_last_changed( $taxonomy )
post_taxonomy_is_fresh( $id, $taxonomy )

post_taxonomy_is_fresh() only needs to be called in get_object_term_cache(). Added some unit tests for the functions and for the condition described in the ticket which, surprise surprise, was reproducible with a basic unit test.

comment:6 @wonderboymusic16 months ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 27101:

Add cache invalidation when updating a term, example: create a category, assign it to a post, edit the category. Currently, the post's term cache is not updated. When updating terms in a given taxonomy, invalidate the object term caches linked to that taxonomy.

Introduce get_taxonomy_last_changed(), set_taxonomy_last_changed(), and post_taxonomy_is_fresh().

post_taxonomy_is_fresh() is only called in get_object_term_cache() - at which point the taxonomy's last_changed value is checked against the post's {$taxonomy}_last_changed value.

set_taxonomy_last_changed() is called whenever directory database queries are made that insert new terms or affect existing terms.

Fixes #22526.

comment:7 @DrewAPicture16 months ago

  • Keywords needs-docs added; needs-unit-tests commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Sorry to reopen, but the docs on get_taxonomy_last_changed(), set_taxonomy_last_changed(), and post_taxonomy_is_fresh() are incomplete.

  • All of the function parameters and returns need descriptions.
  • All function descriptions should end in periods.

comment:8 @DrewAPicture16 months ago

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

In 27141:

Improve inline documentation for several 'last_changed'-related functions introduced in [27101] and [27102].

Functions include:

  • get_taxonomy_last_changed()
  • set_taxonomy_last_changed()
  • post_taxonomy_is_fresh()
  • taxonomy_hierarchy_is_fresh()

Fixes #22526, #14485.

@kovshenin16 months ago

comment:9 @kovshenin16 months ago

  • Keywords dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

r27101 introduces a bunch of new tax_last_changed cache groups, and each one will most likely have a single key. Perhaps we can use the existing terms group to hold that? In get_taxonomy_last_changed you can use wp_cache_add() instead of _set() which I believe can be faster on some cache backends.

Also, this seems to introduce quite a lot of new stuff, all of it for the little bug we're trying to fix. Why can't we just use clean_object_term_cache like we do in wp_delete_term? See 22526.2.diff, it passes the unit test added in r27099.

@wonderboymusic16 months ago

comment:10 @wonderboymusic16 months ago

You are correct about the terms bucket, did that here: 22526.3.diff. Not using _set requires _delete and _add, because every falsely value needs to be overwritten. Will look at your other fix now.

comment:11 @wonderboymusic16 months ago

I applied your patch and turned off my checks in get_object_term_cache() and _get_term_hierarchy(). Unit tests fail for both. Is .2.diff additional or a replacement?

comment:12 @wonderboymusic16 months ago

After thinking about it, I don't think delete/add is an upgrade from set. I can see the benefit of add when your goal is "I want something in the cache, no matter what, so as long as its there, we're good" but set is more appropriate for "the value needs to be overwritten, no matter what."

Also, for cleanliness' sake, I can see the reason to use the terms bucket, so I will probably make that change, but WP doesn't give you any way to invalidate by group, so the change is really arbitrary.

comment:13 @wonderboymusic16 months ago

In 27142:

Reuse the terms cache group for taxonomy cache invalidation.

See #22526, #14485, [27101], [27102].

comment:14 @kovshenin16 months ago

Sorry, re add vs set, I was only referring to the get_taxonomy_last_changed() function. You already check $last_changed. If that's false, then the value is not in cache, because microtime() can not return false, so you don't really need to delete.

@kovshenin16 months ago

comment:15 @kovshenin16 months ago

Re .2.diff, it's only a fix for this ticket and you'll need to revert your changesets before applying it. I added 22526.4.diff which reverts (or partially reverts) r27142, r27141, r27102 and r27101, adds the fix in .2.diff:

$ phpunit --group 22526
OK (1 test, 2 assertions)

This also reverts the fix in #14485, I'll reopen it and add a new patch with a simpler fix.

comment:16 @wonderboymusic16 months ago

Uh, I just applied your patch and did the following:

1) define( 'WP_TESTS_FORCE_KNOWN_BUGS', true );
2) phpunit tests/phpunit/tests/term/cache.php

Results:
Tests: 3, Assertions: 8, Failures: 2.

comment:17 @wonderboymusic16 months ago

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

In 27163:

Partially revert [27101], [27102], [27141], and [27142]. Those commits introduced new functions to sync up cache invalidation events. The current commit alters existing internals.

"The cache invalidation with static was introduced in r9102 with version 2.7. Multisite wasn't in core back then, so something like switch_to_blog() wasn't a concern, but now it breaks if you switch the blog in between calls to clean_term_cache."

This solution is simpler. All unit tests pass. Removes unnecessary tests linked to removed functions.

Props kovshenin.
Fixes #14485, #22526.

Note: See TracTickets for help on using tickets.