Support changing autoload value for largest autoloaded options in Site Health check by mukeshpanchal27 · Pull Request #1048 · WordPress/performance · GitHub

Summary

Fixes #889

Checklist

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

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: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>

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

@westonruter, I have addressed the review feedback. The PR is ready for another round of review. Thank you!

@mukeshpanchal27 I don't have much to add to the existing feedback. Most importantly from my perspective though, we either need to do the AJAX + JS approach the right way, or alternatively go with the much simpler alternative of using a page reload and a regular WP Admin callback via "action URL".

…mance into fix/889-update-autoload

Thanks, @felixarntz, @westonruter, and @swissspidy for the feedback. Per the discussion in #1048 (comment), I also believe that the action URL approach is the right choice. Additionally, I've added an admin notice when the option is updated.

@mukeshpanchal27 This looks very close now. I only have one broader suggestion on how to handle the option size to display to the user more dynamically.

Thank you, @felixarntz and @westonruter, for your feedback. The PR is now ready for the final review and merge.

@mukeshpanchal27 Two small last things, otherwise this is good to go IMO. Great stuff!

Will there be tests added for this?

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

Will there be tests added for this?

This would be a good candidate for an e2e test. Makes me wonder why we don't have any 🤔

Will there be tests added for this?

This would be a good candidate for an e2e test. Makes me wonder why we don't have any 🤔

We don't currently have e2e tests set up in the PL repository. I'm exploring how we can add tests for these changes. 😅

I saw that, hence the wondering why :)

here, some php unit tests should be possible

@swissspidy @westonruter I've added unit tests. It appears that the unit tests are failing in WP 6.3 due to the introduction of wp_set_option_autoload in 6.4. We'll need to update the workflow accordingly.

I thought #1062 updated the workflows? Why are we still testing 6.3? Let's just fix that here.

@mukeshpanchal27 @swissspidy Strange that this is not failing in trunk 🤔

But anyway, great to fix here.

@westonruter Which kinds of tests are you envisioning? In terms of PHPUnit, maybe a test of the "action function" to update the autoload values would be useful. I'm not sure it's worth adding tests for the Site Health UI bits as that's relatively trivial and IMO more difficult to test than it provides value.

mukeshpanchal27 added 2 commits March 12, 2024 15:02

POC

6f2901b

Update since

1d2bf76

mukeshpanchal27 added [Type] Enhancement A suggestion for improvement of an existing feature [Module] Audit Autoloaded Options labels Mar 12, 2024

mukeshpanchal27 requested review from felixarntz, joemcgill, swissspidy and westonruter March 12, 2024 09:37

mukeshpanchal27 self-assigned this Mar 12, 2024

mukeshpanchal27 added the no milestone PRs that do not have a defined milestone for release label Mar 12, 2024

mukeshpanchal27 marked this pull request as ready for review March 12, 2024 09:41

mukeshpanchal27 requested a review from manuelRod as a code owner March 12, 2024 09:41

westonruter requested changes Mar 12, 2024

View reviewed changes

mukeshpanchal27 and others added 2 commits March 13, 2024 11:42

Address review feedbacks

5747c0e

Merge branch 'trunk' into fix/889-update-autoload

461968b

mukeshpanchal27 requested a review from westonruter March 13, 2024 06:21

Ensure function returns string

297b368

westonruter requested changes Mar 13, 2024

View reviewed changes

swissspidy reviewed Mar 13, 2024

View reviewed changes

Comment thread modules/database/audit-autoloaded-options/hooks.php Outdated

felixarntz requested changes Mar 13, 2024

View reviewed changes

Comment thread modules/database/audit-autoloaded-options/helper.php Outdated

Comment thread modules/database/audit-autoloaded-options/hooks.php Outdated

Comment thread modules/database/audit-autoloaded-options/update-autoload.js Outdated

mukeshpanchal27 added 4 commits March 14, 2024 09:38

Resolve conflict

d7da06c

