Add Site Health test for Object Caching#1301
Open
mkurdybanowski wants to merge 8 commits intoWordPress:trunkfrom
Open
Add Site Health test for Object Caching#1301mkurdybanowski wants to merge 8 commits intoWordPress:trunkfrom
mkurdybanowski wants to merge 8 commits intoWordPress:trunkfrom
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
westonruter
requested changes
Jun 18, 2024
| * Helper functions used for Object Cache Support Info. | ||
| * | ||
| * @package performance-lab | ||
| * @since 2.1.0 |
Member
There was a problem hiding this comment.
Suggested change
| * @since 2.1.0 | |
| * @since n.e.x.t |
| * Callback for Object Cache Info fields. | ||
| * | ||
| * @return array Fields. | ||
| * @since 3.3.0 |
Member
There was a problem hiding this comment.
Suggested change
| * @since 3.3.0 | |
| * @since n.e.x.t |
Comment on lines
+27
to
+39
| 'value' => wp_cache_supports( 'get_multiple' ) ? 'Enabled' : 'Disabled', | ||
| ), | ||
| 'multiple_sets' => array( | ||
| 'label' => __( 'Multiple sets', 'performance-lab' ), | ||
| 'value' => wp_cache_supports( 'set_multiple' ) ? 'Enabled' : 'Disabled', | ||
| ), | ||
| 'multiple_deletes' => array( | ||
| 'label' => __( 'Multiple deletes', 'performance-lab' ), | ||
| 'value' => wp_cache_supports( 'delete_multiple' ) ? 'Enabled' : 'Disabled', | ||
| ), | ||
| 'flush_group' => array( | ||
| 'label' => __( 'Flush group', 'performance-lab' ), | ||
| 'value' => wp_cache_supports( 'flush_group' ) ? 'Enabled' : 'Disabled', |
Member
There was a problem hiding this comment.
Shouldn't Enabled and Disabled be translated?
| * @return string | ||
| * @since 3.3.0 | ||
| */ | ||
| function wp_get_cache_type() { |
Member
There was a problem hiding this comment.
This will need a Performance Lab prefix.
| * problems with this function's ability to determine which object cache is being used. | ||
| * | ||
| * @return string | ||
| * @since 3.3.0 |
Member
There was a problem hiding this comment.
Suggested change
| * @since 3.3.0 | |
| * @since n.e.x.t |
| * @return array Fields. | ||
| * @since 3.3.0 | ||
| */ | ||
| function object_cache_supported_fields() { |
Member
There was a problem hiding this comment.
Function needs the Performance Lab prefix.
Comment on lines
+15
to
+20
| * | ||
| * @param array $info Site Health Info. | ||
| * | ||
| * @return array Amended Info. | ||
| * @since 3.3.0 | ||
| * |
Member
There was a problem hiding this comment.
Suggested change
| * | |
| * @param array $info Site Health Info. | |
| * | |
| * @return array Amended Info. | |
| * @since 3.3.0 | |
| * | |
| * | |
| * @since n.e.x.t | |
| * | |
| * @param array<string, mixed> $info Site Health Info. | |
| * @return array<string, mixed> Amended Info. |
| * @since 3.3.0 | ||
| * | ||
| */ | ||
| function object_cache_supported_info( array $info ): array { |
Member
There was a problem hiding this comment.
Needs Performance Lab prefix.
| * Hook callbacks used for Site Health Info. | ||
| * | ||
| * @package performance-lab | ||
| * @since 2.1.0 |
Member
There was a problem hiding this comment.
Suggested change
| * @since 2.1.0 | |
| * @since n.e.x.t |
westonruter
changed the title
Member
|
@mkurdybanowski Hi! Are you planning to pick this up again? |
Author
|
Hi @westonruter, yes, should be able to work on it next week! |
mkurdybanowski
requested review from
adamsilverstein,
felixarntz,
pbearne and
spacedmonkey
as code owners
westonruter
requested changes
Jul 30, 2024
Comment on lines
+16
to
+19
| * @param array{object_cache: array{label: string,description: string,fields: array<string, array{label: string,value: string}>}} $info Site Health Info. | ||
| * | ||
| * @return array{object_cache: array{label: string,description: string,fields: array<string, array{label: string,value: string}>}} Amended Info. | ||
| * @since n.e.x.t |
Member
There was a problem hiding this comment.
The typing can be made a bit more generic.
Suggested change
| * @param array{object_cache: array{label: string,description: string,fields: array<string, array{label: string,value: string}>}} $info Site Health Info. | |
| * | |
| * @return array{object_cache: array{label: string,description: string,fields: array<string, array{label: string,value: string}>}} Amended Info. | |
| * @since n.e.x.t | |
| * @since n.e.x.t | |
| * | |
| * @param array<string, array{label: string, description: string, fields: array<string, array{label: string, value: string}>}> $info Site Health Info. | |
| * @return array<string, array{label: string, description: string, fields: array<string, array{label: string, value: string}>}> Amended Info. | |
| * |
| /** | ||
| * Adds Object Cache module to Site Health Info. | ||
| * | ||
| * @param array{object_cache: array{label: string,description: string,fields: array<string, array{label: string,value: string}>}} $info Site Health Info. |
Member
There was a problem hiding this comment.
The object_cache key wouldn't exist yet in the $info being passed in.
| } | ||
|
|
||
| // Test for Xcache object cache (https://plugins.svn.wordpress.org/xcache/trunk/object-cache.php). | ||
| } elseif ( $wp_object_cache instanceof \XCache_Object_Cache ) { // @phpstan-ignore-line |
Member
There was a problem hiding this comment.
What specifically is being ignored for PHPStan on this line? The specific error identifier should be used here to prevent ignoring something unintentionally.
Suggested change
| } elseif ( $wp_object_cache instanceof \XCache_Object_Cache ) { // @phpstan-ignore-line | |
| } elseif ( $wp_object_cache instanceof XCache_Object_Cache ) { // @phpstan-ignore-line |
| } elseif ( isset( $wp_object_cache->mc ) ) { | ||
| $is_memcache = true; | ||
| foreach ( $wp_object_cache->mc as $bucket ) { | ||
| if ( ! $bucket instanceof \Memcache && ! $bucket instanceof \Memcached ) { |
Member
There was a problem hiding this comment.
Suggested change
| if ( ! $bucket instanceof \Memcache && ! $bucket instanceof \Memcached ) { | |
| if ( ! $bucket instanceof Memcache && ! $bucket instanceof Memcached ) { |
|
|
||
| if ( ! empty( $_wp_using_ext_object_cache ) ) { | ||
| // Test for Memcached PECL extension memcached object cache (https://github.com/tollmanz/wordpress-memcached-backend). | ||
| if ( isset( $wp_object_cache->m ) && $wp_object_cache->m instanceof \Memcached ) { |
Member
There was a problem hiding this comment.
Since we're not using namespaces, the \ an be removed.
Suggested change
| if ( isset( $wp_object_cache->m ) && $wp_object_cache->m instanceof \Memcached ) { | |
| if ( isset( $wp_object_cache->m ) && $wp_object_cache->m instanceof Memcached ) { |
| $message = 'APC'; | ||
|
|
||
| // Test for WP Redis (https://wordpress.org/plugins/wp-redis/). | ||
| } elseif ( isset( $wp_object_cache->redis ) && $wp_object_cache->redis instanceof \Redis ) { |
Member
There was a problem hiding this comment.
Suggested change
| } elseif ( isset( $wp_object_cache->redis ) && $wp_object_cache->redis instanceof \Redis ) { | |
| } elseif ( isset( $wp_object_cache->redis ) && $wp_object_cache->redis instanceof Redis ) { |
| $message = 'Redis'; | ||
|
|
||
| // Test for WP LCache Object cache (https://github.com/lcache/wp-lcache). | ||
| } elseif ( isset( $wp_object_cache->lcache ) && $wp_object_cache->lcache instanceof \LCache\Integrated ) { // @phpstan-ignore-line |
Member
There was a problem hiding this comment.
What specifically is being ignored for PHPStan on this line? The specific error identifier should be used here to prevent ignoring something unintentionally.
Suggested change
| } elseif ( isset( $wp_object_cache->lcache ) && $wp_object_cache->lcache instanceof \LCache\Integrated ) { // @phpstan-ignore-line | |
| } elseif ( isset( $wp_object_cache->lcache ) && $wp_object_cache->lcache instanceof \Cache\Integrated ) { // @phpstan-ignore-line |
Comment on lines
+15
to
+17
| * | ||
| * @return array<string, array{label: string, value: string}> Fields. | ||
| * @since n.e.x.t |
Member
There was a problem hiding this comment.
Suggested change
| * | |
| * @return array<string, array{label: string, value: string}> Fields. | |
| * @since n.e.x.t | |
| * | |
| * @since n.e.x.t | |
| * | |
| * @return array<string, array{label: string, value: string}> Fields. |
Comment on lines
+52
to
+59
| * | ||
| * @link https://github.com/wp-cli/wp-cli/blob/0ca6d920123ac904c918d69181edc5071dc92c9d/php/utils-wp.php#L259-L331. | ||
| * | ||
| * @global bool $_wp_using_ext_object_cache Whether external object cache is being used. | ||
| * @global WP_Object_Cache $wp_object_cache Object cache global instance. | ||
| * | ||
| * @return string Object cache type. | ||
| * @since n.e.x.t |
Member
There was a problem hiding this comment.
Typically the @since goes among the first tags.
Suggested change
| * | |
| * @link https://github.com/wp-cli/wp-cli/blob/0ca6d920123ac904c918d69181edc5071dc92c9d/php/utils-wp.php#L259-L331. | |
| * | |
| * @global bool $_wp_using_ext_object_cache Whether external object cache is being used. | |
| * @global WP_Object_Cache $wp_object_cache Object cache global instance. | |
| * | |
| * @return string Object cache type. | |
| * @since n.e.x.t | |
| * | |
| * @since n.e.x.t | |
| * @link https://github.com/wp-cli/wp-cli/blob/0ca6d920123ac904c918d69181edc5071dc92c9d/php/utils-wp.php#L259-L331. | |
| * | |
| * @global WP_Object_Cache $wp_object_cache Object cache global instance. | |
| * | |
| * @return string Object cache type. |
This also removes the $_wp_using_ext_object_cache per below.
Comment on lines
+62
to
+66
| global $_wp_using_ext_object_cache, $wp_object_cache; | ||
|
|
||
| $message = ''; | ||
|
|
||
| if ( ! empty( $_wp_using_ext_object_cache ) ) { |
Member
There was a problem hiding this comment.
Per #1219 let's avoid using empty() here. And it seems we can avoid using the $_wp_using_ext_object_cache private global by instead just using the wp_using_ext_object_cache() function:
Suggested change
| global $_wp_using_ext_object_cache, $wp_object_cache; | |
| $message = ''; | |
| if ( ! empty( $_wp_using_ext_object_cache ) ) { | |
| global $wp_object_cache; | |
| $message = ''; | |
| if ( wp_using_ext_object_cache() ) { |
Author
There was a problem hiding this comment.
@westonruter will fix mentioned issues after 12 of August, as I just started holidays.
westonruter
reviewed
Jan 21, 2025
Comment on lines
+9
to
+11
| if ( ! defined( 'ABSPATH' ) ) { | ||
| exit; // Exit if accessed directly. | ||
| } |
Member
There was a problem hiding this comment.
Per #1815:
Suggested change
| if ( ! defined( 'ABSPATH' ) ) { | |
| exit; // Exit if accessed directly. | |
| } | |
| // @codeCoverageIgnoreStart | |
| if ( ! defined( 'ABSPATH' ) ) { | |
| exit; // Exit if accessed directly. | |
| } | |
| // @codeCoverageIgnoreEnd |
westonruter
reviewed
Jan 21, 2025
Comment on lines
+9
to
+11
| if ( ! defined( 'ABSPATH' ) ) { | ||
| exit; // Exit if accessed directly. | ||
| } |
Member
There was a problem hiding this comment.
Per #1815:
Suggested change
| if ( ! defined( 'ABSPATH' ) ) { | |
| exit; // Exit if accessed directly. | |
| } | |
| // @codeCoverageIgnoreStart | |
| if ( ! defined( 'ABSPATH' ) ) { | |
| exit; // Exit if accessed directly. | |
| } | |
| // @codeCoverageIgnoreEnd |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #718
Relevant technical choices