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.

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

Improve autoloaded options check by highlighting largest autoloaded options #353

Conversation

rytisder
Copy link
Contributor

@rytisder rytisder commented Jun 2, 2022

Summary

Example:

Screenshot 2022-06-04 at 14 59 55

Fixes #350

Relevant technical choices

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@rytisder rytisder requested a review from manuelRod as a code owner June 2, 2022 15:11
@felixarntz felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature [Focus] Site Health LEGACY LEGACY – Issues related to the Site Health focus area [Module] Audit Autoloaded Options Issues for the Audit Autoloaded Options Health Check module labels Jun 2, 2022
@felixarntz felixarntz added this to the 1.2.0 milestone Jun 2, 2022
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@rytisder Thank you for the pull request!

This looks great. I have a few bits of small feedback, most importantly it would be great to make the threshold value filterable so that external code such as plugins can change it.

modules/site-health/audit-autoloaded-options/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-autoloaded-options/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-autoloaded-options/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-autoloaded-options/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-autoloaded-options/load.php Outdated Show resolved Hide resolved
@rytisder rytisder force-pushed the feature/implement-autoload-explanation-table branch from 93cdfff to 85ea43d Compare June 2, 2022 15:47
@rytisder rytisder force-pushed the feature/implement-autoload-explanation-table branch from 85ea43d to f73bb65 Compare June 4, 2022 11:56
@rytisder rytisder force-pushed the feature/implement-autoload-explanation-table branch from f73bb65 to 9d2539d Compare June 4, 2022 11:58
@mukeshpanchal27
Copy link
Member

@rytisder Thanks for fixing it so quickly.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@rytisder Thank for the updates, looks great! I just left a few more comments, almost all of them small details. Functionally this is already good, but it would be great if you could address the remaining feedback.

FYI no need to force-push, we typically just work with full commit history. So you can just add commits as you go.

modules/site-health/audit-autoloaded-options/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-autoloaded-options/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-autoloaded-options/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-autoloaded-options/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-autoloaded-options/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-autoloaded-options/load.php Outdated Show resolved Hide resolved
modules/site-health/audit-autoloaded-options/load.php Outdated Show resolved Hide resolved
@manuelRod
Copy link
Contributor

manuelRod commented Jun 7, 2022

Hello @rytisder thanks for the PR, it really makes this site health check more self-explanatory and complete :)
I left a small comment, and I would add maybe a unit test to covering perflab_aao_autoloaded_options()
As for the rest, looks good to me.
Also, don't forget to add you as code owner for this module.

@mukeshpanchal27
Copy link
Member

@rytisder I will add some additional feedback on the PR, Below is the description for changes that I request.

  1. You have used the static wp_options table name that needs to update.
  2. Then </tr> missing on code https://github.com/rytisder/performance/blob/feature/implement-autoload-explanation-table/modules/site-health/audit-autoloaded-options/load.php#L155 once it will be added it automatically use WordPress grid design like the attached image.

Table design
Note: To get this site health report in this image I changed some threshold values.

@felixarntz In WordPress site health report it doesn't use a number column to identify its column number in any of their site health report in the info tab( WordPress, Directories and Sizes, Server, etc..) so for consistency do we need to remove it?

@felixarntz
Copy link
Member

@mukeshpanchal27 Thank you, great additional points!

In WordPress site health report it doesn't use a number column to identify its column number in any of their site health report in the info tab( WordPress, Directories and Sizes, Server, etc..) so for consistency do we need to remove it?

Agreed, let's remove the first # column.

@rytisder Would be great if you could follow up on the additional feedback above! :)

@felixarntz felixarntz modified the milestones: 1.2.0, 1.3.0 Jun 14, 2022
@rytisder
Copy link
Contributor Author

rytisder commented Jun 16, 2022

@felixarntz, all suggestions mentioned above are pushed! 🙂

@felixarntz
Copy link
Member

This isn't ready yet due to the outstanding iterations requested, so I'll remove the 1.3.0 milestone from here. Once this gets picked up and continued towards merge, we can add a milestone again.

@rytisder Will you be able to continue on the iterations requested above? If not, would you be open to another contributor taking over and continuing your work?

@felixarntz felixarntz removed this from the 1.3.0 milestone Jul 14, 2022
@mukeshpanchal27
Copy link
Member

Reminder:

@rytisder Will you be able to continue on the iterations requested above? If not, would you be open to another contributor taking over and continuing your work?

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.

@rytisder Thanks for the update. PR looks good now. Can you please address feedback and merge the latest changes from trunk into your branch.

modules/site-health/audit-autoloaded-options/load.php Outdated Show resolved Hide resolved
@mukeshpanchal27 mukeshpanchal27 added this to the 1.5.0 milestone Sep 12, 2022
@mukeshpanchal27
Copy link
Member

Set milestone to 1.5. Feel free to update change it if this is not ready.

@mukeshpanchal27 mukeshpanchal27 added this to Backlog in [Focus] Site Health LEGACY via automation Sep 12, 2022
@mukeshpanchal27 mukeshpanchal27 moved this from Backlog to In progress in [Focus] Site Health LEGACY Sep 12, 2022
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

@mukeshpanchal27 Since the remaining 2 tweaks from https://github.com/WordPress/performance/pull/353/files#r967984960 and #353 (review) are so tiny, let's merge this and then addresse those things in a follow up PR right after, if you're okay with that.

@felixarntz felixarntz changed the title Improve wp_options autoload audit Improve autoloaded options check by highlighting largest autoloaded options Sep 12, 2022
@felixarntz felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature and removed [Type] Enhancement A suggestion for improvement of an existing feature labels Sep 12, 2022
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.

Thank you very much, @rytisder! For the remaining work on this PR, we will create a separate PR.

@mukeshpanchal27
Copy link
Member

@tillkruss Can you please review so we can merge this and add new follow-up PR to address remaining points.

  • Document is missing for the filter perflab_aao_autoloaded_options_table_threshold

@felixarntz
Copy link
Member

@mukeshpanchal27

Can you please review so we can merge this and add new follow-up PR to address remaining points.

  • Document is missing for the filter perflab_aao_autoloaded_options_table_threshold

Looks like we can actually edit the branch here directly. In that case, we could also adjust it directly in here. But either way works, would be great if you could complete the work here. 🙌

Co-authored-by: Peter Wilson <519727+peterwilsoncc@users.noreply.github.com>
@mukeshpanchal27 mukeshpanchal27 merged commit 43369da into WordPress:trunk Sep 14, 2022
[Focus] Site Health LEGACY automation moved this from In progress to Done Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Site Health LEGACY LEGACY – Issues related to the Site Health focus area [Module] Audit Autoloaded Options Issues for the Audit Autoloaded Options Health Check module [Type] Enhancement A suggestion for improvement of an existing feature
Development

Successfully merging this pull request may close these issues.

Improve audit wp_options results with Autoload top list
7 participants