WordPress.org

Make WordPress Core

Opened 22 months ago

Closed 4 months ago

Last modified 4 months ago

#41346 closed feature request (fixed)

Introduce a hook for individual plugin loaded

Reported by: Rarst Owned by: SergeyBiryukov
Milestone: 5.1 Priority: normal
Severity: normal Version:
Component: Plugins Keywords: has-patch has-dev-note
Focuses: performance Cc:

Description

The current plugin load during core boot is essentially a series of include firing:

// Load active plugins.
foreach ( wp_get_active_and_valid_plugins() as $plugin ) {
        wp_register_plugin_realpath( $plugin );
        include_once( $plugin );
}
unset( $plugin );

Currently it is exceptionally hard to fire off arbitrary code between these includes. The closest I got over years is the following code, which I consider highly unstable: https://gist.github.com/Rarst/c32575ffc26df59a45c0

Distinguishing between load events of individual plugins is highly desirable for performance troubleshooting. In current state it is defacto impossible to troubleshoot with PHP code, requiring manual toggling of plugins or low level PHP profiler set up.

Adding individual plugin load event will make it much easier to profile and diagnose performance issues at this load stage.

Can be as simple as:

        include_once( $plugin );
        do_action( 'plugin_loaded', $plugin );

Attachments (4)

41346.diff (395 bytes) - added by schlessera 21 months ago.
Simplest solution to the issue
41346-2.patch (1.5 KB) - added by Rarst 5 months ago.
Inline documentation and hooks for mu and network plugins as well.
41346.2.diff (2.0 KB) - added by desrosj 4 months ago.
41346.3.diff (971 bytes) - added by peterwilsoncc 4 months ago.
Updated docs to clarify full path is passed as argument.

Download all attachments as: .zip

Change History (24)

#1 follow-up: @schlessera
22 months ago

Why not go all in and make it a flexible system that can be the basis of additional work?

// Pre action to allow for timing hooks to be set,
// debugging information to be initialized, etc...
// Could also allow for the loading to be skipped.
// Dangerous hook, though, needs careful consideration.
$should_load = apply_filters( 'pre_plugin_loaded', $plugin );

// Actually load the plugin.
$should_load && include_once( $plugin );

// Post hook to catch timings, debugging state, etc...
do_action( 'post_plugin_loaded', $plugin );

// Named hook to actually have a proper chance of building a simple
// dependency resolution mechanism.
$plugin_slug = some_method_to_efficiently_retrieve_plugin_slug();
do_action( "plugin_loaded_{$plugin_slug}", $plugin );

As an example for the usefulness of the named hook, such a very simple system has been used in WP_CLI to automatically resolve dependencies between commands. The basic principle would be applicable as well here. If a plugin knows it depends on jetpack, it can check whether jetpack is available when loading, and if not, it can postpone its loading by attaching its own loading process onto the plugin_loaded_jetpack hook.

#2 @Rarst
22 months ago

Preventing plugins from load is already possible (if not too cleanly) by filtering the option with active list.

To be clear while dependencies are definitely a problematic area, my prime focus with this suggestions is performance troubleshooting and enabling something that is both valuable and low hanging for it.

I don't see (at the moment) anything critical for inter–plugin interactions that cannot be handled on long established plugins_loaded hook, signifying complete load of them.

@schlessera
21 months ago

Simplest solution to the issue

#3 @schlessera
21 months ago

  • Keywords has-patch added

I uploaded a very simple patch that immediately solves the raised issue.

As regards to what I had suggested as an enhancement, I will work on a bigger issue/patch that makes one proposal of solving plugin interdependencies, just to show how that could potentially work.

#4 @johnbillion
15 months ago

  • Milestone changed from Awaiting Review to 5.0
  • Owner set to johnbillion
  • Status changed from new to reviewing
  • Version 4.9 deleted

#5 @schlessera
13 months ago

I've created a new ticket for the suggestion I made earlier in here at #43882. This way, discussing the more elaborate mechanism does not block us from just committing the simple fix we have in here.

#6 @johnbillion
7 months ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 5.0 to 5.1

#7 @Rarst
6 months ago

I am not sure why has-patch was dropped here, 41346.diff is valid and exactly what I suggested.

#8 @johnbillion
5 months ago

  • Owner johnbillion deleted

@Rarst The patch needs a docblock with description etc

#9 @SergeyBiryukov
5 months ago

  • Keywords needs-docs added
  • Owner set to SergeyBiryukov

