Make WordPress Core

Opened 7 years ago

Closed 14 months ago

#40420 closed defect (bug) (fixed)

key/group naming confusion in _get_non_cached_ids()

Reported by: johnjamesjacoby's profile johnjamesjacoby Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.3 Priority: normal
Severity: normal Version: 3.9
Component: Cache API Keywords: has-patch
Focuses: performance, coding-standards Cc:

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 7 years ago.

Download all attachments as: .zip

Change History (5)

#1 @johnjamesjacoby
7 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

#2 @spacedmonkey
14 months ago

  • Focuses coding-standards added
  • Keywords has-patch added

@SergeyBiryukov Pinging as this is related to renaming variables.

#3 @SergeyBiryukov
14 months ago

  • Milestone changed from Awaiting Review to 6.3

Thanks for the ping!

This makes sense to me, and I agree that $cache_group would be the preferable name here.

#4 @SergeyBiryukov
14 months ago

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

In 55566:

Cache API: Correct the second parameter's name in _get_non_cached_ids().

The parameter represents the cache group, not the cache key.

Follow-up to [19918].

Props johnjamesjacoby, spacedmonkey.
Fixes #40420.

Note: See TracTickets for help on using tickets.