Make WordPress Core

Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#41346 closed feature request (fixed)

Introduce a hook for individual plugin loaded

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


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:

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 7 years ago.
Simplest solution to the issue
41346-2.patch (1.5 KB) - added by Rarst 5 years ago.
Inline documentation and hooks for mu and network plugins as well.
41346.2.diff (2.0 KB) - added by desrosj 5 years ago.
41346.3.diff (971 bytes) - added by peterwilsoncc 5 years ago.
Updated docs to clarify full path is passed as argument.

Download all attachments as: .zip

Change History (24)

#1 follow-up: @schlessera
7 years 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
7 years 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.

7 years ago

Simplest solution to the issue

#3 @schlessera
7 years 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
6 years 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
6 years 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
6 years ago

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

#7 @Rarst
6 years ago

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

#8 @johnbillion
5 years ago

  • Owner johnbillion deleted

@Rarst The patch needs a docblock with description etc

#9 @SergeyBiryukov
5 years ago

  • Keywords needs-docs added
  • Owner set to SergeyBiryukov

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

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

#10 @Rarst
5 years ago

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

#11 in reply to: ↑ 1 ; follow-up: @ChriCo
5 years 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 years 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 years ago by Rarst (previous) (diff)

#13 @Rarst
5 years 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.

5 years ago

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

#14 @SergeyBiryukov
5 years 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
5 years 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.

5 years ago

#16 @desrosj
5 years ago

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

#17 @Rarst
5 years 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
5 years ago

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

5 years ago

Updated docs to clarify full path is passed as argument.

#19 @peterwilsoncc
5 years 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
5 years ago

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