Fix/conflicting libraries loaded case

[enejb]

This PR is a proposal for how we could load libraries (classes) that are currently included in jetpack but could also be in other plugins and potentially avoid plugin conflicts.

This is more of a proof of concept then the end result. There is more work to be done on the autoloader part. The part of the code that would be required by all the different plugins.

This PR tries to demonstrate:

1. That it doesn't matter what plugin gets loaded in into WP first. Since we would only load the classes when they are required via PHP classes autoloading. We could also just load all the classes if a particular version of PHP is not supported.
2. Only the latest version of the particular class gets loaded.
3. The only requirement for this to work is that the classes are called after all the plugins have loaded. Things would still work but we can't guarantee that the latest version would get loaded any more. I think this is a WP best practice anyways.

Things to notice in this PR.

1. Calling the Jetpack_Constants::log_constant( 'JETPACK__VERSION' ); works because we are loading the latest version of the library.
2. We don't load the broken Jetpack class because that is an older version of the library.

Some problems that don't quite have a good solution.
What if we need to update the code of the autoloader. (the part of the code that gets loaded first)

Should we be versioning that part as well...
Right now the autoloader is pretty simple. It doesn't care about any sort of class dependencies.
I am not sure it really needs to.

Registering all the different classes could be done by some sort of build script.

Changes proposed in this Pull Request:

... This is a draft PR do not merge.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

This is a new concept.

Testing instructions:

Load the site in the docker WordPress development environment.
Notice that everything still works as expected.

Proposed changelog entry for your changes:

...

[gravityrail]

I have requested a few changes, but what follows is my feedback on the overall approach:

• I do like the simplicity of doing a version check. It feels easy to understand what is going on.
• the limitation that you can't use any classes until after plugins_loaded is major, as it would often require implementing plugins to refactor themselves in some ways, but perhaps one we can live with if we don't find another way to do this.
• I am not sure that the assumption that we should always load the latest version of a class is necessarily valid. Perhaps it's enough to spot plugins with conflicting dependency versions and prompt users to update one, or disable one.
• enqueuing libraries with embedded paths goes against the grain of providing path independence for libraries. For example, one of the goals of my filter-based approach is that a major host, e.g. WPCOM, could filter the autoload mechanism entirely and point it at a vendor directory which embeds all of Jetpack's libraries in one place, making our opcache etc. more efficient and reducing the number of files in SVN. Since your code hard-codes paths relative to the plugin root, we don't get that benefit.

[gravityrail]

Why not do the version comparison here and just overwrite the existing entry if the new entry has a higher version number? That way, you don't have to do any version comparisons at autoload time, since you never plan on loading older versions anyway.

[gravityrail]

As per comment above, this whole function could just be:

global $jetpack_libraries;
if ( isset( $jetpack_libraries[ $class_name ] ) ) { 
  if ( ! did_action( 'plugins_loaded' ) ) { 
    // do warning 
  }
  require_once $jetpack_libraries[ $class_name ]['path'];
}

Note that in this example, I nested the plugins_loaded check inside the test which determines if our class is registered, since we don't want to perform that check for ANY missing class (some may be from other plugins, etc, and loading those before plugins_loaded may be totally fine)

[enejb]

the limitation that you can't use any classes until after plugins_loaded is major, as it would often require implementing plugins to refactor themselves in some ways, but perhaps one we can live with if we don't find another way to do this.

There is no other way in WordPress to know what version of a plugin a site is running for sure during runtime. Since the plugins get loaded in the order they are activated or based on their file name if they live in the mu folder.

Doing any sort of build process on a sites is hard to do across WordPress sites. Since a site with wrong permissions would have trouble placing a writing to a file.

I am not sure that the assumption that we should always load the latest version of a class is necessarily valid. Perhaps it's enough to spot plugins with conflicting dependency versions and prompt users to update one, or disable one.

I think it would be great if we didn't have to worry about conflicting dependencies as much as possible. Assuring that we always load the latest version of a class is one way to go about it.
There is always ways around this. If a class for example intoruduces many new methods for example we could call the class V2 and extend the previous class so that the methods get inherited.
class Jetpack_V2 extends Jetpack {} ideally we would reserve that sort of extension for things that make logical sense vs for some sort of compatibility purposes.

enqueuing libraries with embedded paths goes against the grain of providing path independence for libraries. For example, one of the goals of my filter-based approach is that a major host, e.g. WPCOM, could filter the autoload mechanism entirely and point it at a vendor directory which embeds all of Jetpack's libraries in one place, making our opcache etc. more efficient and reducing the number of files in SVN. Since your code hard-codes paths relative to the plugin root, we don't get that benefit.

I think there is a few different ways that this could be address. We could make sure that on WPCOM or have the expected $jetpack_libraries global variable defiend in a file that gets loaded last or even first. using 0-jetpack-lib.php for example

We could use something like

$jetpack_libraries['Jetpack']['path'] = '/specific-wpcom-path/class.jetpack.php'
$jetpack_libraries['Jetpack']['version'] ='1000'; // evergreen 

make sure that add some sort of filtering to the autoloader.

[tyxla]

Or it could be a separate package on its own 😉

[enejb]

great idea! This was more of a proof of concert.

[tyxla]

Why would we want to load the libraries after the plugins have loaded?

[enejb]

That is the only way to know for sure that plugins are all loaded.
If we don't wait till all the plugins load how can we be sure that we are loading the latest version?

[tyxla]

Thanks for elaborating, that makes sense to me.

[enejb]

That is the only way to know for sure that plugins are all loaded.

…

On Mon., May 13, 2019, 9:51 a.m. Marin Atanasov, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In jetpack.php <#12295 (comment)>: > + $jetpack_libraries = array(); + } + function jetpack_enqueue_library( $class_name, $version, $path ) { + global $jetpack_libraries; + if ( ! isset( $jetpack_libraries[ $class_name ] ) + || version_compare( $jetpack_libraries[ $class_name ] ['version'], $version, '<' ) + ) { + $jetpack_libraries[ $class_name ] = array( 'version' => $version, 'path' => $path ); + } + } + + // add the autoloader + spl_autoload_register( function ( $class_name ) { + global $jetpack_libraries; + if ( isset( $jetpack_libraries[ $class_name ] ) ) { + if ( ! did_action( 'plugins_loaded' ) ) { Why would we want to load the libraries after the plugins have loaded? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#12295?email_source=notifications&email_token=AAA4C7YT2QCVKXTWXTQBT63PVGMARA5CNFSM4HLNW6XKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOBYOT2XI#pullrequestreview-236797277>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA4C74BERSOKSDWMTEWKYLPVGMARANCNFSM4HLNW6XA> .

[tyxla]

That is the only way to know for sure that plugins are all loaded.

I see, but I don't follow what's the reason for us to wait for all plugins to be loaded.

[gravityrail]

Closing, superseded by #12447