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

Fix WordPress.DB.DirectDatabaseQuery.DirectQuery warning for Autoloaded Options Health Check #1179

Merged
merged 15 commits into from
May 14, 2024
Merged

Fix WordPress.DB.DirectDatabaseQuery.DirectQuery warning for Autoloaded Options Health Check #1179

merged 15 commits into from
May 14, 2024

Conversation

Ellusu
Copy link
Contributor

@Ellusu Ellusu commented Apr 24, 2024 •
edited by westonruter
Loading

Fixes #919

Summary

Fix to reduce direct database call as suggested by Plugin Check

OLD:
old
NEW:
new

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 24, 2024 •
edited
Loading

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: Ellusu <matteoenna@git.wordpress.org>
Co-authored-by: joemcgill <joemcgill@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>

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

@Ellusu Ellusu closed this Apr 24, 2024
@Ellusu Ellusu reopened this Apr 26, 2024
@Ellusu Ellusu closed this Apr 26, 2024
@Ellusu Ellusu reopened this Apr 28, 2024
@Ellusu Ellusu mentioned this pull request Apr 29, 2024
Closed
@mukeshpanchal27 mukeshpanchal27 added [Type] Bug An existing feature is broken no milestone PRs that do not have a defined milestone for release [Module] Audit Autoloaded Options labels Apr 29, 2024
@mukeshpanchal27 mukeshpanchal27 changed the title Update helper.php Fix WordPress.DB.DirectDatabaseQuery.DirectQuery warning for Autoloaded Options Health Check Apr 29, 2024
@Ellusu @mukeshpanchal27
Update includes/site-health/audit-autoloaded-options/helper.php
0079dd9 
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
joemcgill
joemcgill requested changes Apr 29, 2024
View reviewed changes
Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @Ellusu! Left a couple questions/suggestions.

includes/site-health/audit-autoloaded-options/helper.php Outdated Show resolved Hide resolved
includes/site-health/audit-autoloaded-options/helper.php Show resolved Hide resolved
joemcgill
joemcgill approved these changes May 6, 2024
View reviewed changes
Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

Thanks, @Ellusu. Looks good to me.

joemcgill
joemcgill requested changes May 6, 2024
View reviewed changes
Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

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.

@westonruter
Copy link
Member

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.

@westonruter westonruter requested a review from joemcgill May 6, 2024 23:34
@westonruter westonruter mentioned this pull request May 6, 2024
Merged
@westonruter westonruter added this to the performance-lab 3.1.0 milestone May 6, 2024
mukeshpanchal27
mukeshpanchal27 reviewed May 7, 2024
View reviewed changes
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Thanks for the update.

Left some nit-pick. Can we update test_perflab_aao_autoloaded_options_size tests to remove the query?

includes/site-health/audit-autoloaded-options/helper.php Show resolved Hide resolved
includes/site-health/audit-autoloaded-options/helper.php Outdated Show resolved Hide resolved
includes/site-health/audit-autoloaded-options/helper.php Show resolved Hide resolved
joemcgill
joemcgill approved these changes May 8, 2024
View reviewed changes
Copy link
Member

@joemcgill joemcgill left a comment

Choose a reason for hiding this comment

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

Looks good to me.

westonruter
westonruter approved these changes May 8, 2024
View reviewed changes
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

LGTM. Let's merge once @mukeshpanchal27 approves.

westonruter
westonruter reviewed May 13, 2024
View reviewed changes
includes/site-health/audit-autoloaded-options/helper.php
'autoload' => 'true',
),
admin_url( 'site-health.php' )
)
);
$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 );
Copy link
Member

@westonruter westonruter May 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

westonruter
westonruter reviewed May 13, 2024
View reviewed changes
includes/site-health/audit-autoloaded-options/helper.php
Comment on lines +225 to +227
if ( count( $disabled_options_summary ) === 0 ) {
return '';
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

@mukeshpanchal27 mukeshpanchal27 merged commit 3febac9 into WordPress:trunk May 14, 2024
16 checks passed
@westonruter westonruter mentioned this pull request May 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonruter westonruter westonruter approved these changes

@mukeshpanchal27 mukeshpanchal27 mukeshpanchal27 approved these changes

@joemcgill joemcgill joemcgill approved these changes

@manuelRod manuelRod Awaiting requested review from manuelRod

@felixarntz felixarntz Awaiting requested review from felixarntz

Assignees
No one assigned
Labels
no milestone PRs that do not have a defined milestone for release [Type] Bug An existing feature is broken
Projects
None yet
Milestone
performance-lab 3.1.0
Development

Successfully merging this pull request may close these issues.

Plugin Check: Report WordPress.DB.DirectDatabaseQuery.DirectQuery warning for Autoloaded Options Health Check
4 participants
@Ellusu @westonruter @joemcgill @mukeshpanchal27