PluginRegistry should store loaded state of plugin categories to prevent dual-registration of hooks

[NateWr]

PluginRegistry::loadCategory($category) is used to load a particular set of plugins on-demand. Plugins are loaded and their register method is called.

Subsequent requests for a category of plugins should use PluginRegistry::getPlugins($category) to prevent dual-registration. However, it's not always very easy to know if a category of plugins has already been loaded, short of adding in blocks of logic like:

$themes = PluginRegistry::getPlugins('themes');
if (is_null($themes)) {
    $themes = PluginRegistry::loadPlugins('themes');
}

This logic does not tend to be used throughout the software, which means it's very common for categories of plugins to be loaded and registered multiple times, which also leads to hooks firing off multiple times when a hook is called.

I'd like to see PluginRegistry::getPlugins() automatically check under the hood if the plugin category has been loaded and load it automatically if it hasn't. Then we can refactor our usage of PluginRegistry to use getPlugins() pretty much all the time, turning loadCategory into more of an internal method.

This raises some concerns, though, about how the $enabled_only flag is used in PluginRegistry::loadCategory'. We don't maintain two lists of plugins, so it's not very easy to tell in any given point in the code whether a getPlugins call expects to receive all plugins or only enabled plugins.

Also, if PluginRegistry::getPlugins() is extended with an $enabled_only flag, the enabled status would need to be checked on each plugin before returning it. The returned $plugins list is a reference to the global plugins list, so we'd need to be passing new arrays of plugin objects if we're filtering the list. Is that alright?

I don't think there's a lot of work here, but I do think it takes someone with your bird's eye view, @asmecher, to make sure this change doesn't cause a raft of unnoticed issues.

[asmecher]

@NateWr, I suspect plugins are double-loaded on the plugin administration area, but I can't think of anywhere else it would happen... Did you run into it somewhere else, or is it a problem there?

[NateWr]

@asmecher It seems to be pretty rampant in the backend. To log this, I added the following at the top of PluginRegistry::loadCategory():

error_log( 'load plugin category: ' . $category );

To disambiguate separate requests (since a single page load could mean several separate requests to the server), I added the following to the top of the index.php file in ojs' root directory:

error_log('request: ' . uniqid() );

I then clicked around just a little bit, and found a bunch of requests where categories were loaded multiple times. Here's the log.

My sense is that the PluginRegistry api ought to handle this under the hood. At any point in the loading process I might decide I want to retrieve some plugins. I shouldn't have to know whether they've already been loaded. The registry can track that and return something appropriate.

[asmecher]

I think we'll need to support multiple loads in at least once case. Think of generic plugins:

• Once, early in the request handling, to allow enabled plugins to intervene (this needs to be fast)
• Again, e.g. on the plugins page, to list all plugins (not just enabled)

But that page is a bit of a special case.

But I think you're still seeing more duplicate loads than I would expect. And if that were widespread we'd see plugins acting several times on the same hooks due to duplicate registrations. Can you try adding this to PluginRegistry::loadCategory?

error_log("Loading " . $category . " (" . $_SERVER['REQUEST_URI'] . ")");

The URI should be helpful in disambiguating the main request from subrequests.

[NateWr]

You're right, there was some concurrency going on that made multi-loaded look more common. I still found a couple places where this happens without searching too hard.

https://gist.github.com/NateWr/e3d98ec0b95ff8bcbbd475d6d21e4648

I appreciate that, if we're careful, we can probably prevent the dual loading. But why should we have to? Shouldn't we just be able to ask PluginRegistry for what we want and have it deliver it, loading what it needs to or retrieving what's already been loaded when possible?

And yes, I think the plugins page is a special case. It's the only time I can think that we'll want to retrieve plugins that aren't enabled.

[asmecher]

@bozana, could you take a quick look at this? I think the pubIds category is getting loaded several times, and that might be problematic (not sure OTOH).

@NateWr, I like what you're proposing, but for post-3.0.

[NateWr]

Ok I've solved the problem for parent-child themes for 3.0. Punting to 3.0.1.

[jonasraoni]

I've touched this code at #8172, I think I'll double check to see if there's something missing to close this discussion 🤔