Following up on #1831350: Break the circular dependency between bootstrap container and kernel , we need to make sure FileStorage is not hardwired as it is now. The naive approach is a classname overrideable in settings.php. A more versatile approach is a callable again overrideable from settings.php. That sounds the best approach we just need to figure out where to put a static function with return new FileStorage(config_get_config_directory()); in it.

The current suggestion is to pass an object which looks like like a precompiled container containing the config storage (complete with an event dispatcher and conf override), the php storage and the classloader to the DrupalKernel. An optional callable overriding FileStorage will be provided. This way a) the config storage objects in most cases won't need to be instantiated b) we get the most speed which is crucial at this extremely early stage c) FileStorage is not hardwired.

As for overriding the services in the container itself, simpletest can add a bundle which adds a compiler pass overriding the services. Compiler passes run after registering services.

Related

#2105689: Acknowledge that our import/export config storage is not swappable

Comments

chx’s picture

Following up on #1831350-92: Break the circular dependency between bootstrap container and kernel which suggested building a mini container and making the name of the callable returning that container overrideable in settings.php. This would allow settings.php to override the system.module config object via $conf so that's an interesting approach to explore. I think we could make the callable providing this container overrideable via settings.php.

I am less sure about putting the php storage into the container as it basically would duplicate the exisiting drupal_php_storage function.

sun’s picture

The main idea is:

  1. There is a minimal bootstrap container. It has the sole purpose of providing the necessary services to boot or (re)build a Drupal kernel.
  2. Unlike drupal_container(), this minimal bootstrap container is injected into the kernel, because it supplies the kernel's dependencies.
  3. It persists kernel rebuilds/module-updates within the kernel as a property.
  4. It is not used for anything else than bootstrapping the kernel and does not attempt to be useful for anything else.
  5. It is only accessed once to boot the kernel, which explains why it does not need any static caching in the first place.
  6. As soon as the kernel booted, the minimal bootstrap container is irrelevant. (unless the kernel cannot be booted and needs to be rebuilt)
  7. Tests can inject a different minimal bootstrap container, so as to boot the kernel differently.

I think this is what we need and the way to go.

It basically eliminates drupal_container() and turns the kernel into the thing it seemingly wants to be: the God object that controls the entire application. In other words, without booting the kernel, you cannot do anything. (I'm absolutely not a fan of this architectural design, but this is where Symfony/WSCCI folks are pushing us into.)

Further work on #92 revealed, however, that this indeed introduces the kernel as a God object, and God objects commonly do not allow for fine-grained manipulations. As such, we're running into the problem that the current drupal_container() - albeit an ugly construct - allows DrupalUnitTestBase to partially override a range of services with memory implementations.

With the new flow, that won't be easily possible anymore, since it will always be this:

  1. drupal_bootstrap_container()
  2. DrupalKernel::boot() (forgets about drupal_bootstrap_container())
  3. DrupalKernel::registerBundles() —> CoreBundle::build() —> config + lock + cache + etc.pp. using regular implementations.

To clarify the problem again: DrupalUnitTestBase::setUp() needs a way to override the service definitions that happen in CoreBundle::build(). (drupal_container() ceases to exist; it will no longer register services; its only purpose is to reach DrupalKernel::getContainer() from procedural code.)

To clarify further: Tests need to be able to override the services in a fine-grained way. I.e., providing a "default" override/replacement of CoreBundle for tests is not of any help.

AFAICS, there'd be two possibilities to achieve that:

  1. Container parameters:
    1. Parametrize most service definitions in bundles (especially CoreBundle) by leveraging Container parameters.
    2. Set the overrides for default parameters on the drupal_bootstrap_container().
    3. Merge the parameters (not the services) from drupal_bootstrap_container() into the kernel container, so they take precedence over the default parameters.
    4. Make DrupalUnitTestBase::setUp() call drupal_bootstrap_container() and adjust the service container parameters before it bootstraps the kernel.

    By implanting the identical concept/construct into drupal_settings_initialize() like this:

    $container = drupal_bootstrap_container();
    require 'settings.php';
    

    ...we could automatically leverage the identical mechanism for service overrides from settings.php.

    (instead of a new wonky global $settings variable or countless of new service-specific globals like $cache_classes or $php_storage or whatever)

    The only "downside" to this approach would be that all possible service implementations would always have to be registered as services, because only the container parameters decide which service is referenced for another service; i.e., keyvalue.memory, cache.memory, lock.null, etc.pp. would always be registered as services — merely not used unless referenced through parameters.

  2. Turn test classes into bundles:
    1. Find a way to turn a test class into a bundle, and ensure that the test class bundle is registered last.
    2. Implement DrupalUnitTestBase::build() methods to override service definitions in the kernel container.
chx’s picture

I had written an elaborate answer then it hit me that we do not need most of it.

Yes to the bootstrap container. Just make sure it's very clearly documented what it's for. If it's not static'd it's not like it can be used for evil things.

My proposal is to change registerBundles to read a list of extra bundles of a parameter of the bootstrap container and add those after the modules. bootstrap.inc should read that list from settings.php and testing should set it to whatever it needs. Very simple and very powerful overrides for everyone.

chx’s picture

to read a list of extra bundles of a parameter of the bootstrap container => to read a list of extra bundles from a parameter of the bootstrap container

chx’s picture

D'oh but that doesn't solve the problem of making FileStorage pluggable! And it bothers me that we are using a much slower mechanism (the service container) when 99.99999999% of the time just hardwiring 'em will just do. While the container has the advantage of creating objects on demand, that's not too hard to emulate here as well. Basically what I suggest is pre-compile this container, pre-compile it in our editors:

class BootstrapContainer {
  function configStorage() {
    $dispatcher = new EventDispatcher;
    $dispatcher->addSubscriber(new ConfigGlobalOverrideSubscriber);
    if (isset($GLOBALS['conf']['drupal_bootstrap_config_storage'])) 
      $config_storage = call_user_func($GLOBALS['conf']['drupal_bootstrap_config_storage']);
    }
    else {
      $config_storage = new FileStorage(config_get_config_directory(CONFIG_ACTIVE_DIRECTORY));
    }
    return new ConfigFactory($config_storage, $dispatcher);
  }
  function classloader() {
    return drupal_classloader();
  }
  function phpStorage() {
    return drupal_php_storage('service_container');
  }
}
function drupal_god() {
  $extra_bundles = isset($GLOBALS['conf']['extra_bundles']) ? $GLOBALS['conf']['extra_bundles'] : array();
  return new DrupalKernel('prod', FALSE, new BootstrapContainer, $extra_bundles);
}

Can be tweaked by sending $GLOBALS['conf'] to the constructor etc.

The advantages are speed and the re-use of the existing pluggability in drupal_php_storage.

katbailey’s picture

I really don't see why we need a container to build out the services needed to boot the kernel. Just instantiate what is needed, e.g. whatever storage mechanism is needed for CMI, and inject it into the kernel. Same for PHP storage, just as it's done now in fact. Then if these are required to be services themselves they can be injected into the container as synthetic services. I will write a patch for this once #1831350: Break the circular dependency between bootstrap container and kernel gets in.

Tests need to be able to override the services in a fine-grained way.

Your option 2 above is the way to do this, ideally using a compiler pass. See http://symfony.com/doc/2.0/cookbook/bundles/override.html#services-confi..., in particular:
"Secondly, if the class is not available as a parameter, you want to make sure the class is always overridden when your bundle is used, or you need to modify something beyond just the class name, you should use a compiler pass."

katbailey’s picture

OK, I chatted with chx about this in irc and he pointed out the reason for using a container, i.e. to lazily instantiate these things because they will not actually be needed on every request. So I grudgingly concede (wah!) that a bootstrap container, albeit a precompiled one like this, might be the way to go.
As for the 'extra_bundles' idea, this is not needed. As I mentioned in the previous comment we can just use a compiler pass for overriding services.
I won't even comment on the drupal_god() function. Well I guess I just did :-P

chx’s picture

If extra bundles go away we don't need that. And consider the naming of the function my way of commenting on what the container is becoming :)

I updated the issue summary.

chx’s picture

Issue summary: View changes

updated with the pseudo-container approach

chx’s picture

As for synthetic services, we could (but perhaps not in this issue!) move drupal_php_storage into a PhpStorageFactory object and get the kernel register that as a synthetic service into the DIC. (an example for a synthetic service is request which is registered in CoreBundle and set in core/lib/Drupal/Core/HttpKernel.php) This would allow testing to use a different storage factory.

Crell’s picture

I think exposing the PHP Storage system as a (synthetic) service in the container is a good idea anyway, even if it's never truly swappable. It still makes it more cleanly exposed to other systems post-kernel.

sun’s picture

Issue tags: +Configuration system
sun’s picture

Issue summary: View changes

Added compiler pass.

mtift’s picture

Issue summary: View changes

Add related issue

alexpott’s picture

Issue summary: View changes
Status: Active » Closed (duplicate)

Since #2161591: Change default active config from file storage to DB storage we've got DrupalKernel using an alternative config storage. And https://drupal.org/project/config_devel proves we can swap it back.