Disregard transient cache in perflab_query_plugin_info() when a plugin is absent #1694
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
added
[Plugin] Performance Lab
| __( 'Plugin not found in WordPress.org API response.', 'performance-lab' ) | ||
| ); | ||
| // Cache the fact that the plugin was not found. | ||
| $plugins[ $current_plugin_slug ] = false; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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; | |||
There was a problem hiding this comment.
See above about storing a WP_Error instance instead.
| 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' ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
If $plugins[ $plugin_slug ] is either an array or a WP_Error, then this can just return that.
| 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' ) | |
| ); | |
| } |
| // Cache the fact that the plugin was not found. | ||
| $plugins[ $plugin_slug ] = false; | ||
| } | ||
|
|
||
| set_transient( $transient_key, $plugins, HOUR_IN_SECONDS ); |
There was a problem hiding this comment.
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.
Summary
Fixes #1617
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:
false.falseraise the error.This logic will make sure that the multiple request are not made for the plugin which is absent.