Make WordPress Core

Opened 9 months ago

Closed 8 months ago

Last modified 8 months ago

#56605 closed defect (bug) (fixed)

Reconsider wp_cache_supports_group_flush implementation

Reported by: johnjamesjacoby's profile johnjamesjacoby Owned by: tillkruess's profile tillkruess
Milestone: 6.1 Priority: normal
Severity: normal Version: 6.1
Component: Cache API Keywords: 2nd-opinion changes-requested has-patch has-unit-tests add-to-field-guide
Focuses: Cc:

Description

r53767 introduced the ability to flush cache groups via #4476, but I believe it may include a non-trivial oversight.

The new wp_cache_supports_group_flush() function simply returns true or false, and will require a drop-in plugin to redefine this function in a pluggable way to override its return value.

This approach does not empower the $wp_object_cache global Class to define for itself whether or not its backend supports flushing by cache group. Instead, it relies on the internal exceptions of wp_start_object_cache() to skip loading wp-includes/cache.php (which returns unpluggable true) and load wp-includes/cache-compat.php (which returns pluggable false).

This makes this new "flag" or "setting" unique relative to the other Cache API functions located in wp-includes/cache.php, which my intuition tells me is likely not the ideal DUX, or an intentional implementation detail for this new feature in WordPress 6.1.


I would like to politely suggest that before 6.1 ships without it, that the WP_Object_Cache be considered to have added to it some method of checking and reporting if the cache backend it interfaces with supports flushing groups (or potentially any future thing?)

Perhaps that means a new WP_Object_Cache::supports( $feature = '' ) {} method and a new wp_cache_supports( $feature = '' ) {} function?

Perhaps that just means a new WP_Object_Cache::can_flush_groups() method specifically (though, I'd hate to add it now with the idea of deprecating it later for supports() anyways.)

Thoughts?

Change History (13)

#1 @johnjamesjacoby
9 months ago

  • Summary changed from Move supports_group_flush to Reconsider wp_cache_supports_group_flush implementation

#2 in reply to: ↑ description @SergeyBiryukov
9 months ago

Thanks for the ticket!

Replying to johnjamesjacoby:

Perhaps that means a new WP_Object_Cache::supports( $feature = '' ) {} method and a new wp_cache_supports( $feature = '' ) {} function?

That makes sense to me and seems more future-proof.

It looks like the current implementation is the result of the discussion on PR #2368 Delete Cache by Group.

#3 @spacedmonkey
8 months ago

@tillkruess What do you think?

@johnjamesjacoby Any chance of a POC PR to better show what you mean?

#4 @spacedmonkey
8 months ago

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

#5 @tillkruess
8 months ago

The wp_cache_supports() sounds like a good idea, because WP_Object_Cache may not be present.

@johnjamesjacoby Would each plugin supply it as usual and a fallback in core?

This ticket was mentioned in PR #3347 on WordPress/wordpress-develop by SergeyBiryukov.


8 months ago
#6

  • Keywords has-patch has-unit-tests added

#7 @SergeyBiryukov
8 months ago

Added a PR to replace wp_cache_supports_group_flush() with wp_cache_supports().

#8 @tillkruess
8 months ago

Are there any other features like the wp_cache_*_multiple() methods we want to allow checking support for?

tillkruss commented on PR #3347:


8 months ago
#9

Are there any other features like the wp_cache_*_multiple() methods we want to allow checking support for? It would be handy to have for various diagnostics.

#10 follow-up: @spacedmonkey
8 months ago

@SergeyBiryukov You can commit if you wish. However, dev notes will have to be updated. CC @milana_cap

#11 in reply to: ↑ 10 @milana_cap
8 months ago

  • Keywords add-to-field-guide added

Replying to spacedmonkey:

@SergeyBiryukov You can commit if you wish. However, dev notes will have to be updated. CC @milana_cap

Ticket added to my watch list. Thank you @spacedmonkey

#12 @SergeyBiryukov
8 months ago

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

In 54448:

Cache API: Introduce wp_cache_supports() function.

WordPress has recently introduced a variety of caching API improvements:

  • wp_cache_add_multiple()
  • wp_cache_set_multiple()
  • wp_cache_get_multiple()
  • wp_cache_delete_multiple()
  • wp_cache_flush_runtime()
  • wp_cache_flush_group()

Although WordPress core provides a compatibility layer if these functions are missing from third-party object cache implementations, there should be a method of checking whether the cache backend supports a particular feature.

This commit aims to improve developer experience by allowing third-party object cache plugins to declare a wp_cache_supports() function and correctly list their supported features:

  • add_multiple
  • set_multiple
  • get_multiple
  • delete_multiple
  • flush_runtime
  • flush_group

Note: The wp_cache_supports() function replaces and supersedes the wp_cache_supports_group_flush() function added earlier.

Follow-up to [47938], [47944], [52700], [52703], [52706], [52708], [53763], [53767], [54423].

Props johnjamesjacoby, tillkruess, spacedmonkey, SergeyBiryukov.
Fixes #56605.

SergeyBiryukov commented on PR #3347:


8 months ago
#13

Merged in r54448.

Note: See TracTickets for help on using tickets.