-
Notifications
You must be signed in to change notification settings - Fork 808
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
autoloader: Preload classes on plugin upgrade to avoid mid-upgrade fatals #23039
autoloader: Preload classes on plugin upgrade to avoid mid-upgrade fatals #23039
Conversation
…tals It's possible that the new version of the plugin will move some files around that some later hook will try to load, causing a fatal as the autoloader will still be looking in the old path. To avoid this, have the Jetpack autoloader detect when an upgrade is happening and pre-load all the classes in the plugin. We do the same for plugin removal, for similar reasons. Note this doesn't work for classes loaded via PSR-0 or PSR-4. But that shouldn't matter for us since we always generate an optimized (classmap) autoloader for releases of our plugins. Also of note is that this won't work if you're manually installing a specific version of the plugin as `wp plugin install jetpack --version=10.5`, as wp-cli does that as an "install" rather than an "upgrade" operation.
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Beta plugin:
Jetpack plugin:
Backup plugin:
Vaultpress plugin:
Boost plugin:
|
…-everything-on-plugin-update
…-everything-on-plugin-update
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.
Hi Brad,
I tested using your instructions and everything works as expected as far as updating from JP 10.4 to JP 10.5 is concerned.
However, after successfully updating to 10.5 I removed the mu-plugin and tried updating to JP 10.6
This produced the following fatal:
PHP Fatal error: require(): Failed opening required '(redacted)/public/wp-content/plugins/jetpack/vendor/automattic/jetpack-composer-plugin/src/class-plugin.php' (include_path='.:/opt/sp/php7.4/lib/php') in (redacted)/public/wp-content/plugins/jetpack-beta-dev/vendor/jetpack-autoloader/class-php-autoloader.php on line 90
Co-authored-by: Foteini Giannaropoulou <giannaropoulou.foteini@gmail.com>
…-everything-on-plugin-update
Good find! When we added the composer-plugin we didn't bother including it in the plugin bundle, but the autoloader still "knows" about its classes. Normally that doesn't matter since we don't actually load those classes, but this PR would try loading them. Should be fixed now. |
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.
This approach looks good to me. As expected, commit 0981b47 did resolve the issue with attempting to load files that aren’t in the release in my tests.
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.
👍 Thanks for working on this! I repeated the testing instructions, plus attempting to update to 10.6 and 10.7 and succeeded without any errors.
…-everything-on-plugin-update
Needs re-approval after version bump |
…-everything-on-plugin-update
After several problems with plugin upgrades failing since #23039, we need a more robust CI test for plugin upgrades failing. For each plugin built, this will test upgrading via the web UI and wp-cli, from the latest stable (if any) and from the built version to a hypothetical future version.
After several problems with plugin upgrades failing since #23039, we need a more robust CI test for plugin upgrades failing. For each plugin built, this will test upgrading via the web UI and wp-cli, from the latest stable (if any), from master, and from the built version to a hypothetical future version.
Changes proposed in this Pull Request:
It's possible that the new version of the plugin will move some files
around that some later hook will try to load, causing a fatal as the
autoloader will still be looking in the old path. To avoid this, have
the Jetpack autoloader detect when an upgrade is happening and pre-load
all the classes in the plugin.
We do the same for plugin removal, for similar reasons.
Note this doesn't work for classes loaded via PSR-0 or PSR-4. But that
shouldn't matter for us since we always generate an optimized (classmap)
autoloader for releases of our plugins.
Also of note is that this won't work if you're manually installing a
specific version of the plugin as
wp plugin install jetpack --version=10.5
,as wp-cli does that as an "install" rather than an "upgrade" operation.
Jetpack product discussion
p9dueE-4c4-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions:
wp-content/plugins/*-dev
, just to be sure.wp plugin upgrade
should both want to upgrade Jetpack to 10.5. Doing so should give a mid-upgrade fatal as described in p9dueE-4c4-p2.wp shell <<<'spl_autoload_functions()[0];'
should indicate that it's usingAutomattic\Jetpack\Autoloader\jp567fa3f555de8fd218dfdc1688bb97b5_betaⓥ3_1_1_alpha\PHP_Autoloader
.wp plugin upgrade
should succeed without a mid-upgrade fatal.