Fix WordPress.DB.DirectDatabaseQuery.DirectQuery warning for Autoloaded Options Health Check#1179
Conversation
Ref issue: 919 WordPress#919
Ellusu
requested review from
felixarntz,
manuelRod and
mukeshpanchal27
as code owners
|
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. |
mukeshpanchal27
added
[Type] Bug
mukeshpanchal27
changed the title
WordPress.DB.DirectDatabaseQuery.DirectQuery warning for Autoloaded Options Health Check
Co-authored-by: Mukesh Panchal <[email protected]>
joemcgill
left a comment
joemcgill
left a comment
There was a problem hiding this comment.
I tried resolving conflicts to this branch following some of the upstream PHPStan changes were applied, and looks like it's caught that the return value is slightly changed following this change. Would be good to review and see if the docblock just needs to be updated, or if something in the function needs to be updated.
Ah, my bad. I got the typing wrong here: https://github.com/WordPress/performance/pull/1188/files#r1591653385 Fixed in 6f89a54. |
mukeshpanchal27
left a comment
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Thanks for the update.
Left some nit-pick. Can we update test_perflab_aao_autoloaded_options_size tests to remove the query?
westonruter
left a comment
westonruter
left a comment
There was a problem hiding this comment.
LGTM. Let's merge once @mukeshpanchal27 approves.
| ); | ||
| $disable_button = sprintf( '<a class="button" href="%s">%s</a>', esc_url( $url ), esc_html__( 'Revert to Autoload', 'performance-lab' ) ); | ||
| $html_table .= sprintf( '<tr><td>%s</td><td>%s</td><td>%s</td></tr>', esc_html( $value->option_name ), size_format( $value->option_value_length, 2 ), $disable_button ); | ||
| $html_table .= sprintf( '<tr><td>%s</td><td>%s</td><td>%s</td></tr>', esc_html( $option_name ), size_format( $option_length, 2 ), $disable_button ); |
There was a problem hiding this comment.
With the PHPStan level updates, this line was now giving an error about $option_name potentially not being a string. In normal plugin operation this would always be a string. But since any other plugin can manipulate options as well, there's no guarantee that it will be an array of strings. So I've added these checks in 3dd2f69.
| if ( count( $disabled_options_summary ) === 0 ) { | ||
| return ''; | ||
| } |
There was a problem hiding this comment.
I realized that with the refactor here, it is possible for $disabled_options_summary to be an empty array at this point, if none of the options are in the database anymore. This is quite unlikely, but to account for this I've added e6f8650.
4 participants
Fixes #919
Summary
Fix to reduce direct database call as suggested by Plugin Check
OLD:
NEW: