-
Notifications
You must be signed in to change notification settings - Fork 101
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
Add support for autoloading enhancements in WordPress 6.6 trunk to autoloaded options Site Health check #1112
Conversation
Move `crate-ci/typos` config to `.github/typos.toml`
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. |
Once this fix is merged into |
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.
Overall look good to me. Left some nit-pick
tests/includes/site-health/audit-autoloaded-options/audit-autoloaded-options-test.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
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.
Just one suggestion but not a blocker.
if ( function_exists( 'wp_autoload_values_to_autoload' ) ) { | ||
$autoload_values = wp_autoload_values_to_autoload(); | ||
} else { | ||
$autoload_values = array( 'yes' ); | ||
} |
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.
Rather than repeating this several times, how about consolidating it to our own perflab_aao_get_autoload_values()
helper?
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.
@joemcgill Great idea, done in 0316586
Summary
This PR fixes the test failures seen in https://github.com/WordPress/performance/actions/runs/8547642712/job/23420147133?pr=1111, which are unrelated to the PR they're reported on. The next time we'll make a PHP change against
trunk
, we should see the same failures too.Relevant technical choices
The failures happen due to https://core.trac.wordpress.org/changeset/57920 which makes an intentional change regarding large autoloaded options, to no longer autoload them unless
$autoload = true
is explicitly provided. Additionally, there are now more database values thanyes
that should trigger autoloading. All of this is expected. Plugins that perform low-level optimizations around autoloaded options are likely to run into similar failures - all of which is straightforward to fix though. This is why the underlying Trac ticket is annotated withneeds-dev-note
.So we need to do two things:
wp_autoload_values_to_autoload()
that exists for this purpose.