-
Notifications
You must be signed in to change notification settings - Fork 109
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
Support changing autoload value for largest autoloaded options in Site Health check #1048
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, I have addressed the review feedback. The PR is ready for another round of review. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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>
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 |
I thought #1062 updated the workflows? Why are we still testing 6.3? Let's just fix that here. |
@@ -46,7 +46,7 @@ jobs: | |||
wp: [ 'latest' ] | |||
include: | |||
- php: '7.4' | |||
wp: '6.3' | |||
wp: '6.4' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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. |
Summary
Fixes #889
Checklist
[Focus]
orInfrastructure
label.[Type]
label.no milestone
label.