Couldn't use the plugin manager here because getActivatedPlugins() returned an empty array (plugins weren't loaded yet).

Can we load plugins before that or so? We should not really include configs of not activated plugins. Also in tracker mode we have much less plugins that need to be loaded/activated etc.

Here is how FrontController::init() goes:

        $tmpPath = StaticContainer::get('path.tmp');

        $directoriesToCheck = array(
            $tmpPath,
            $tmpPath . '/assets/',
            $tmpPath . '/cache/',
            $tmpPath . '/logs/',
            $tmpPath . '/tcpdf/',
            $tmpPath . '/templates_c/',
        );

        Filechecks::dieIfDirectoriesNotWritable($directoriesToCheck);

        $this->handleMaintenanceMode();
        $this->handleProfiler();
        $this->handleSSLRedirection();

        Plugin\Manager::getInstance()->loadPluginTranslations();
        Plugin\Manager::getInstance()->loadActivatedPlugins();

Is it safe to move Plugin\Manager::getInstance()->loadActivatedPlugins(); before the "is writable" check? If yes then that would allow to load the plugins before the container is created.

Not sure, it should be safe. Needs to be tested and checked what is done in there, only had a rough look.

Also we need to make sure that during this process DI is not used in the background eg by Config etc? Otherwise it might end in a problem again...

Maybe it makes sense to load "global.php" first... while loading/activating the plugins we also load their definitions. Might be easiest solution that should work and also makes sure we load it only if needed eg while tests are run etc. On "deactivate Plugin" we could maybe unload the definition - if possible. Not really needed though.

Loading config files (or even changing definitions) after the container has already been used is really risky. If any service or value has already been resolved and injected somewhere, it will not be replaced.

E.g. the logger is injected somewhere, then a plugin changes the logger: the old one is still injected in some places… Leading to inconsistent services.

It might not be a problem directly, and the logger might not be so critical, but it's calling for trouble later with more serious components or config values.

Ideally the container should be built in one step and entries should not be changed after that.

Actually funny thing, path.tmp is an example of something a plugin might change: the CloudAdmin plugin could override that to add the "instance ID" to separate data for each instance.

Today it's using this "instance ID" hack but in the future it could be the plugin that handles that using the container.

Quick n dirty example to illustrate:

// global.php
'path.tmp' => 'tmp',

// CloudAdmin/plugin.php
'path.tmp' => 'tmp/' . $instanceId,

I've updated the diff to load plugins before the container.

I've updated the diff to load plugins before the container.

It looks like the diff is not updated though? it says Config::getInstance()->Plugins['Plugins'];

Yep after the discussions (below) I went back to the initial solution

It's not yet loaded in the correct order, is it?

Is the config already cached or do we do this on each request? Can we name it maybe config.php or so? Would be maybe more intuitive for developers

As I said in the PR config.php wasn't possible.

There is no cache for now (well there is the ArrayCache but of course not persistent between requests).

Would be nice to have a cache at some point, especially re tracking.

Can we maybe rename/move the existing Config classes? Or check whether that file actually returns something and if not, ignore it? We will probably have this convention for a while so might be worth it

Could the file be located in a config folder, ie plugins/MyPlugin/config/config.php? Ie, mirror the top-level folder structure.

I've implemented plugins/MyPlugin/config.php, see the diff. Not the most efficient piece of code ever but I guess the opcache should make it a non-problem.

I like having config.php at the root of the plugin. But if the change is really not efficient, we could move it to a sub-directory.

But for 3.0 we could say config.php must be a config file, move your class elsewhere. In the meantime we keep BC. So the current Config classes would be sort of "deprecated".

@diosmosis haven't thought of that. Actually I'd be 👍 We always try to use same structure already (eg with other folders Columns, tests/UI and tests/*, Tracker, Visualization, ... ) so would make sense. I'm sure we'll have a generator for this at some point so it wouldn't matter when it is in a config directory. Would also allow to add maybe other config entries if needed. Maybe the config folder would be also useful if we allow to provide something like config-dev.php or config/environment/dev.php that will be automatically used for development etc?

/config.php would be otherwise ok for me as well

Would also allow to add maybe other config entries if needed. Maybe the config folder would be also useful if we allow to provide something like config-dev.php or config/environment/dev.php that will be automatically used for development etc?

Great point, I'm +1 with that idea then, I'll move again to config/config.php.

@tsteur I have added this method but it kinda duplicates this one just above:

    public function getActivatedPlugins()
    {
        return $this->pluginsToLoad;
    }

However replacing the existing method by the one I added just blew up everything… I think it's simply that the existing one has a bad name (it doesn't return the activated plugins, it returns the ones the plugin manager is supposed to load). I could rename it from getActivatedPlugins() to getPluginsToLoad() if it makes the diff any better.