Merge branch 'fix/889-update-autoload' of github.com:WordPress/perfor…

817c6ae

…mance into fix/889-update-autoload

Use button instead a tag

e1a1afc

Use action URL approach

781871b

mukeshpanchal27 requested review from swissspidy and westonruter March 14, 2024 06:13

westonruter reviewed Mar 15, 2024

View reviewed changes

Comment thread includes/site-health/audit-autoloaded-options/hooks.php Outdated

Change update option logic

93c6a42

mukeshpanchal27 requested a review from westonruter March 18, 2024 04:28

felixarntz reviewed Mar 18, 2024

View reviewed changes

Update approach for modified option

4e857a4

mukeshpanchal27 requested a review from felixarntz March 20, 2024 08:59

felixarntz approved these changes Mar 20, 2024

View reviewed changes

Comment thread includes/site-health/audit-autoloaded-options/hooks.php Outdated

Comment thread includes/site-health/audit-autoloaded-options/helper.php Outdated

felixarntz added this to the performance-lab 3.0.0 milestone Mar 20, 2024

felixarntz removed the no milestone PRs that do not have a defined milestone for release label Mar 20, 2024

felixarntz changed the title Support changing autoload value for largest autoloaded options Support changing autoload value for largest autoloaded options in Site Health check Mar 20, 2024

Apply suggestions from code review

cfce655

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

mukeshpanchal27 and others added 2 commits March 21, 2024 17:48

Add unit test

19b2b4f

Merge branch 'trunk' into fix/889-update-autoload

e3da82e

mukeshpanchal27 and others added 2 commits March 21, 2024 20:46

Remove WP 6.3 from workflow

7bb08d3

Merge branch 'trunk' into fix/889-update-autoload

c5f772e

felixarntz reviewed Mar 21, 2024

View reviewed changes

felixarntz approved these changes Mar 21, 2024

View reviewed changes

westonruter approved these changes Mar 21, 2024

View reviewed changes

westonruter merged commit 5e2bdb5 into trunk Mar 21, 2024

westonruter deleted the fix/889-update-autoload branch March 21, 2024 18:57

felixarntz mentioned this pull request Mar 26, 2024

Improve UX/UI of enabling performance features via standalone plugins #1091

Merged

mukeshpanchal27 mentioned this pull request Jul 10, 2024

Autoloaded Options Health Check: Disabled options reappear in Site Health after external update #1340

Closed

                       include:
                         - php: '7.4'
-                          wp: '6.3'
+                          wp: '6.4'

Support changing autoload value for largest autoloaded options in Site Health check by mukeshpanchal27 · Pull Request #1048 · WordPress/performance · GitHub

Uh oh!

Support changing autoload value for largest autoloaded options in Site Health check - #1048

Support changing autoload value for largest autoloaded options in Site Health check#1048westonruter merged 20 commits intotrunkWordPress/performance:trunkfrom fix/889-update-autoloadWordPress/performance:fix/889-update-autoloadCopy head branch name to clipboard

Conversation

mukeshpanchal27 commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

github-actions Bot commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mukeshpanchal27 commented Mar 13, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mukeshpanchal27 commented Mar 14, 2024

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mukeshpanchal27 commented Mar 20, 2024

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

westonruter commented Mar 20, 2024

Uh oh!

swissspidy commented Mar 21, 2024

Uh oh!

mukeshpanchal27 commented Mar 21, 2024

Uh oh!

swissspidy commented Mar 21, 2024

Uh oh!

mukeshpanchal27 commented Mar 21, 2024

Uh oh!

swissspidy commented Mar 21, 2024

Uh oh!

felixarntz Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

felixarntz commented Mar 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Support changing autoload value for largest autoloaded options in Site Health check#1048
westonruter merged 20 commits into
trunkfrom
fix/889-update-autoload

mukeshpanchal27 commented Mar 12, 2024 •
edited
Loading

github-actions Bot commented Mar 12, 2024 •
edited
Loading