October: Invalid plugin registration order

[adrenth]

Hi OctoberCMS community! We (Van der Let & Partners) would like to address an issue we found in the OctoberCMS' plugin registration order.

• OctoberCMS Build: latest
• PHP Version: 7.0+
• Database Engine: any
• Plugins Installed: Acme.Demo1, Acme.Demo2

---

Problem

Currently the PluginManager uses an RecursiveIteratorIterator https://github.com/octobercms/october/blob/master/modules/system/classes/PluginManager.php#L401 to fetch all the plugins of the application. The order of this is completely random.

There are a couple of situations in which the random order of plugin registration can cause issues.

1. Overriding container bindings of an other plugin.

• When you have Acme.Demo1 with an interface ExampleServiceInterface and implementation DefaultExample. If you bind that to the container via $this->app->bind(ExampleServiceInterface::class, DefaultExample::class) and then resolve ExampleServiceInterface you get an instance of DefaultExample.

• When you have an Acme.Demo2 that depends on Acme.Demo1 (via the Plugin.php property $depends = ['Acme.Demo1']), you'd expect to override ExampleServiceInterface with OtherDefaultExample via $this->app->bind(ExampleServiceInterface::class, OtherDefaultExample::class).

• However, with the random ordering of plugin registration, it could happen that Acme.Demo2 is loaded before Acme.Demo1.

2. Having migrations that depend on tables of another plugin.

• Basically the same as above. Plugin registration of one plugin occurs before another, with no way to influence the order.

Our solution

This is a solution we apply to all our October CMS projects for a couple of years now. Our solution is overriding the property $plugins on the PluginManager. This array is looped over to register the plugins, boot them and execute migrations.

Since the property is protected we use reflection.

config/app.php

    'providers' => array_merge(include(base_path('modules/system/providers.php')), [

        Acme\Demo\ServiceProviders\SystemServiceProvider::class,
        System\ServiceProvider::class,

    ]),

plugins/acme/demo/serviceproviders/SystemServiceProvider.php

namespace Acme\Demo\ServiceProviders;

use Illuminate\Support\ServiceProvider;
use ReflectionException;
use ReflectionProperty;
use System\Classes\PluginManager;

/**
 * Class SystemServiceProvider
 */
final class SystemServiceProvider extends ServiceProvider
{
    /**
     * @return void
     * @throws ReflectionException
     */
    public function register(): void
    {
        $pluginManager = PluginManager::instance();

        $plugins = $pluginManager->getPlugins();
        ksort($plugins);

        $sortedPlugins = $pluginManager->sortByDependencies($plugins);
        $map = [];

        foreach ($sortedPlugins as $pluginName) {
            $map[$pluginName] = $plugins[$pluginName];
        }

        $reflection = new ReflectionProperty($pluginManager, 'plugins');
        $reflection->setAccessible(true);
        $reflection->setValue($pluginManager, $map);
    }
}

A better solution?

• Solve this issue in the October Rain library: This could be achieved by using the sortByDependencies when loading the property plugins in the loadPlugins method.
• Introduce a (temporary) plugin which addresses this issue?
• Add comment below...

We would like to know if anyone in the community experiences the same behaviour in their projects. And what solution you'd prefer.

[LukeTowers]

@adrenth we'd accept a PR to sort by dependency by default, this is an issue that multiple people have reported over the years and we just haven't gotten around to it yet.

[daftspunk]

I understand how this could be frustrating but in most cases there are ways to influence the load order regardless of how they appear in the file system, using boot/register methods, using App::before(), etc.

1. Having migrations that depend on tables of another plugin.

The migrations should be accounted for in the update manager using the $require property:

https://github.com/octobercms/october/blob/91d7b980a9abd299f0300b878b43410934a696b4/modules/system/classes/UpdateManager.php#L149-L155

If a plugin depends on another, then the parent is migrated first. Be careful with circular dependencies

