Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Site Health test for Object Caching #1301

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from

Conversation

mkurdybanowski
Copy link

@mkurdybanowski mkurdybanowski commented Jun 13, 2024

Summary

Fixes #718

Relevant technical choices

Copy link

github-actions bot commented Jun 13, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mkurdybanowski <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: mukeshpanchal27 <[email protected]>
Co-authored-by: OllieJones <[email protected]>
Co-authored-by: tillkruss <[email protected]>
Co-authored-by: adamsilverstein <[email protected]>
Co-authored-by: spacedmonkey <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

* Helper functions used for Object Cache Support Info.
*
* @package performance-lab
* @since 2.1.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @since 2.1.0
* @since n.e.x.t

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

* Callback for Object Cache Info fields.
*
* @return array Fields.
* @since 3.3.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @since 3.3.0
* @since n.e.x.t

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

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',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't Enabled and Disabled be translated?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

* @return string
* @since 3.3.0
*/
function wp_get_cache_type() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need a Performance Lab prefix.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

* problems with this function's ability to determine which object cache is being used.
*
* @return string
* @since 3.3.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @since 3.3.0
* @since n.e.x.t

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

* @return array Fields.
* @since 3.3.0
*/
function object_cache_supported_fields() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function needs the Performance Lab prefix.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Comment on lines 15 to 20
*
* @param array $info Site Health Info.
*
* @return array Amended Info.
* @since 3.3.0
*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

* @since 3.3.0
*
*/
function object_cache_supported_info( array $info ): array {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs Performance Lab prefix.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

* Hook callbacks used for Site Health Info.
*
* @package performance-lab
* @since 2.1.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @since 2.1.0
* @since n.e.x.t

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@westonruter westonruter changed the title #718: object caching info Add Site Health test for Object Caching Jun 18, 2024
@westonruter westonruter added the [Type] Feature A new feature within an existing module label Jun 18, 2024
@westonruter westonruter added this to the performance-lab n.e.x.t milestone Jun 18, 2024
@westonruter
Copy link
Member

@mkurdybanowski Hi! Are you planning to pick this up again?

@westonruter westonruter removed this from the performance-lab 3.3.0 milestone Jul 9, 2024
@mkurdybanowski
Copy link
Author

mkurdybanowski commented Jul 12, 2024

Hi @westonruter, yes, should be able to work on it next week!

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() ) {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter will fix mentioned issues after 12 of August, as I just started holidays.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great. Enjoy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Feature A new feature within an existing module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Module Proposal: Add object caching information to site health
4 participants