WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 years ago

#40420 new defect (bug)

key/group naming confusion in _get_non_cached_ids()

Reported by: johnjamesjacoby Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.9
Component: Cache API Keywords:
Focuses: performance Cc:
PR Number:

Description

The _get_non_cached_ids() function uses a parameter named $cache_key that actually represents a cache group everywhere else within WordPress.

This means when looking at the block or inline documentations, a developer may accidentally be guided down the path of passing in an actual cache-key instead of the cache-group they're checking against.

My recommendation would be to clarify this bit of code to use $group or $cache_group rather than $cache_key, and update any surrounding code documentation to be explicit if it needs it. It's a non-functional change, but will make the code more clear for people.

(Related: BuddyPress comes packaged with it's own similar helper function, and opted to use $cache_group.)

Attachments (1)

40420.patch (824 bytes) - added by johnjamesjacoby 3 years ago.

Download all attachments as: .zip

Change History (2)

#1 @johnjamesjacoby
3 years ago

  • Focuses performance added

40420.patch paints the bikeshed in the following ways:

  • Uses $cache_group instead of $cache_key
  • Updates "cache bucket" documentation to "cache group"

Notes:

  • Core mostly $group instead of $cache_group everywhere else but 1 place (the customizer) which was newly introduced in 4.7.0. (See: WP_Customize_Manager->find_changeset_post_id())
  • My personal preference would be to use $cache_group everywhere, but that's a lot of code-churn
  • $group reads too ambiguously to me, especially given the context of BuddyPress Groups

Additional findings:

  • We do !wp_cache_get() here, which might actually be a logic error. It's plausible this should be a strict false comparison, as the intended functionality is to identify totally uncached keys, and not a value that might be an empty string or array. (This type of problem exists through-out WordPress already though.)
  • See #22174 & #20875 for other nice improvements to this function
Note: See TracTickets for help on using tickets.