Disregard transient cache in `perflab_query_plugin_info()` when a plugin is absent #1694

b1ink0 · 2024-11-22T14:04:26Z

Summary

Relevant technical choices

When a new feature plugin is published, it may not yet exist in the transient. Therefore, whenever one of the expected feature plugin is absent, we should act as if the transient was not set in the first place so that we can obtain the latest plugin info right away.

Logic for this to work:

If the plugin slug is not found in the transient then instead of returning a error we proceeded to fetch information from API.
If the plugin slug is also not found in the API it is also stored in transient but with key as plugin slug and value as error.
If the plugin slug is found in the transient but if error is set then raise the error.

This logic will make sure that the multiple request are not made for the plugin which is absent.

…che when a plugin is absent

github-actions · 2024-11-22T14:07:51Z

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: b1ink0 <[email protected]>
Co-authored-by: mukeshpanchal27 <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: felixarntz <[email protected]>
Co-authored-by: swissspidy <[email protected]>

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

westonruter · 2024-11-22T18:23:35Z

plugins/performance-lab/includes/admin/plugins.php

-				__( 'Plugin not found in WordPress.org API response.', 'performance-lab' )
-			);
+			// Cache the fact that the plugin was not found.
+			$plugins[ $current_plugin_slug ] = false;


Instead of storing false here, what about storing the WP_Error instance? As it stands right now, there are two conditions resulting in false being logged, which conflates these two error scenarios which we tried to get rid of in #1651 to assist with debugging.

Overall this makes sense to me. We probably shouldn't store the WP_Error instance though, but just its data, since storing PHP class instances is a bit unpredictable with what would happen in different object cache implementations.

westonruter · 2024-11-22T18:24:39Z

plugins/performance-lab/includes/admin/plugins.php

@@ -101,9 +102,14 @@ function perflab_query_plugin_info( string $plugin_slug ) {
 		}
 	}

+	if ( ! isset( $plugins[ $plugin_slug ] ) ) {
+		// Cache the fact that the plugin was not found.
+		$plugins[ $plugin_slug ] = false;


See above about storing a WP_Error instance instead.

westonruter · 2024-11-22T18:25:28Z

plugins/performance-lab/includes/admin/plugins.php

+			if ( false === $plugins[ $plugin_slug ] ) {
+				// Plugin was requested before and not found.
+				return new WP_Error(
+					'plugin_not_found',
+					__( 'Plugin not found in cached API response.', 'performance-lab' )
+				);
+			}


If $plugins[ $plugin_slug ] is either an array or a WP_Error, then this can just return that.

Suggested change

if ( false === $plugins[ $plugin_slug ] ) {

// Plugin was requested before and not found.

return new WP_Error(

'plugin_not_found',

__( 'Plugin not found in cached API response.', 'performance-lab' )

);

}

westonruter · 2024-11-22T18:26:29Z

plugins/performance-lab/includes/admin/plugins.php

+		// Cache the fact that the plugin was not found.
+		$plugins[ $plugin_slug ] = false;
+	}
+
 	set_transient( $transient_key, $plugins, HOUR_IN_SECONDS );


Here I think it would be good to check if there was an error condition, and if so, set the expiration to 1 minute instead of HOUR_IN_SECONDS.

westonruter · 2024-11-25T19:48:53Z

There are still these error scenarios that isn't getting cached:

performance/plugins/performance-lab/includes/admin/plugins.php

Lines 58 to 72 in 2a344c9

    
           if ( is_wp_error( $response ) ) { 
        
           	return new WP_Error( 
        
           		'api_error', 
        
           		sprintf( 
        
           			/* translators: %s: API error message */ 
        
           			__( 'Failed to retrieve plugins data from WordPress.org API: %s', 'performance-lab' ), 
        
           			$response->get_error_message() 
        
           		) 
        
           	); 
        
           } 
        
           // Check if the response contains plugins. 
        
           if ( ! ( is_object( $response ) && property_exists( $response, 'plugins' ) ) ) { 
        
           	return new WP_Error( 'no_plugins', __( 'No plugins found in the API response.', 'performance-lab' ) ); 
        
           }

felixarntz

Thanks @b1ink0! This looks good, just one thing that needs to be fixed I think.

felixarntz · 2024-11-26T01:37:17Z

plugins/performance-lab/includes/admin/plugins.php

+
+		set_transient( $transient_key, $plugins, MINUTE_IN_SECONDS );
+	} else {
+		set_transient( $transient_key, $plugins, HOUR_IN_SECONDS );
+	}


This differentiation does not happen in the correct clause. We should use a shorter cache duration in case of any error, not just in the case of this specific error.

I think it would be most straightforward to implement this via a boolean flag like $has_errors that is set if anywhere in the (uncached) logic an error is set. Then the code here could be changed like:

Suggested change

set_transient( $transient_key, $plugins, MINUTE_IN_SECONDS );

} else {

set_transient( $transient_key, $plugins, HOUR_IN_SECONDS );

}

}

if ( $has_errors ) {

set_transient( $transient_key, $plugins, MINUTE_IN_SECONDS );

} else {

set_transient( $transient_key, $plugins, HOUR_IN_SECONDS );

}

felixarntz

@b1ink0 There are two points of follow-up feedback, though some of them have overlap with #1691. But since this is broken too and affected by the same logic, maybe we should work towards a solution that addresses both?

cc @westonruter

plugins/performance-lab/includes/admin/plugins.php

felixarntz · 2024-11-26T18:49:26Z

plugins/performance-lab/includes/admin/plugins.php