I think it would be better to discuss an actual use case that is causing you problems to see if there isn't already a solution that can be used. By this I mean, not Acme/Example hypotheticals

[mjauvin]

@daftspunk can you elaborate on these:

in most cases there are ways to influence the load order regardless of how they appear
in the file system:

- using boot/register methods
- using App::before()

[daftspunk]

Given a concrete example of the problem it would be easier to articulate an elaborate response. While it is usually ok to depend on this sorting for migrations, for service providers an issue arises where two equal dependencies fight for dominance and the issue remains; we only kick the can down the road. The solution beyond that only gets more complex and less performant for systems that don’t care about this, so it is best not to punish everyone for it

While I agree the random load ordering is an annoyance. There are many ways to approach this, such as weighing plugins, so they don’t fight. Without a clear demonstration of the issue it is hard to know what is best, due to the complex nature of the issue

[daftspunk]

From Laravel book:

After all providers have been registered, they are “booted”. This will fire the boot method on each provider. A common mistake when using service providers is attempting to use the services provided by another provider in the register method. Since, within the register method, we have no guarantee all other providers have been loaded, the service you are trying to use may not be available yet. So, service provider code that uses other services should always live in the boot method. The register method should only be used for, you guessed it, registering services with the container. Within the boot method, you may do whatever you like: register event listeners, include a routes file, register filters, or anything else you can imagine.

To answer the above example:

Acme.Demo1 should register its interface via the register method, it is now the owner of this interface

public function register()
{
    // First priority (always first)
    $this->app->bind(ExampleServiceInterface::class, DefaultExample::class);
}

To ensure availability Acme.Demo2 should override this via the boot method, it is now a consumer of this interface

public function boot()
{
    // Second priority (we have room for weighting here)
    $this->app->bind(ExampleServiceInterface::class, OtherDefaultExample::class);
}

In a rare case you want a secondary consumer to override the interface, there is another option to use App::before to achieve this, Acme.Demo3 would need to do the following:

public function boot()
{
    // Last priority (always very last)
    App::before(function() {
        $this->app->bind(ExampleServiceInterface::class, OtherDefaultExample::class);
    });
}

Secondary consumers should be a rare event, but still manageable. For us to make this unbounded, multi-consumer, a weighted event system is required, for example, in the boot() method we could apply something like this:

public function boot()
{
    // Override something in the container with a weight of 500
    $this->afterBoot(function(){ /* modify IoC container */ }, 500);
}

Note: The above is a proposed solution/code. It is not currently available but an idea of how we can apply service provider weighting

This would trigger an event with a priority of 500, allowing other plugins to get in before (>500) or after (<500)

[adrenth]

Thank you all for commenting! Great responses so far.

I think @daftspunk has brought a good work-around for this issue. But why make it so complex? The only way to specify the plugin dependency hierarchy is by filling the $require. That's very clear to all developers.

Plugin B depends on plugin A, so Plugin B should assume all ServiceProviders and such are registered at the point plugin B registers its ServiceProviders. And the same counts for the migrations.

So if the plugins are loaded/booted in the correct order in the first place, developers don't need to create such code structures to force the correct order of loading... right? (e.g. using the $this->afterBoot() to mutate the IoC container, that feels wrong).

We will come up with some concrete examples shortly.

[daftspunk]

After some lengthy consideration and playing devil's advocate on this idea. @adrenth makes the champion point that sorting by dependencies "just works" in the majority of cases

Performance issues

• The additional execution time of sorting plugins appears to be negligible
• A caching system may not ever be needed

Circular dependencies

• Circular dependency is classed as a fatal error
• When a circular dependency is found, the system will now crash with SystemException
• Previously, it would only crash upon database migration

Consideration

• Plugins are now sorted alphabetically by their keys, this makes the system more predictable overall
• When two equal dependencies are detected, we can introduce afterRegister and afterBoot hooks to overcome this rare scenario, should it ever occur

Added in adb303a53c5eabff0a41519f3b52638166decd99
Available in Build 463+