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 false.
If the plugin slug is found in the transient but if its value is equal to false 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: 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

felixarntz · 2024-11-26T18:44:23Z

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

 	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()
-			)
+		$plugins[ $plugin_slug ] = array(
+			'error' => array(
+				'code'    => 'api_error',
+				'message' => sprintf(
+					/* translators: %s: API error message */
+					__( 'Failed to retrieve plugins data from WordPress.org API: %s', 'performance-lab' ),
+					$response->get_error_message()
+				),
+			),
 		);
+
+		$has_errors = true;
 	}

 	// 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' ) ) ) {
+		$plugins[ $plugin_slug ] = array(
+			'error' => array(
+				'code'    => 'no_plugins',
+				'message' => __( 'No plugins found in the API response.', 'performance-lab' ),
+			),
+		);
+


I think for both of these scenarios we need to set the error for every single plugin of ours. In other words, iterate through the list from perflab_get_standalone_plugins() and set the same error for every plugin's slug.

Otherwise, this can cause a problem where the API failing would cause the request to be issued for every single plugin, which would defeat the purpose of the transient cache and the combination of those plugin requests into a single API request (see #1542). This would effectively fix #1691, and since this PR is touching the exact same logic, I wonder whether we should go straight to a solution that addresses both issues.

To make things clear as its getting confusing please answer these below question or my understanding of the solution needed.

If error is encounterd at line 57 and 76 we should add the error to the all perflab_get_standalone_plugins plugins so that if any one of the plugin is requested it should return error?

Correct. That's because a failing API response is by definition an error that affects all plugins.

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.

b1ink0 · 2024-11-27T14:28:00Z

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

+						'message' => __( 'Plugin not found in API response.', 'performance-lab' ),
+					),
+				);
+				continue;


Due to the recent commit ff1b440 this could cause incorrect behavour if $plugin_slug is not found in the all_performance_plugins it will add error for that $plugin_slug as we are now using the $has_errors flag which will not be set to true in this case. The below logic will fail.
Solution could be to check it using the isset( $plugins[ $plugin_slug ]['error'] ) condition but it will defect the usage of the $has_errors flag.

if ( $has_errors ) { set_transient( $transient_key, $plugins, MINUTE_IN_SECONDS ); return new WP_Error( $plugins[ $plugin_slug ]['error']['code'], $plugins[ $plugin_slug ]['error']['message'] ); }

Reason for the $has_errors flag not setting: As the errors could occur for other plugin ( not $plugin_slug ) currently code will cache the error but will return the requested $plugin_slug information.

Question: Should we return error even if the error is encountered for the other plugin ( not $plugin_slug ) ?
cc: @felixarntz

See my reply in #1694 (comment) - I think this problem would become irrelevant if we checked that the plugin slug is among our performance plugins in the beginning of the function?

Revisiting this: Why should we not set the $has_errors flag to true here? I think we should, given this is an error too.

Reason for the $has_errors flag not setting: As the errors could occur for other plugin ( not $plugin_slug ) currently code will cache the error but will return the requested $plugin_slug information.

Question: Should we return error even if the error is encountered for the other plugin ( not $plugin_slug ) ?

This above is reason. And question related to your question.

…ng plugins

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

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

1d3ef59

…ng plugins

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

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

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

b1ink0 Nov 27, 2024 •

edited

Loading

felixarntz Nov 27, 2024

felixarntz Nov 28, 2024

b1ink0 Nov 28, 2024

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

Are you sure you want to change the base?

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

Conversation

b1ink0 commented Nov 22, 2024

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

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

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

github-actions bot commented Nov 22, 2024 •

edited

Loading

b1ink0 Nov 27, 2024 •

edited

Loading

b1ink0 Nov 27, 2024 •

edited

Loading