Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#54511 closed enhancement (fixed)

Remove cache time limitation in WP_Term_Query

Reported by: spacedmonkey's profile spacedmonkey Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.0 Priority: normal
Severity: normal Version: 4.6
Component: Taxonomy Keywords: has-patch has-dev-note
Focuses: performance Cc:

Description (last modified by spacedmonkey)

Currently there is a limitation, that term caches found in WP_Term_Query will only last one day. See the following snippet.

wp_cache_add( $cache_key, $terms, 'terms', DAY_IN_SECONDS );

If we can ensure that cache invalidation is happening correctly, this limitation can be removed. Once this limitation is removed, cache will live until their are correctly invalidation.

Change History (17)

#1 @spacedmonkey
3 years ago

  • Description modified (diff)

#2 @flixos90
3 years ago

I don't recall why this was introduced. Maybe there are some limitations in the current cache invalidation for term queries, and this was implemented to more or less cater for that. Or it could have been to not pollute the object cache, since those term query hashes used for cache keys can frequently change.

Anyway, since this isn't present in other object type query implementations, at a high level I agree this should be removed at some point. However, we should make sure it isn't indeed there for a reason where removing it could result in a regression.

#3 @dlh
3 years ago

If you'll forgive me, I would be very appreciative if the enhancement proposed in #52710 could be considered alongside this one.

Last edited 3 years ago by dlh (previous) (diff)

#4 @tillkruess
3 years ago

I can see the original argument for filling up the cache unnecessarily, but not for invalidation purposes.

+1

This ticket was mentioned in PR #2162 on WordPress/wordpress-develop by spacedmonkey.


2 years ago
#5

  • Keywords has-patch added

#6 @spacedmonkey
2 years ago

  • Keywords needs-unit-tests added

I have created a patch at #2162, ready for review.

#9 @adamsilverstein
2 years ago

Originally added in https://core.trac.wordpress.org/ticket/35381 - where the day cache was also questioned at the time. Based on the response, the reasons for keeping it seemed week and open for reconsideration. +1 to removing and writing a dev note explaining why.

cc: @boonebgorges hi friend, we are touching your code, feedback welcome!

#10 @boonebgorges
2 years ago

Thanks for the ping!

Looking back at https://core.trac.wordpress.org/ticket/35381#comment:13, I see that we simply inherited this TTL strategy from [15583]. As such, I'm +1 for the change proposed here.

#11 @spacedmonkey
2 years ago

  • Keywords needs-unit-tests removed
  • Milestone changed from Awaiting Review to 6.0

@boonebgorges @adamsilverstein

Are you happy for me to commit this change then?

#12 follow-up: @spacedmonkey
2 years ago

@boonebgorges @adamsilverstein

Are you happy for me to commit this change then?

#13 in reply to: ↑ 12 @adamsilverstein
2 years ago

Replying to spacedmonkey:

@boonebgorges @adamsilverstein

Are you happy for me to commit this change then?

yes, +1 go for it!

#14 @spacedmonkey
2 years ago

  • Owner set to spacedmonkey
  • Status changed from new to assigned

#15 @spacedmonkey
2 years ago

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

In 52669:

Taxonomy: Remove cache expiry limitation in WP_Term_Query.

Remove the one day expiry limitation from query caches found in the WP_Term_Qurery class. Removing this limitation means that the caches will remain in object caching, as long as possible. Ensure that all term / taxonomy cache clear functions invalidate query caches, by deleting the last_changed value in the terms cache group.

Props spacedmonkey, adamsilverstein, boonebgorges, tillkruess, dlh, flixos90.
Fixes #54511.

#16 @spacedmonkey
2 years ago

  • Keywords needs-dev-note added

Dev note ready for review.

Note: See TracTickets for help on using tickets.