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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d55cf6b
Update 'perflab_query_plugin_info' function to disregard transient ca…
b1ink0 Nov 20, 2024
d629ec2
Merge branch 'WordPress:trunk' into update/disregard-transient-cache
b1ink0 Nov 22, 2024
54532b5
Add logic for preventing re-request if dependencies not found
b1ink0 Nov 22, 2024
c923c97
Update the error message
b1ink0 Nov 22, 2024
89b4c7f
Refactor to store error instead of false
b1ink0 Nov 25, 2024
2a344c9
Adjust transient expiration time based on plugin availability
b1ink0 Nov 25, 2024
46b1216
Merge branch 'WordPress:trunk' into update/disregard-transient-cache
b1ink0 Nov 26, 2024
ff1b440
Add missing errors to cache, Shorten cache duration for any error
b1ink0 Nov 26, 2024
1d3ef59
Cache error for standalone plugins and update error message for missi…
b1ink0 Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 78 additions & 38 deletions plugins/performance-lab/includes/admin/plugins.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ function perflab_query_plugin_info( string $plugin_slug ) {
$transient_key = 'perflab_plugins_info';
$plugins = get_transient( $transient_key );

if ( is_array( $plugins ) ) {
// If the specific plugin_slug is not in the cache, return an error.
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.
return new WP_Error(
'plugin_not_found',
__( 'Plugin not found in cached API response.', 'performance-lab' )
$plugins[ $plugin_slug ]['error']['code'],
$plugins[ $plugin_slug ]['error']['message']
);
}
return $plugins[ $plugin_slug ]; // Return cached plugin info if found.
Expand All @@ -54,61 +54,101 @@ function perflab_query_plugin_info( string $plugin_slug ) {
)
);

$has_errors = false;
$plugins = array();

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' ),
),
);

Comment on lines 60 to +83
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

$has_errors = true;
}

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

// 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' ) ) {
$plugin_queue = perflab_get_standalone_plugins();

// Start processing the plugins using a queue-based approach.
while ( count( $plugin_queue ) > 0 ) { // phpcs:ignore Squiz.PHP.DisallowSizeFunctionsInLoops.Found
$current_plugin_slug = array_shift( $plugin_queue );
// Index the plugins from the API response by their slug for efficient lookup.
$all_performance_plugins = array_column( $response->plugins, null, 'slug' );

if ( isset( $plugins[ $current_plugin_slug ] ) ) {
continue;
}
// Start processing the plugins using a queue-based approach.
while ( count( $plugin_queue ) > 0 ) { // phpcs:ignore Squiz.PHP.DisallowSizeFunctionsInLoops.Found
$current_plugin_slug = array_shift( $plugin_queue );

if ( ! isset( $all_performance_plugins[ $current_plugin_slug ] ) ) {
return new WP_Error(
'plugin_not_found',
__( 'Plugin not found in WordPress.org API response.', 'performance-lab' )
);
if ( isset( $plugins[ $current_plugin_slug ] ) ) {
continue;
}

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

@b1ink0 b1ink0 Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

$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'] );
}
}

$plugin_data = $all_performance_plugins[ $current_plugin_slug ];
$plugins[ $current_plugin_slug ] = wp_array_slice_assoc( $plugin_data, $fields );
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' => __( 'The requested plugin is not part of Performance Lab plugins.', '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;
}
}

set_transient( $transient_key, $plugins, HOUR_IN_SECONDS );
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 );

/**
* Validated (mostly) plugin data.
*
Expand Down