Automattic / jetpack

Security, performance, marketing, and design tools — Jetpack is made by WordPress experts to make WP sites safer and faster, and help you grow your traffic.
https://jetpack.com/
Other
1.59k stars 799 forks source link

Autoloader: A potential issue with different major versions of dependencies #17221

Open anomiex opened 4 years ago

anomiex commented 4 years ago

I looked into the Jetpack autoloader when doing some reviews. It seems like an interesting approach to the problem of trying to drop plugins, each with their own vendor directory full of dependencies, into an existing installation without requiring a top level composer install to properly collect all the dependencies into one vendor directory.

For example, if a plugin uses "some/library" at version 1.0.3, and another plugin uses "some/library" at version 1.0.7, our autoloader will pick the 1.0.7 version.

But consider if a third plugin brings in "some/library" at version 2.0.0, which (as indicated by the semver version number) has breaking changes with respect to the 1.x series. Our autoloader would pick 2.0.0, which might break one of the first two plugins if they use something from the library that was removed or changed.

Remember that even if we avoid this in our own libraries by releasing a "some/libraryV2" instead of a 2.0.0 version, the autoloader may also be used to bring in other libraries that likely don't do that sort of thing.

Composer itself handles this sort of situation by raising an error when you try to composer install or composer update, complaining that no version satisfies all the dependencies. The third plugin might even avoid the problem by depending on "~1.0.7 || ~2.0.0" (and doing library feature detection somehow in-code).

At the least, we might log a warning of some sort if we have to choose between different major versions. Ideally we might collect the constraints applying to each package in addition to the package's version in the generated jetpack_autoload_*.php, and use that when autoloading to select the best version of the package (or raise an error if no version satisfies all constraints) instead of just picking the highest.

jeherve commented 4 years ago

cc @kbrown9 for your thoughts on this.

kbrown9 commented 4 years ago

For example, if a plugin uses "some/library" at version 1.0.3, and another plugin uses "some/library" at version 1.0.7, our autoloader will pick the 1.0.7 version.

But consider if a third plugin brings in "some/library" at version 2.0.0, which (as indicated by the semver version number) has breaking changes with respect to the 1.x series. Our autoloader would pick 2.0.0, which might break one of the first two plugins if they use something from the library that was removed or changed.

Yes, this is a major limitation of Jetpack's autoloader.

In addition, Jetpack's autoloader could lose control of which package version is loaded when another plugin uses a package but does not use Jetpack's autoloader. For example, if a fourth plugin installs "some/library" version 1.0.2, but does not use Jetpack's autoloader, version 1.0.2 files from "some/library" could be loaded if the fourth plugin is loaded before the others.

At the least, we might log a warning of some sort if we have to choose between different major versions. Ideally we might collect the constraints applying to each package in addition to the package's version in the generated jetpack_autoload_*.php, and use that when autoloading to select the best version of the package (or raise an error if no version satisfies all constraints) instead of just picking the highest.

Collecting version constraints, attempting to resolve them, and displaying a message when version constraints cannot be resolved are long-term goals for the Jetpack autoloader. As far as I know, no one is working on it yet. If you're interested in that, feel free to work on it!

anomiex commented 4 years ago

If you're interested in that, feel free to work on it!

I might just do that at some point. Some notes/brainstorming: