Skip to content
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.

Sign up for GitHub

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

Facilitate embedding Speculative Loading in other plugins/themes #1159

Merged
merged 10 commits into from
Apr 29, 2024
Merged

Facilitate embedding Speculative Loading in other plugins/themes #1159

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b44313a
Facilitate embedding Speculative Loading in other plugins/themes
westonruter Apr 17, 2024
7ae4e44
Use 0.0.0 as SPECULATION_RULES_VERSION for PHPStan
westonruter Apr 18, 2024
bdf96a6
Eliminate passing version into load callback
westonruter Apr 18, 2024
cb53fad
Merge branch 'trunk' of https://github.com/WordPress/performance into…
westonruter Apr 19, 2024
7b3ee06
Set loaded flag to prevent theme from attempting to load
westonruter Apr 19, 2024
27284ab
Delay loading plugin until after_setup_theme
westonruter Apr 19, 2024
224a471
Explain why after_setup_theme action is used
westonruter Apr 23, 2024
ee85351
Use PHP_INT_MIN instead of 0
westonruter Apr 23, 2024
36fbaf4
Revert "Eliminate passing version into load callback"
westonruter Apr 23, 2024
809e076
Merge branch 'trunk' into add/speculation-rules-embeddability
westonruter Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions bin/phpstan/constants.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php
/**
* Constants for PHPStan static analysis.
*
* @package speculation-rules
*/

define( 'SPECULATION_RULES_VERSION', '0.0.0' );
define( 'SPECULATION_RULES_MAIN_FILE', 'plugins/speculation-rules/load.php' );
2 changes: 1 addition & 1 deletion phpstan.neon.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ parameters:
- plugins/
- tests/
bootstrapFiles:
- plugins/speculation-rules/load.php
- bin/phpstan/constants.php
scanDirectories:
- vendor/wp-phpunit/wp-phpunit/
dynamicConstantNames:
Expand Down
74 changes: 64 additions & 10 deletions plugins/speculation-rules/load.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,69 @@
exit;
}

// Define the constant.
if ( defined( 'SPECULATION_RULES_VERSION' ) ) {
return;
}
(
/**
* Register this copy of the plugin among other potential copies.
*
* @param string $global_var_name Global variable name for storing the plugin pending loading.
* @param string $version Version.
* @param Closure $load Callback that loads the plugin.
*/
static function ( string $global_var_name, string $version, Closure $load ) {
if ( ! isset( $GLOBALS[ $global_var_name ] ) ) {
$bootstrap = static function () use ( $global_var_name ) {
if (
isset( $GLOBALS[ $global_var_name ]['load'], $GLOBALS[ $global_var_name ]['version'] )
&&
$GLOBALS[ $global_var_name ]['load'] instanceof Closure
&&
is_string( $GLOBALS[ $global_var_name ]['version'] )
) {
call_user_func( $GLOBALS[ $global_var_name ]['load'], $GLOBALS[ $global_var_name ]['version'] );
unset( $GLOBALS[ $global_var_name ] );
Copy link
Member Author

@westonruter westonruter Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
};

// 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', $bootstrap, 0 );
}

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

// Register this copy of the plugin.
if (
// Register this copy if none has been registered yet.
! isset( $GLOBALS[ $global_var_name ]['version'] )
||
// Or register this copy if the version greater than what is currently registered.
version_compare( $version, $GLOBALS[ $global_var_name ]['version'], '>' )
||
// Otherwise, register this copy if it is actually the one installed in the directory for plugins.
rtrim( WP_PLUGIN_DIR, '/' ) === dirname( __DIR__ )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

) {
$GLOBALS[ $global_var_name ]['version'] = $version;
$GLOBALS[ $global_var_name ]['load'] = $load;
}
}
)(
'plsr_pending_plugin_info',
'1.2.1',
static function ( string $version ) {

Copy link
Member Author

@westonruter westonruter Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

define( 'SPECULATION_RULES_VERSION', '1.2.1' );
define( 'SPECULATION_RULES_MAIN_FILE', plugin_basename( __FILE__ ) );
define( 'SPECULATION_RULES_VERSION', $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';
require_once __DIR__ . '/class-plsr-url-pattern-prefixer.php';
require_once __DIR__ . '/helper.php';
require_once __DIR__ . '/hooks.php';
require_once __DIR__ . '/settings.php';
}
);
Loading