Facilitate embedding Speculative Loading in other plugins/themes

[westonruter]

This ports over the demonstrated in the plugin bootstrap file of the woocommerce/action-scheduler plugin. It facilitates multiple copies of the plugin to be installed on a site:

• As a standalone plugin
• Embedded in another plugin
• Embedded in a theme

In each case, the logic in this PR ensures that only one copy is loaded. If it is embedded in multiple plugins, then the version of Speculation Rules which has the highest version is chosen. If it was not embedded in a plugin and isn't active as a standalone plugin but is embedded in a theme instead, then that is the copy which will load.

In contrast with the action scheduler plugin, I decided that if a copy of the plugin is installed in the plugins directory, then it should win out over other copies. This is because there is a Settings link in the plugin row actions, and when something is misbehaving with the plugin it would be confusing for them to report an issue for the plugin which is active but isn't actually loaded.

See #1081.

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: joemcgill <joemcgill@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[westonruter]

Note: Whatever ends up getting approved and merged here I intend to also implement in Optimization Detective and Embed Optimizer among other plugins to facilitate embedding so that we can increase the reach of the plugins.

[westonruter]

With whitespace ignored, note the remaining lines in this file are largely unchanged.

[joemcgill]

This is a clever way of handling possible plugin conflicts and looks like it would work, but encapsulating all of these function and global definitions inside anonymously executed static functions like this is not super intuitive.

A few minor concerns with this approach:

1. The plugin version number needs to be updated as part of the args of the closure, rather than a constant or a variable this is easy to identify
2. The first version of the plugin to define the bootstrap callback and register hooks will always be used, which is probably fine, but is a future-compat concern if for some reason we would want to update the bootstrap logic.
3. Storing the callback that will be executed by call_user_func in a global creates the opportunity for tampering so any code that updates that global would be able to execute arbitrary code.

I don't have an immediately better idea, but wonder if we could conditionally load an autoloader that we bundle in each plugin or conditionally load a bootstrap library that each plugin would register itself to so that we could use OOP to keep things like version number, registration callback, etc. encapsulated inside plugin classes, rather than global variables.

[westonruter]

@joemcgill

encapsulating all of these function and global definitions inside anonymously executed static functions like this is not super intuitive

Yes, although this is done intentionally in order for there to be no collisions with other potential copies of the same plugin.

1. The plugin version number needs to be updated as part of the args of the closure, rather than a constant or a variable this is easy to identify

A constant cannot be used since it would conflict with other copies. A variable at the top of the closure could be used instead. I was intending for that closure to be some boilerplate code which could be copied among plugins, so that's why I went with the version being passed as an argument to the closure.

2. The first version of the plugin to define the bootstrap callback and register hooks will always be used, which is probably fine, but is a future-compat concern if for some reason we would want to update the bootstrap logic.

That's right.

3. Storing the callback that will be executed by call_user_func in a global creates the opportunity for tampering so any code that updates that global would be able to execute arbitrary code.

If some other plugin is manipulating the $plsr_pending_plugin_info global, yes, then this could allow another plugin to override the load callback. But if another plugin is active which is doing this, then essentially a site is already executing arbitrary code. So I don't believe this is a real problem. Global variables like this are part and parcel of WordPress.

I don't have an immediately better idea, but wonder if we could conditionally load an autoloader that we bundle in each plugin or conditionally load a bootstrap library that each plugin would register itself to so that we could use OOP to keep things like version number, registration callback, etc. encapsulated inside plugin classes, rather than global variables.

The WooCommerce implementation does go with a more OOP approach. But ActionScheduler_Versions seems needlessly complex. For example, it stores all registered versions and then finally finds the latest one to then actually load. This is simplified in my version by just only updating the pending version to load if the version is greater than what was previously registered.

Also, with OOP there is an even greater risk of your 2nd concern above. In the action-scheduler example, the first copy of the plugin gets its copy of ActionScheduler_Versions loaded into the global namespace. This freezes the class structure so that any subsequent version of the plugin must always use the same interface, or else a fatal error will occur. By going with a simple global variable for this registry, it's much less plone to such conflicts, I think.

[westonruter]

Oh, I just realized that this should not be unset. It should rather set a flag to say that the callback has been fired. Otherwise, if another copy of the plugin is loaded later (e.g. in a theme) then it would attempt to load it again, which could cause an error. An error doesn't happen in Speculative Loading currently because our load logic includes this check:

if ( defined( 'SPECULATION_RULES_VERSION' ) ) {
	return;
}

[joemcgill]

You may also be able to just set $GLOBALS[ $global_var_name ]['load'] to null so it won't fire again.

[westonruter]

In 7b3ee06 I opted to set a loaded flag on the array instead. The problem with setting $GLOBALS[ $global_var_name ]['load'] to null is that isset() would treat it the same as if $GLOBALS[ $global_var_name ]['load'] was not set at all.

[westonruter]

This is obsolete with moving to after_setup_theme so this is undone in 27284ab.

[adamsilverstein]

Overall this looks good, I'm going to test loading it multiple ways to validate functionality.

[joemcgill]

I've been thinking more about this, and I still have reservations about relying on anonymous closures and globals but this is a pretty low-risk MVP for how we could handle this across several different plugins, so I'm ok for us to go this direction for now.

As an experiment, I spent some time working up what an OOP version of this could look like in order to keep everything encapsulated in class properties rather than in the global space. We could do something simple like this:

<?php
/**
 * Plugin Loader
 *
 * @package speculation-rules
 */

if ( ! class_exists( 'PLSR_Plugin_Loader' ) ) {
	/**
	 * Class PLSR_Plugin_Loader
	 */
	class PLSR_Plugin_Loader {
		/**
		 * The plugin instance.
		 *
		 * @var PLSR_Plugin_Loader
		 */
		private static $instance;

		/**
		 * The plugin version.
		 *
		 * @var string
		 */
		public static $version;

		/**
		 * The plugin loader.
		 *
		 * @var callable
		 */
		public static $loader;

		/**
		 * PLSR_Plugin_Loader constructor.
		 */
		public function __construct() {
			if ( isset( self::$instance ) ) {
				return;
			}

			self::$instance = $this;

			$this->register_hooks();
		}

		/**
		 * Register hooks.
		 */
		private function register_hooks() {
			// Handle either where the plugin is installed as a regular plugin or is embedded in another plugin or in a theme.
			if ( ! did_action( 'plugins_loaded' ) ) {
				add_action( 'plugins_loaded', array( __CLASS__, 'load' ), 0 );
			}

			// Handle case where plugin is embedded in a theme.
			add_action( 'after_setup_theme', array( __CLASS__, 'load' ), 0 );
		}

		/**
		 * Register a plugin.
		 *
		 * @param string   $version The plugin version.
		 * @param callable $loader The plugin loader.
		 */
		public static function register( string $version, callable $loader ) {
			if (
			// Register this copy if none has been registered yet.
			! isset( self::$version ) ||
			// Or register this copy if the version greater than what is currently registered.
			version_compare( $version, self::$version, '>' )
			||
			// Otherwise, register this copy if it is actually the one installed in the directory for plugins.
			rtrim( WP_PLUGIN_DIR, '/' ) === dirname( __DIR__ )
			) {
				self::$version = $version;
				self::$loader  = $loader;
			}
		}

		/**
		 * Load the plugin.
		 */
		public static function load() {
			if ( self::$loader instanceof Closure ) {
				call_user_func( self::$loader );

				self::$loader = null;
			}
		}
	}

	new PLSR_Plugin_Loader();

}

And call it from the main plugin file like this:

<?php
/**
 * Plugin Name: Speculative Loading
 * Plugin URI: https://github.com/WordPress/performance/tree/trunk/plugins/speculation-rules
 * Description: Enables browsers to speculatively prerender or prefetch pages when hovering over links.
 * Requires at least: 6.4
 * Requires PHP: 7.2
 * Version: 1.2.1
 * Author: WordPress Performance Team
 * Author URI: https://make.wordpress.org/performance/
 * License: GPLv2 or later
 * License URI: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html
 * Text Domain: speculation-rules
 *
 * @package speculation-rules
 */

// Exit if accessed directly.
if ( ! defined( 'ABSPATH' ) ) {
	exit;
}

// Load the plugin loader.
require_once __DIR__ . '/class-perflab-plugin-loader.php';

PLSR_Plugin_Loader::register(
	'1.2.1',
	static function () {

		// Define the constant.
		if ( defined( 'SPECULATION_RULES_VERSION' ) ) {
			return;
		}

		define( 'SPECULATION_RULES_VERSION', PLSR_Plugin_Loader::$version );
		define( 'SPECULATION_RULES_MAIN_FILE', plugin_basename( __FILE__ ) );

		require_once __DIR__ . '/class-plsr-url-pattern-prefixer.php';
		require_once __DIR__ . '/helper.php';
		require_once __DIR__ . '/hooks.php';
		require_once __DIR__ . '/settings.php';
	}
);

This example would be limited to just one plugin, but it could be enhanced to support multiple different plugins by either defining a plugin class that is passed into the loader or by updating the register method of this loader class to accept a plugin slug and list of args that accept the version and loading callback.

[joemcgill]

You may also be able to just set $GLOBALS[ $global_var_name ]['load'] to null so it won't fire again.

[joemcgill]

It would be great if we could avoid having to modify the version number in more than one place. Could this be defined from the global?

[westonruter]

Like I removed in bdf96a6? I thought maybe this was overly fancy and made it more difficult to reason about. Since there's already two instances of the version in the file, a third instance doesn't seem terrible. They should all probably get updated programmatically anyway.

[westonruter]

Alternatively we could do something like this:

--- a/plugins/speculation-rules/load.php
+++ b/plugins/speculation-rules/load.php
@@ -20,6 +20,8 @@ if ( ! defined( 'ABSPATH' ) ) {
 	exit;
 }
 
+require_once ABSPATH . 'wp-admin/includes/plugin.php'; // Note: Removed in build step.
+
 (
 	/**
 	 * Register this copy of the plugin among other potential copies.
@@ -62,7 +64,7 @@ if ( ! defined( 'ABSPATH' ) ) {
 	}
 )(
 	'plsr_pending_plugin_info',
-	'1.2.2',
+	get_plugin_data( __FILE__ )['Version'],
 	static function () {
 
 		// Define the constant.
@@ -70,7 +72,7 @@ if ( ! defined( 'ABSPATH' ) ) {
 			return;
 		}
 
-		define( 'SPECULATION_RULES_VERSION', '1.2.1' );
+		define( 'SPECULATION_RULES_VERSION', get_plugin_data( __FILE__ )['Version'] );
 		define( 'SPECULATION_RULES_MAIN_FILE', plugin_basename( __FILE__ ) );
 
 		require_once __DIR__ . '/class-plsr-url-pattern-prefixer.php';

And the build step could remove those calls with the actual version. But since this requires requiring a file from wp-admin/includes this is dangerous as we could assume the functions in there are present only to have them throw a fatal when called in a production build.

[joemcgill]

Being able to get the version number directly from the file header would be ideal. However, since the version number is being stored as a global, you still shouldn't need to pass the global value to the callback like was happening prior to bdf96a6. You should be able to access the global from within the callback.

[westonruter]

It doesn't look like it because the $global_var_name is not passed to the $load callback. I'll just undo bdf96a6.

Done: 36fbaf4

[adamsilverstein]

@westonruter when I tested this as it currently stands it did not appear to work correctly, maybe because of the one change you noted is needed above.

Also, possibly I am not testing correctly - here are the steps I took to try to reproduce:

• Build the plugin and make two copies
• put one in wp-content/plugins and one in wp-content/themes/twentyeleven
• change the version string for one of the two copies to make it 1.2.0 or 1.2.2 (changed in three places where the string is used in load.php)
• include the load.php file from the theme's functions.php and activate the plugin

Check the version that was loaded by checking the global late:

add_action( 'wp_loaded', function() {
	error_log(  SPECULATION_RULES_VERSION );
} );

Expected results:

• The higher version should load

Actual results:

• The plugin always loaded when active, the theme version was only loaded when the plugin was not active, even if it had a "newer" version

[joemcgill]

@adamsilverstein

The plugin always loaded when active, the theme version was only loaded when the plugin was not active, even if it had a "newer" version

This is to be expected based on how this was implemented. See this comment from @westonruter in the description:

In contrast with the action scheduler plugin, I decided that if a copy of the plugin is installed in the plugins directory, then it should win out over other copies.

I think you would need to test a version embedded in a separate plugin against one embedded in the theme.

[adamsilverstein]

Ok good, then working as expected! I'll test including from another plugin.

[adamsilverstein]

I think you would need to test a version embedded in a separate plugin against one embedded in the theme.

Same results when included from another plugin and a theme - when that plugin is active, its version is used instead of the theme version. maybe that is expected @westonruter?

I also tested including from two separate plugins. In this case the code worked as I expected it to, the newer version was always used between the two plugins. When I activated the actual "Speculative Loading" PL plugin, that version was used.

[adamsilverstein]

Looks good, one question about functionality expectations (above)

[joemcgill]

Same results when included from another plugin and a theme - when that plugin is active, its version is used instead of the theme version.

This sounds like a bug. Could be because the bootstrap callback unsets the global before the theme has a chance to register its loader?

[westonruter]

Same results when included from another plugin and a theme - when that plugin is active, its version is used instead of the theme version.

@adamsilverstein The order of precedence currently would be this:

1. Installed as a plugin in the plugins directory, with any version.
2. Embedded in a plugin with the highest version from among other copies embedded in a plugin.
3. Embedded in a theme.

When embedded in a plugin, it's not waiting until after_setup_theme to go ahead and call the plugin's load callback. So that means in this case the copy embedded in a theme can't ever take precedence over a copy embedded in a plugin. I thought to do this because it's what action scheduler also does, and also because it could be that the plugin needs to add hooks that fire before after_setup_theme. This isn't the case for Speculative Loading, however. So maybe it should delay initializing until after_setup_theme to give themes a chance to win out?

[joemcgill]

This isn't the case for Speculative Loading, however. So maybe it should delay initializing until after_setup_theme to give themes a chance to win out?

I'd opt for consistency over trying to adjust this algorithm depending on which plugin is using something like this. I think our goal here should be to try to find a reusable solution that can be extended to any/all of our features.

[westonruter]

I'd opt for consistency over trying to adjust this algorithm depending on which plugin is using something like this. I think our goal here should be to try to find a reusable solution that can be extended to any/all of our features.

@joemcgill Since none of our plugins need to hook in before after_setup_theme I've committed 27284ab so that whether the plugin is embedded in a theme or a plugin, they'll have equal chance to win out. Still in both cases, if the plugin is installed standalone then it will win, even if it has a lesser version for the reasons explained in the description. So now the order of precedence is:

1. Installed as a plugin in the plugins directory, with any version.
2. Embedded in a theme or plugin with the highest version from among other embedded copies.

[joemcgill]

Thanks @westonruter, I think delaying the initialization of this functionality to after_setup_theme makes sense. However, if we're delaying initialization anyway, what would you think about doing so on the init hook, since most of the time, these plugins don't really need to worry about themes at all so firing on the after_setup_theme hook could be confusing.

[westonruter]

I think delaying the initialization of this functionality to after_setup_theme makes sense. However, if we're delaying initialization anyway, what would you think about doing so on the init hook, since most of the time, these plugins don't really need to worry about themes at all so firing on the after_setup_theme hook could be confusing.

@joemcgill Yeah, perhaps. But 3 of our plugins are already doing their own initialization at the init action. I suppose as long as the bootstrap logic happens at priority 0 (or even PHP_INT_MIN) then the plugin logic that runs at init would also run fine. But I think the earliest action we can use may be better as it gives us more opportunities to modify how WordPress loads which could be needed by other feature plugins to override core behavior. The after_setup_theme action is the first action that fires once a theme's functions.php has loaded, and thus it's the first action that fires which we will be able to be aware of all copies of a given plugin whether they be in a plugin or theme.

I'll note too that the Action Scheduler plugin boots its logic just before after_setup_theme since it runs inline without doing any add_action( 'after_setup_theme' ) call:

	// Support usage in themes - load this version if no plugin has loaded a version yet.
	if ( did_action( 'plugins_loaded' ) && ! doing_action( 'plugins_loaded' ) && ! class_exists( 'ActionScheduler', false ) ) {
		action_scheduler_initialize_3_dot_7_dot_3(); // WRCS: DEFINED_VERSION.
		do_action( 'action_scheduler_pre_theme_init' );
		ActionScheduler_Versions::initialize_latest_version();
	}

However, it is initializing at plugins_loaded as well:

	if ( ! class_exists( 'ActionScheduler_Versions', false ) ) {
		require_once __DIR__ . '/classes/ActionScheduler_Versions.php';
		add_action( 'plugins_loaded', array( 'ActionScheduler_Versions', 'initialize_latest_version' ), 1, 0 );
	}

What this means is that a newer version of Action Scheduler embedded in a theme cannot take precedence over an older version embedded in a plugin, because the plugin-embedded version will initialize at plugins_loaded priority 1 which will load the ActionScheduler class, which prevents the theme-specific code which has a class_exists( 'ActionScheduler', false ) condition from ever running.

All that to say, I think using after_setup_theme, with a comment explaining why that action is used, will give us the most flexibility.

Update: Done in 224a471.

[joemcgill]

We may need to think about systems that install a copy of this as an MU Plugin. Should we allow a plugin with a newer version of the API to override the MU Plugin copy?

[westonruter]

I think so because only plugins in the plugins directory can be updated.

[joemcgill]

Only if you're talking about updating from the .org plugins directory. Lots of platforms require updating via deployments where you would update the mu-plugin in similar ways to any other plugin. Regardless, you're probably right that we should leave handling of MU Plugins to the people managing those systems.

The scenario that came to mind was when a platform intentionally installed the Speculation Rules plugin as part of an MU plugin setup, but allowed some other plugin to be installed that happened to bundle a newer version of the feature. As someone managing a platform, I would want the MU Plugin version to take priority, but I could write code to ensure that happens and load it as part of whatever loader file I've placed in the mu-plugins directory.

[westonruter]

The other reason for why I think a regular plugin should take precedence over an mu-plugin is the same reason as I outlined in the description:

This is because there is a Settings link in the plugin row actions, and when something is misbehaving with the plugin it would be confusing for them to report an issue for the plugin which is active but isn't actually loaded.

If someone has the Speculative Loading plugin installed and they can see it in their plugins list as active, then if something goes awry with the plugin and they report a bug to the support forum, they should be able to share the version as listed in the plugins list page. (Granted, they could look for the generator meta tag, but users would be more likely to use the version listed in the plugin list table when reporting a bug.)

[joemcgill]

All that to say, I think using after_setup_theme, with a comment explaining why that action is used, will give us the most flexibility.

This sounds good to me. I think this is worth trying out as a first pass.

[adamsilverstein]

Looks good!