The new hook needs to be documented as per the documentation standards.

Last edited 5 months ago by SergeyBiryukov (previous) (diff)

#10 @Rarst
5 months ago

I'll take care of documentation, cheers. :)

#11 in reply to: ↑ 1 ; follow-up: @ChriCo
5 months ago

Replying to schlessera:

// snip ...
do_action( "plugin_loaded_{$plugin_slug}", $plugin );

Isn't it better to have

do_action( "plugin_loaded", $plugin, $plugin_slug );

? Otherwhise you would need to hook into {n}-Plugins. Instead you can hook into one action and have all possibilities.

#12 in reply to: ↑ 11 @Rarst
5 months ago

Replying to ChriCo:

Replying to schlessera:

// snip ...
do_action( "plugin_loaded_{$plugin_slug}", $plugin );

Please note that's not what the current patch actually suggests, it's in fact close to your snippet. :) I would be against dynamic hook as well, they are a mess.

Last edited 5 months ago by Rarst (previous) (diff)

#13 @Rarst
5 months ago

  • Keywords has-patch added; needs-patch needs-docs removed

I've added inline documentation and for consistency respective hooks of MU and network plugins as well.

@Rarst
5 months ago

Inline documentation and hooks for mu and network plugins as well.

#14 @SergeyBiryukov
5 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 44344:

Plugins: Introduce actions for individual plugin load events:

  • plugin_loaded: Fires once a single activated plugin has loaded.
  • mu_plugin_loaded: Fires once a single must-use plugin has loaded.
  • network_plugin_loaded: Fires once a single network-activated plugin has loaded.

Props Rarst, schlessera.
Fixes #41346.

#15 @desrosj
4 months ago

  • Keywords needs-dev-note added
  • Resolution fixed deleted
  • Status changed from closed to reopened

I am working on a dev note for these new action hooks and came across something we should fix.

The inline documentation for the three new hooks says that the plugin basename is passed. In the current state of the code, this is not correct. The full path to the plugin is actually passed instead. At a minimum, the inline docs should be updated to accurately reflect that the full path is passed.

While I don't necessarily think that passing the full path is bad, I don't know that it is the most useful value to pass to this action. In writing code examples for the dev note, I realized that targeting a specific plugin in that action hook is not as straightforward as it could be.

For example, if I wanted to target code to only run when "myplugin" was loaded, I believe that the best way to really write a conditional to target that plugin in the current state would be as follows:

if ( false !== strpos( $plugin, 'myplugin/myplugin.php ) )

While not terrible, it is not ideal. The full path could be any number of variations baed on server configuration and file structure and the plugin directories can be changed by defining constants before core to name a few of the issues that could come up. The following would be a much better approach:

if ( 'myplugin/myplugin.php' === $plugin )

Thinking this through, there are a few options. First is to update wp_get_mu_plugins(), wp_get_active_network_plugins(), and wp_get_active_and_valid_plugins(), to accept an optional parameter to return both the full path and the shorter plugin name. This seems too risky, though. Especially this late in the release cycle, as these functions have been around for quite a while.

The second option is to just reverse the string concatenation performed in those functions before passing the value to the hook. Attaching a patch with that approach for feedback.

@desrosj
4 months ago

#16 @desrosj
4 months ago

I would also be ok with not passing the full path if there is agreement that it is not particularly useful.

#17 @Rarst
4 months ago

Good catch! Somehow I was sure those are basenames, though now that you pointed it out obviously they need to be full paths to be correctly included. Hashtag embarrassed.

As a note on your patch the plugin_basename() should probably be used for consistent result and normalization of path.

However I question do we really care that path is full? Effectively it's just more complete set of information.

Also I need to point out that folder/filename.php is not a reliable signature of plugin. There are scenarios where plugin is defacto installed in a different folder name from what original developer intended. That's why the best practice is to always calculate actual basename on plugin boot, rather than hardcode it.

So providing basenames can be somewhat encouraging to hardcoded checks.

So I suggest we just stick with full path and change inline documentation accordingly. If basename is needed it can be extracted out of full path as necessary by a callback.

#18 @desrosj
4 months ago

Continuing to pass the full path and updating the documentation is fine with me!

@peterwilsoncc
4 months ago

Updated docs to clarify full path is passed as argument.

#19 @peterwilsoncc
4 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 44692:

Plugins: Update docs for individual plugin loaded actions.

Corrects documentation to indicate the full path is passed to each action.

Fixes #41346.

#20 @desrosj
4 months ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.