+		if ( ! isset( $plugins[ $plugin_slug ] ) ) {
+			// Cache the fact that the plugin was not found.
+			$plugins[ $plugin_slug ] = array(
+				'error' => array(
+					'code'    => 'plugin_not_found',
+					'message' => __( 'Plugin not found in API response.', 'performance-lab' ),
+				),
+			);

-		// Enqueue the required plugins slug by adding it to the queue.
-		if ( isset( $plugin_data['requires_plugins'] ) && is_array( $plugin_data['requires_plugins'] ) ) {
-			$plugin_queue = array_merge( $plugin_queue, $plugin_data['requires_plugins'] );
+			$has_errors = true;
 		}


Now that I look at this again, what is this particular check needed for? Wouldn't this already be catered for by the check above in line 101?

This check is needed because if the $plugin_slug is not in the perflab_get_standalone_plugins() then this $plugin_slug will not be processed inside the loop.

In that case, I think we should make this check at the very beginning of the function, even before looking at the transient cache. This function should never be used for a plugin that is not among our plugins I think, so we could already check that before all the main logic in this function.

We wouldn't want that someone calling the function with e.g. jetpack would trigger this API request and logic to run for every single page load just because jetpack would (rightfully) never be included in the response.

If we only check the $plugin_slug inside perflab_get_standalone_plugins() it will also cause error for the dependency plugins. And if we want to dynamically check the dependency then we need to request the API.

Why does the function need to be called for dependency plugins?

The perflab_render_plugin_card() function is calling the perflab_get_plugin_availability() function
and inside the perflab_get_plugin_availability() on here it is calling the perflab_query_plugin_info() for the dependency plugin.

Ah, that's a good point. But in that case, I think at least this error should use a different message? I would try to clarify it as: If the plugin is not found at this point, we know it's not one of our plugins, since it's not in the API response, and the API response should always included all our plugins plus its dependencies.

plugins/performance-lab/includes/admin/plugins.php

…ng plugins

felixarntz · 2024-12-02T16:33:11Z

plugins/performance-lab/includes/admin/plugins.php

+	if ( $has_errors ) {
+		set_transient( $transient_key, $plugins, MINUTE_IN_SECONDS );

-	if ( ! isset( $plugins[ $plugin_slug ] ) ) {
 		return new WP_Error(
-			'plugin_not_found',
-			__( 'Plugin not found in API response.', 'performance-lab' )
+			$plugins[ $plugin_slug ]['error']['code'],
+			$plugins[ $plugin_slug ]['error']['message']
 		);
 	}

+	set_transient( $transient_key, $plugins, HOUR_IN_SECONDS );


See my response in #1694 (comment), this logic is problematic.

We should only use $has_errors to change the transient duration.

The actual return value needs to depend on the specific $plugins[ $plugin_slug ] value.

felixarntz

@b1ink0 This technically looks like it's working well, though I think there's a few places where the logic can be simplified / consolidated.

This kind of thing is usually not a big deal, but since the conditions here are a bit complex, I think it would be great to make the code more understandable for people looking at it in the future without all the context from what we've been discussing.

felixarntz · 2024-12-03T21:45:18Z

plugins/performance-lab/includes/admin/plugins.php

+	// Cache error for all standalone plugins.
+	if ( $has_errors ) {
+		foreach ( perflab_get_standalone_plugins() as $standalone_plugin ) {
+			$plugins[ $standalone_plugin ] = $plugins[ $plugin_slug ];
+		}
+	}


This works as is, but is hard to follow in terms of code path, because it relies on $has_errors, which is intended for any errors but here it's used to check for whether there are general errors that affect all plugins. It only works correctly because of the position in the code, which is a bit fragile.

I think this would be more intuitive and easier to understand for people looking at the code in the future if we moved this foreach loop into both of the individual conditions where general errors are found (i.e. the conditions in line 57 and 76).

This also avoids an extra if check here that technically is unnecessary.

felixarntz · 2024-12-03T21:46:41Z

plugins/performance-lab/includes/admin/plugins.php


-	// Index the plugins from the API response by their slug for efficient lookup.
-	$all_performance_plugins = array_column( $response->plugins, null, 'slug' );
+	if ( ! $has_errors && is_object( $response ) && property_exists( $response, 'plugins' ) ) {


Instead of reiterating an almost similar condition as above, can we move this to an else or elseif please?

felixarntz · 2024-12-03T21:47:24Z

plugins/performance-lab/includes/admin/plugins.php

 	}

 	// Check if the response contains plugins.
-	if ( ! ( is_object( $response ) && property_exists( $response, 'plugins' ) ) ) {
-		return new WP_Error( 'no_plugins', __( 'No plugins found in the API response.', 'performance-lab' ) );
+	if ( ! $has_errors && ! ( is_object( $response ) && property_exists( $response, 'plugins' ) ) ) {


I think it would make more sense to have this as an elseif. Easier to understand, and then there's no need to check for ! $has_errors.

felixarntz · 2024-12-03T21:50:49Z

plugins/performance-lab/includes/admin/plugins.php

-		if ( ! isset( $plugins[ $plugin_slug ] ) ) {
+	if ( is_array( $plugins ) && isset( $plugins[ $plugin_slug ] ) ) {
+		if ( isset( $plugins[ $plugin_slug ]['error'] ) ) {
+			// Plugin was requested before and not found.


Suggested change

// Plugin was requested before and not found.

// Plugin was requested before but an error occurred for it.

b1ink0 · 2024-12-04T19:12:09Z

Some checks are failing because of disruption to the GItHub services current status.
cc: @felixarntz

felixarntz

@b1ink0 Thank you for the continuous iterations, this looks great!

One minor nit-pick, but not a blocker.

felixarntz · 2024-12-04T23:39:48Z

plugins/performance-lab/includes/admin/plugins.php

-	$plugins      = array();
-	$plugin_queue = perflab_get_standalone_plugins();
+		$has_errors = true;
+	} elseif ( ! ( is_object( $response ) && property_exists( $response, 'plugins' ) ) ) {


Cognitively easier to remove the negation around the whole clause.

Suggested change

} elseif ( ! ( is_object( $response ) && property_exists( $response, 'plugins' ) ) ) {

} elseif ( ! is_object( $response ) || ! property_exists( $response, 'plugins' ) ) {

felixarntz · 2024-12-04T23:44:19Z

@b1ink0 @westonruter FYI I've updated the PR description to indicate this addresses both #1617 and #1691.

westonruter · 2024-12-05T00:12:35Z

plugins/performance-lab/includes/admin/plugins.php

+				continue;
+			}
+
+			$plugin_data                     = $all_performance_plugins[ $current_plugin_slug ];
+			$plugins[ $current_plugin_slug ] = wp_array_slice_assoc( $plugin_data, $fields );
+
+			// Enqueue the required plugins slug by adding it to the queue.
+			if ( isset( $plugin_data['requires_plugins'] ) && is_array( $plugin_data['requires_plugins'] ) ) {
+				$plugin_queue = array_merge( $plugin_queue, $plugin_data['requires_plugins'] );
+			}


Trivial point, but I think an else here makes sense:

Suggested change

continue;

}

$plugin_data = $all_performance_plugins[ $current_plugin_slug ];

$plugins[ $current_plugin_slug ] = wp_array_slice_assoc( $plugin_data, $fields );

// Enqueue the required plugins slug by adding it to the queue.

if ( isset( $plugin_data['requires_plugins'] ) && is_array( $plugin_data['requires_plugins'] ) ) {

$plugin_queue = array_merge( $plugin_queue, $plugin_data['requires_plugins'] );

}

} else {

$plugin_data = $all_performance_plugins[ $current_plugin_slug ];

$plugins[ $current_plugin_slug ] = wp_array_slice_assoc( $plugin_data, $fields );

// Enqueue the required plugins slug by adding it to the queue.

if ( isset( $plugin_data['requires_plugins'] ) && is_array( $plugin_data['requires_plugins'] ) ) {

$plugin_queue = array_merge( $plugin_queue, $plugin_data['requires_plugins'] );

}

}

plugins/performance-lab/includes/admin/plugins.php

westonruter · 2024-12-05T00:16:07Z

plugins/performance-lab/includes/admin/plugins.php

+	if ( $has_errors ) {
+		set_transient( $transient_key, $plugins, MINUTE_IN_SECONDS );
+	} else {
+		set_transient( $transient_key, $plugins, HOUR_IN_SECONDS );
+	}


To reduce code duplication:

Suggested change

if ( $has_errors ) {

set_transient( $transient_key, $plugins, MINUTE_IN_SECONDS );

} else {

set_transient( $transient_key, $plugins, HOUR_IN_SECONDS );

}

set_transient( $transient_key, $plugins, $has_errors ? MINUTE_IN_SECONDS : HOUR_IN_SECONDS );

b1ink0 and others added 4 commits November 20, 2024 13:52

Update 'perflab_query_plugin_info' function to disregard transient ca…

d55cf6b

…che when a plugin is absent

Merge branch 'WordPress:trunk' into update/disregard-transient-cache

d629ec2

Add logic for preventing re-request if dependencies not found

54532b5

Update the error message

c923c97

b1ink0 marked this pull request as ready for review November 22, 2024 14:07

b1ink0 requested review from mukeshpanchal27 and felixarntz as code owners November 22, 2024 14:07

westonruter added this to the performance-lab n.e.x.t milestone Nov 22, 2024

westonruter added [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only [Type] Bug An existing feature is broken [Type] Enhancement A suggestion for improvement of an existing feature and removed [Type] Bug An existing feature is broken labels Nov 22, 2024

westonruter requested changes Nov 22, 2024

View reviewed changes

b1ink0 added 2 commits November 25, 2024 19:56

Refactor to store error instead of false

89b4c7f

Adjust transient expiration time based on plugin availability

2a344c9

b1ink0 requested a review from westonruter November 25, 2024 14:38

felixarntz requested changes Nov 26, 2024

View reviewed changes

b1ink0 and others added 2 commits November 26, 2024 11:35

Merge branch 'WordPress:trunk' into update/disregard-transient-cache

46b1216

Add missing errors to cache, Shorten cache duration for any error

ff1b440

b1ink0 requested a review from felixarntz November 26, 2024 17:32

felixarntz reviewed Nov 26, 2024

View reviewed changes

b1ink0 commented Nov 27, 2024

View reviewed changes

plugins/performance-lab/includes/admin/plugins.php Outdated Show resolved Hide resolved

Cache error for standalone plugins and update error message for missi…

1d3ef59

…ng plugins

felixarntz reviewed Dec 2, 2024

View reviewed changes

b1ink0 and others added 2 commits December 3, 2024 23:35

Merge branch 'trunk' into update/disregard-transient-cache

80573b6

Change transient cache duration if any error is encountered

49692cb

b1ink0 requested a review from felixarntz December 3, 2024 18:14

felixarntz reviewed Dec 3, 2024

View reviewed changes

b1ink0 added 4 commits December 5, 2024 00:14

Refactor to move foreach loop into individual conditions

c376ca2

Refactor to move logic to elseif and else condition

74cc52e

Clarify error message comment

aeb9fe5

Merge branch 'trunk' into update/disregard-transient-cache

74ab1e2

b1ink0 requested a review from felixarntz December 4, 2024 19:09

felixarntz approved these changes Dec 4, 2024

View reviewed changes

westonruter reviewed Dec 5, 2024

View reviewed changes

plugins/performance-lab/includes/admin/plugins.php Show resolved Hide resolved

westonruter reviewed Dec 5, 2024

View reviewed changes

b1ink0 added 3 commits December 5, 2024 21:38

Refactor conditional logic to improve readability

c0fa985

Simplify transient cache duration logic

3868965

Merge branch 'trunk' into update/disregard-transient-cache

7709e41

b1ink0 requested a review from westonruter December 5, 2024 16:18

westonruter approved these changes Dec 5, 2024

View reviewed changes

Merge branch 'trunk' into update/disregard-transient-cache

a090c1b

mukeshpanchal27 approved these changes Dec 10, 2024

View reviewed changes

Merge branch 'trunk' into update/disregard-transient-cache

571027b

mukeshpanchal27 merged commit 9661a3c into WordPress:trunk Dec 10, 2024
13 checks passed

westonruter changed the title ~~Disregard transient cache in perflab_query_plugin_info() when a plugin is absent~~ Disregard transient cache in perflab_query_plugin_info() when a plugin is absent Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disregard transient cache in `perflab_query_plugin_info()` when a plugin is absent #1694

Disregard transient cache in `perflab_query_plugin_info()` when a plugin is absent #1694

b1ink0 commented Nov 22, 2024 •

edited by felixarntz

Loading

github-actions bot commented Nov 22, 2024 •

edited

Loading

westonruter Nov 22, 2024

felixarntz Nov 23, 2024

westonruter Nov 22, 2024

westonruter Nov 22, 2024

westonruter Nov 22, 2024

westonruter commented Nov 25, 2024

felixarntz left a comment

felixarntz Nov 26, 2024

felixarntz left a comment

felixarntz Nov 26, 2024

b1ink0 Nov 27, 2024

felixarntz Nov 27, 2024

b1ink0 Nov 27, 2024 •

edited

Loading

felixarntz Nov 27, 2024

b1ink0 Nov 27, 2024

felixarntz Nov 28, 2024

felixarntz Dec 2, 2024

felixarntz left a comment

felixarntz Dec 3, 2024

felixarntz Dec 3, 2024

felixarntz Dec 3, 2024

felixarntz Dec 3, 2024

b1ink0 commented Dec 4, 2024 •

edited

Loading

felixarntz left a comment

felixarntz Dec 4, 2024

felixarntz commented Dec 4, 2024

westonruter Dec 5, 2024

westonruter Dec 5, 2024

	// Plugin was requested before and not found.
	// Plugin was requested before but an error occurred for it.

	} elseif ( ! ( is_object( $response ) && property_exists( $response, 'plugins' ) ) ) {
	} elseif ( ! is_object( $response ) \|\| ! property_exists( $response, 'plugins' ) ) {

Disregard transient cache in perflab_query_plugin_info() when a plugin is absent #1694

Disregard transient cache in perflab_query_plugin_info() when a plugin is absent #1694

Conversation

b1ink0 commented Nov 22, 2024 • edited by felixarntz Loading

Summary

Relevant technical choices

github-actions bot commented Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Nov 25, 2024

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b1ink0 Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b1ink0 commented Dec 4, 2024 • edited Loading

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Disregard transient cache in `perflab_query_plugin_info()` when a plugin is absent #1694

Disregard transient cache in `perflab_query_plugin_info()` when a plugin is absent #1694

b1ink0 commented Nov 22, 2024 •

edited by felixarntz

Loading

github-actions bot commented Nov 22, 2024 •

edited

Loading

b1ink0 Nov 27, 2024 •

edited

Loading

b1ink0 commented Dec 4, 2024 •

edited

Loading