-
Notifications
You must be signed in to change notification settings - Fork 815
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
Feature Branch: Update the Autoloader #15106
Conversation
This is an automated check which relies on E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-15106 |
6f4ba71
to
48be845
Compare
868863c
to
e2a0e9d
Compare
21007a7
to
ffb56e1
Compare
I rebased on the master branch and force-pushed to resolve some failing unit tests. |
077ff7f
to
da34a8a
Compare
I rebased on the master branch to resolve a conflict. |
This is super exciting! Nice work on this. I ran into a Fatal error while testing this. I am not sure if this is due to the woocommerce plugin missing some files or something else but I wasn't able to replicate this issue. :( I wanted to report it here just in case. What I did was install woo, activate vaultPress, then went to try to activate woocommerce activation failed. I tried again and it worked as expected. The fatal error that I encountered was:
|
That's a weird issue with the Autoloader missing, a search yields the same issue someone had on the forum, which went away after reinstalling WooCommerce. |
I did more testing here. I tried to look at this from a performance prospective. I noticed the more active plugin the site has the slower the site performs. So if you have just 1 plugin installed vs having a number plugin that have auto loader active installed make the page load slower. I think that the call to Another call that is Can we look at improving this performance? Or maybe we can do more proper performance testing to see where we can improve things. |
Thanks for looking into this! I haven't seen much of an increase in load times in my tests; maybe I didn't install enough plugins. We should definitely look into improving the performance! I'll take a look at doing some more comprehensive performance testing and making some changes to improve performance. |
Remove Plugins_Handler::create_map_path_array() and Plugins_Handler::get_active_plugins_paths() because they are not used.
* Autoloader: refactor a few methods in Plugins_Handler Refactor a few methods in Plugins_Handler to make the class easier to test. * Autoloader: add unit tests for Plugins_Handler
Performance profiling showed that the get_plugins() call in Plugins_Handler::get_current_plugin() is the longest running call in the autoloader setup process. We use get_plugins() to obtain the main file for the current plugin. Instead of identifying plugins by their directory and main file, let's identify them with their directory. This will allow us to eliminate the get_plugins() call and improve autoloader performance.
Refactor Plugin_Handler so that units tests will cover the Plugins_Handler::convert_plugins_to_dirs() method. Also fix the Plugins_Handler unit tests.
PHPUnit5.7.27 doesn't support the assertEqualsCanonicalizing() method, so just sort the output arrays before calling assertEquals().
The Autoloader generates the in-memory classmap when the first autoloader package is loaded. When a plugin is updated, the path to a package class could have changed, making the in-memory classmap point to an invalid file location. Reset the autoloader when a plugin with the v2.x autoloader is updated so that the classmap is kept up to date.
The plugin slugs are converted to the directory names in the get_all_active_plugins() method. So remove the convert_plugins_to_dirs() calls from get_multisite_plugins() and get_active_plugins().
ccfc5d3
to
3d801f0
Compare
Rebased. Mostly myself with PHPCS changes. |
Tested with the suite of example plugins. All seems well here. 🚀 |
This PR introduces major changes to the autoloader package, so it should be versioned 2.0.
Why version 2.0?
The previous versions of the Jetpack Autoloader built an in-memory classmap, which mapped each package class to the latest version of the package. It updated this map as each plugin that used the autoloader was loaded. This meant that the autoloader couldn't identify the latest version of a package until all of the plugins have been loaded. To ensure that the latest versions of packages were used, the Autoloader required that plugins never load a package class before all of the plugins had been loaded. This constraint was too limiting, so we decided to try a different approach.
In the new approach introduced in this PR, the first autoloader package that is loaded examines the classmaps for all active plugins and builds the in-memory classmap. This means that after the first plugin that uses the v2.0 autoloader has loaded, the autoloader knows where the latest versions of the packages are located. The plugins no longer need to wait until all the packages have loaded to use the package classes.
Changes proposed in this Pull Request
The plugin build process
Note that the Autoloader package's existing
composer.json
file has an extra value: "class":Automattic\\Jetpack\\Autoloader\\CustomAutoloaderPlugin
. This is a custom installer that hooks a method onto thepost-autoload-dump event
.In
CustomAutoloaderPlugin::postAutoloadDump()
, we generate an error if the defaultvendor
directory is not used. The autoloader will look for files in thevendor
directory, so consuming plugins must use the default directory.When the
post-autoload-dump
event fires,AutoloadGenerator::dump()
is called. This method creates a number of files, including multiple files that are new for v2.0:jetpack_autoload_classmap.php
andjetpack_autoload_filemap.php
are the filenames of the classmap and filemap generated by the autoloader. We've changed the names of these files to make it easy for the autoloader to distinguish between v2.x and v1.x versions of the autoloader.AutoloadGenerator::getAutoloadPackageFile()
method. This method adds a header with a unique namespace to each file. The unique namespace allows the autoloader to load files that are within its own namespace.The autoloader setup process
A consuming plugin loads its autoloader using
require_once . plugin_dir_path( __FILE__ ) . '/vendor/autoload_packages.php';
.The
autoload_packages.php
file (which was created using the package'sautoload.php
file) has a unique namespace. It loads theautoload_functions.php
file and calls theset_up_autoloader()
function.The
autoload_functions.php
file (which was created using the package'sfunctions.php
file) contains a number of functions. Note the global variables in this file,$jetpack_autoloader_latest_version
,jetpack_packages_classmap
, etc.. These globals are used by the autoloaders to determine what needs to be done.set_up_autoloader()
is called first. It does the following:Plugins_Handler::should_autoloader_reset()
. This resets the autoloader global variables when an activating plugin is detected. (More info about activating plugins is in step 9).Autoloader_Handler::find_latest_version()
. This method traverses through the active plugin folders, looking for autoloaders. It chooses the autoloader with the latest version, and that autoloader does the work.enqueue_files
andAutoloader_Handler::update_autoloader_chain
.enqueue_files()
sets up the in-memory classmap and filemap for the site. TheClasses_Handler
andFiles_Handler
classes traverse through the active plugins and look at the classmap and filemap files for each plugin. They update the locations of the latest version of each package class and file. Each of these classes uses thePlugins_Handler
andVersion_Selector
to help with their work.The
Version_Selector
class is used to compare package versions.JETPACK_AUTOLOAD_DEV
constant is set to true, the first development version that is found is used. Development versions are versions that begin withdev-
or9999999-dev
.JETPACK_AUTOLOAD_DEV
is not set to true, development versions are only used if no other version is available.version_compare
function.The
Plugins_Handler
class is used to collect information about the plugins. ThePlugins_Handler::get_all_active_plugins()
method is used to retrieve a list of all active and activating plugins.Activating plugins require some special considerations. If a plugin is activated on the wp-admin's Plugins page, the page request will contain an
action=activate
query. We look for this in thePlugins_Handler::get_plugins_activating_via_request()
method.If a plugin is activated using a different method, for example using WP CLI, the autoloader will not be aware of the plugin until it loads its
autoload_packages.php
file. Then the setup process begins again at step 1, and thePlugins::Handler::should_autoloader_reset()
method detects that the current plugin is unknown. This causes a reset of the autoloader global variables. We maintain a list of activating plugins in the$jetpack_autoloader_activating_plugins
variable.The autoloader chain is a list of functions that are called in order when a class is requested. The
Autoloader_Handler::update_autoloader_chain()
method maintains the order of the autoloader functions. It does two things:Version selection when both a v1.x autoloader and a v2.0 autoloader are installed
Version selection is a bit complicated when a v1.x autoloader and a v2.0 autoloader are installed on a site. The v2.0 autoloader cannot access the map files created by the v1.x autoloader, so the v2.0 autoloader doesn’t know anything about packages that have been installed by a plugin using a v1.x autoloader.
The v2.0 autoloader will select the latest version of a package from the package versions it can access.
The v2.0 autoloader places its autoloader function before the v1.x autoloader function in the PHP autoloader chain. This means that when the v2.0 autoloader is aware of a package, the v2.0 autoloader will select which version of a package is loaded.
If a package is used only by plugins with v1.x autoloaders, the v2.0 autoloader won’t know about that package. That package will be loaded by the v1.x autoloader function.
Here is an example site environment with both a v1.7 autoloader and a v2.0 autoloader installed:
Is this a new feature or does it add/remove features to an existing part of Jetpack?
Testing instructions:
This PR introduces major changes to the autoloader, so thorough testing is required! If you can think of a test environment that isn't covered by the test instructions, please test it.
Build the autoloader files
composer install
and verify that the autoloader files have been generated:Test in a realistic environment
Test with test plugins
JETPACK_AUTOLOAD_DEV
constant set to both true and false and confirm that development versions are selected when the constant is set.Test with an Updating Plugin
It’s challenging to test a plugin update. Here are instructions for how I test it. Please let me know if have any other ideas!
In these tests, we’ll simulate a file being moved in the updated version of Jetpack.
Cause a fatal error during a plugin update
jetpack-dev
folder tojetpack
.upgrader_process_complete
hook fires. Note that the snippet uses thePre_Connection_JITM
class, which is not loaded when Jetpack is connected.wp-content/plugins/jetpack.zip
.Create the source zip file for the Jetpack update using a command like
zip -r jetpack.zip jetpack
. The file name and location must match the source zip file in theupgrader_package_options
hook callback. If you use the snippet in step 5, the source file path must bewp-content/plugins/jetpack.zip
.In
jetpack/vendor/composer/jetpack_autoload_classmap.php
, change the file name for thePre_Connection_JITM
class.jetpack.zip
file. This change allows us to simulate the file being moved.jetpack/vendor/automattic/jetpack-jitm/src/class-pre-connection-jitm.php
to match the classmap file name if you'd like. This file won't be requested until after the update, so it doesn't matter.Comment out the
add_filter( ‘upgrader_post_install’, …)
call here.Navigate to wp-admin -> Plugins. An update should be available for Jetpack. Click
update now
.The update should fail and a fatal error should be generated in the debug log because the required file for
Pre_Connection_JITM
couldn't be found. This fatal occurred because the in-memory classmap was not updated.Test the fix
Using the same snippets as above, test without removing the the
add_filter( ‘upgrader_post_install’, …)
call.In
jetpack/vendor/composer/jetpack_autoload_classmap.php
, change the file name for thePre_Connection_JITM
class.Navigate to wp-admin -> Plugins. Reload the page if you're already there. An update should be available for Jetpack. Click
update now
.The update should be successful and no errors should be generated.
Proposed changelog entry for your changes: