Stop doing so much pre-kernel bootstrapping

IMNSHO the solution would be a 'push' based approach where a module enable/disable creates the DIC and that's it, one DIC.

But this creates a registry-like nightmare where the loss of a module or the movement of it causes Drupal to go down with a fatal.

Due to PHP throwing uncatchable fatals instead of Exceptions, this is not easy to solve. But, I think, one thing we can do is to redirect to a rebuild (I was even thinking of reusing install.php, think of it) from the classloader if we are to return FALSE. Except that class_exists triggers the same classloader and I am not sure we want to peek into the backtrace to figure out whether the autoloader is fired for a friendly class_exists check or a 'hostile' situation (class foo extends bar, bar tries to load, moved away, kaboom). Perhaps a drupal_class_exists should be used which marks somehow that a false is ok to return? There are only 12 class_exists calls in Symfony and Twig involving a variable class name, 5 of them in some sort of a Loader. We might be able to get away with mandating a drupal_class_exists indeed and do the redirect to a rebuild in case of trouble.

I am just rambling aloud.

I don't think DIC rebuilding is the issue here; DIC instantiation is.

Is there a reason we can't just cut/paste most of index.php pre-DrupalKernel-instantiation into DrupalKernel::boot() (or init(), or whichever is appropriate), then let the chips fall where they may and refactor from there? That is, the startup routine is:

1) Load config.
2) Create Kernel.
3) Init kernel (which starts the DIC).
4) Do anything else useful.
5) Handle a request as normal.

We more or less fixed this already in #1597696: Consider whether HttpCache offers any significant benefit over the existing page cache, but I like the idea of getting this in independently of that issue.

Attached patch is just the changes beejeebus and I made to fix the bootstrap issue.

Looks like an improvement to me.

Can't we initialize the DIC first, then call drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE)?

One major question I have is:
AFAIK, the container that is being compiled by the Kernel gets written to an arbitrary filesystem location via PhpStorage. Which makes sense, because most of the services in there are defined dynamically; i.e., they depend on modules/bundles being enabled and so on.

However, I think we want and need the low-level services defined in the early/minimal bootstrap container to be static and editable. I.e., I need to be able to replace and customize the configuration system, state storage, database, and cache backend service definitions before Drupal attempts to use them in any way, so as to prevent it from attempting to read or write anything from or to a storage/backend that must not be used in my setup.

In light of that, I really liked the idea of having a precompiled container in a [site]/services.yml as mentioned in #1703266-5: Use a precompiled container for the minimal bootstrap container required outside of page requests, since it essentially serves as a replacement for wonky global variables defined in settings.php and gets rid of them by using explicit service definitions. Unless manually prepared and provided already, this container would be initially written by the installer (and install profiles/distros would be able to have their say), but afterwards left alone.

Core is a Symfony bundle or not? If so, it can define those "pre-bootstrap" services as normal DIC services, which will be compiled as in any SF site. Sites could define overrides via their own bundles (profile bundle, module bundle, or just some config file somewhere). If I recall correctly, those overrides are possible and legit in Symfony, so why not for us? If we consider this as being true, we don't need any "pre-bootstrap container", we just have a core bundle whose configuration can be overriden in many ways quite easily (compiling doesn't matter here, it's just an optionnally added behavior to the DIC but which should not matter to business). The only requirement for bootstrapping normally just like any SF2 site in this context is having the system list pre-defined somewhere instead of into database so that we can load bundles without requiring database (or at least the profile and core bundles, and the path to potential per config overrides).

If we write the container to disk, then it should not be necessary to call system_list() to register bundles each request, since we'd only need to do that when compiling the container no? I didn't get to properly review the bundles patch before it went in and I think I need to sit down with it a bit more after the fact now..

True, but in that case we still need the database service in order to load bundles, which kinda makes a circular dependency.

If we write the compiled DIC to disk on module change then we are good. I do not understand #2 which is supposed to be an answer to my rant? How is DIC instantiation is a problem once it's compiled? Once it's compiled if I understand things correctly you do not need to have bundles, that's just the build phase.

See #1 for more.

IMHO we should not even consider the compile phase exists when we deal with this sort of bugs. The normal behavior is making it work gracefully without, then if it works, the compiled version will follow gracefully, since it's just supposed to be a decorator pattern around the DIC that is tied to performances only but that should never ever alter anything else than performances.

If we leave out the compiled DIC, needing the database to load bundle might be some kind of circular dependency because we need to 1) load core bundle 2) build the DIC in order to bootstrap and use the database 3) load all other bundles that will alter the DIC (sadly too late because it's already have been used).

Hm. Is my question in #11 deliberately ignored? ;)

Once more, shorter: How do you guys plan to swap out e.g. the config service definition, before the system attempts to get the service?

(PhpStorage cannot be edited, since it actively prevents edits. Global variables cause all kinds of pain and hinder testability.)

@sun Your proposal of having pre bootstrap services is I think the right way to go, but I'd call them "core bundle's services" which could ideally be overridable using either another config file (Yaml or PHP or anything else, since SF allows us to use all that) or another bundle.

EDIT: Actually I re-read your proposal, which is to have two DICs, one which is not compiled but built using a Yaml file, and another which is the one being compiled. I think this is bad and adds too much complexity.

If we leave out the compiled DIC, needing the database to load bundle might be some kind of circular dependency because we need to 1) load core bundle 2) build the DIC in order to bootstrap and use the database 3) load all other bundles that will alter the DIC (sadly too late because it's already have been used).

Yes even if we move modules to CMI then there's still going to be a circular dependency (assuming CMI is in the DIC) and it makes me uncomfortable.

The issue is that we want to register stuff into the DIC, and the registration process itself depends on services registered to the DIC.

If we want people to be able to dynamically add and remove bundles from with Drupal (i.e. via enabling/disabling modules but really via any other mechanism that's not hand-editing a PHP or YAML file somewhere), then that's going to be there in some sense. I'd really like to be removing circular dependencies from core rather than adding them at this point, so I'm bumping this to critical.

I like @sun idea about 2 phase bootstrap, my vision on this is that Bundles should contain only code that could work without "module.inc" so applicable for early bootstrap.
A good example is #1161486: Move IP blocking into its own module module should work different depending on database class availability so could fire on early bootstrap and also on full.
So having a bundle-set-for-bootstrap that uses a pre-compiled container is a good idea, this can load a module system if it needed to serve a current request.

Refiling.

> Move these things (the list of module locations and the container class name) to not be stored in pluggable backends.

Use drupal_php_storage for these because it is not relying on anything (like a database) and is fairly fast (definitely faster than a YAML file parsing).

Trying if the old one is suffering from the race conditions as well:

#29: 1784312.unbork-bootstrap.29.patch queued for re-testing.

#29: 1784312.unbork-bootstrap.29.patch queued for re-testing.

It does, before it had only 1 fail, now it has much more, several exceptions and also a warning that mkdir already exists.

This is a bot-only patch that contains #1708692-79: Bundle services aren't available in the request that enables the module inside as multi-patch.

Some changes in this patch look suspicious to me. As agreed on in #1831350: Break the circular dependency between bootstrap container and kernel, I demand that the essential DrupalKernel test coverage from #1774388: Unit Tests Remastered™ is committed first. Afterwards, we can happily revise DrupalKernel in whatever way we want.

Add class_loader and php_storage as synthetic services and switch php storage to PhpStorageFactory (poor drupal_php_storage function, only lived three months).

Moral of the story: before posting a patch run a WebTestBase too, I only ran unittestbase based ones...

Not so minor change: php storage consumers now keep their own objects instead of static or static-like cache (yuck).

sun: please be careful with words like 'I demand' (see #42). You don't have special privileges to demand things. It goes against our collaborative nature. It's really important that we maintain the right culture. Thanks.

#51: 1784312_51.patch queued for re-testing.

Well, we no longer merge in the bootstrap container. Nor we need to IMO. Just leave the kernel alone with this test.

Attached are some nit-pickitty observations from reading through the patch in #55. Perhaps these can be included if a re-roll is performed?

+++ b/core/lib/Drupal/Component/PhpStorage/PhpStorageFactory.phpundefined
@@ -0,0 +1,58 @@
+/**
+ * @file
+ * Definition of Drupal\Component\PhpStorage\PhpStorageFactory.

My understanding from [#1354] is that this should be 'Contains' instead of 'Definition of'. Same elsewhere in the patch.

+++ b/core/lib/Drupal/Component/PhpStorage/PhpStorageFactory.phpundefined
@@ -0,0 +1,58 @@
+   *
+   * @param $name
+   *   The name for which the storage controller should be returned. Defaults to
+   *   'default'. The name is also used as the storage bin if one is not

Can we add a type hint here? 'string'?

+++ b/core/lib/Drupal/Core/Config/BootstrapConfigStorageFactory.phpundefined
@@ -0,0 +1,31 @@
+   *
+   * @return Drupal\Core\Config\StorageInterface
+   *   A configuration storage implementation.

This type hint should start with a '\'.

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -32,6 +32,50 @@ class CoreBundle extends Bundle
 
   public function build(ContainerBuilder $container) {
 
+    // Register active configuration storage.
+    $container

Appears that this method is missing a docblock.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -7,11 +7,10 @@
-use Drupal\Core\Cache\CacheBackendInterface;
+use Drupal\Component\PhpStorage\PhpStorageFactory;
 use Symfony\Component\ClassLoader\UniversalClassLoader;
-use Drupal\Core\Config\FileStorage;
+use Drupal\Core\Config\BootstrapConfigStorageFactory;
 use Drupal\Core\CoreBundle;
-use Drupal\Component\PhpStorage\PhpStorageInterface;
 use Symfony\Component\HttpKernel\Kernel;
 use Drupal\Core\DependencyInjection\ContainerBuilder;
 use Symfony\Component\Config\Loader\LoaderInterface;
@@ -59,6 +58,20 @@ class DrupalKernel extends Kernel implements DrupalKernelInterface {

Any reason why use statements are organized like this? Makes little sense to me.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -95,15 +129,14 @@ class DrupalKernel extends Kernel implements DrupalKernelInterface {
+   * @param bool $allow_dumping
+   *   (optional) FALSE to stop the container from being written to or read

Should include the default value like 'Defaults to TRUE.'.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -125,28 +158,44 @@ public function boot() {
   /**
    * Returns an array of available bundles.

Appears we are missing a @return directive. Not clear from just reading patch.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -292,12 +343,9 @@ protected function buildContainer() {
+    // Register the class loader as a synthetic service.
+    $container->register('class_loader', 'Symfony\Component\ClassLoader\UniversalClassLoader')->setSynthetic(TRUE);
     foreach ($this->bundles as $bundle) {
       $bundle->build($container);

Surprised to see this class defined as a constant. What happens if we want to use a different Class Loader?

+++ b/core/lib/Drupal/Core/DrupalKernelInterface.phpundefined
@@ -25,8 +25,6 @@
    * @param array $module_list
    *   The new list of modules.
-   * @param array $module_path
-   *   List of module paths, keyed by module name.
    */
-  public function updateModules(array $module_list, array $module_path = array());
+  public function updateModules(array $module_list, array $module_paths = array());

Appears we are missing a @param directive for $module_paths.

+++ b/core/lib/Drupal/Core/Template/TwigEnvironment.phpundefined
@@ -6,6 +6,7 @@
 
 namespace Drupal\Core\Template;
+use Drupal\Component\PhpStorage\PhpStorageFactory;

Could use a blank line before use declaration.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -713,11 +713,22 @@ protected function setUp() {
+    foreach ($variables as $name => $value) {
+      variable_set($name, $value);
+    }

This change makes the conversion of variables to state system harder. Perhaps include the conversion of those variables here? Or at least cross-reference the other issue here?

+++ b/core/modules/system/lib/Drupal/system/Tests/PhpStorage/PhpStorageTestBase.phpundefined
@@ -15,6 +16,19 @@
   /**
+   * @var \Drupal\Component\PhpStorage\PhpStorageFactory
+   */
+  protected $storageFactory;

Missing a one-line description.

> Surprised to see this class defined as a constant. What happens if we want to use a different Class Loader?

It's a synthetic service, it just sets expectations for the container, an object (of whatever classloader extending class we want) is set later. Added a comment to the container->set to make it easier to see.

> This change makes the conversion of variables to state system harder.

It's still search/replace of variable_set to state()->set

I addressed the rest and hopefully fixed the tests. Also, we get the useful feature of adding bundles from settings.php (possibilities include a SiteBundle for site specific things that are not module bound, perhaps a theme specific bundle or two w the parent etc). This was discussed at length w Crell.

I found a few little things. Beside that it looks fine, although I can only "parse" it as I'm not fully understanding what it does and why it's done like this.

diff --git a/core/lib/Drupal/Core/DrupalKernel.php b/core/lib/Drupal/Core/DrupalKernel.php
index 25c0a1a..e31ef2b 100644
--- a/core/lib/Drupal/Core/DrupalKernel.php
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -125,28 +158,47 @@ public function boot() {
   /**
    * Returns an array of available bundles.
+   *
+   * @retun array
+   *   The available bundles.
    */

Typo in @return.

diff --git a/core/lib/Drupal/Core/DrupalKernelInterface.php b/core/lib/Drupal/Core/DrupalKernelInterface.php
index d4c8b8b..bff9932 100644
--- a/core/lib/Drupal/Core/DrupalKernelInterface.php
+++ b/core/lib/Drupal/Core/DrupalKernelInterface.php
@@ -28,5 +28,5 @@
    * @param array $module_path
    *   List of module paths, keyed by module name.
    */
-  public function updateModules(array $module_list, array $module_path = array());
+  public function updateModules(array $module_list, array $module_paths = array());
 }

Change of parameter name should be in comment as well.

diff --git a/core/includes/install.core.inc b/core/includes/install.core.inc
index d5983b6..4391e27 100644
--- a/core/includes/install.core.inc
+++ b/core/includes/install.core.inc
@@ -1,6 +1,7 @@
 <?php
 
 use Drupal\Core\DrupalKernel;
+use Drupal\Component\PhpStorage\PhpStorageFactory;
 use Drupal\Core\Database\Database;
 use Drupal\Core\Database\Install\TaskException;
 use Drupal\Core\Language\Language;
@@ -1502,7 +1503,7 @@ function install_bootstrap_full(&$install_state) {
   module_list_reset();
 
   // Instantiate the kernel.
-  $kernel = new DrupalKernel('prod', FALSE, drupal_classloader());
+  $kernel = new DrupalKernel('prod', FALSE, drupal_classloader(), FALSE);
   $kernel->boot();
   drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
 }
 

I can't see why PhpStorageFactory is used here.

Rerolled.

This is such an awesome step towards cleaning up our bootstrap process. It all looks good to me, except:

+++ b/core/modules/system/lib/Drupal/system/Tests/Module/ClassLoaderTest.php
@@ -37,10 +37,13 @@ function testClassLoading() {
-    // Check twice to test an unprimed and primed system_list() cache.
+    // The first request after a module has been disabled will result in that
+    // module's namespace getting registered because the kernel registers all
+    // namespaces in the existing 'container.modules' parameter before checking
+    // whether the list of modules has changed and rebuilding the container.
     for ($i=0; $i<2; $i++) {
       $this->drupalGet('module-test/class-loading');
-      $this->assertNoText($expected, 'Autoloader does not load classes from a disabled module.');
     }
+    $this->assertNoText($expected, 'Autoloader does not load classes from a disabled module.');

Unless someone can explain how this is not a bug, can we at least add a @todo here to follow up on this?

Thanks.

I actually stumbled over that part, too.

Given the current state of plugin system discovery, which relies on the classloader's registry of namespaces, I do not think that it is a good idea to introduce that kind of "mystery." That's sorta guaranteed to cause havoc down the line for other, innocent core contributors.

Unfortunately not... Due to the issue referenced in #65, this detail of the proposed change here can realistically have terrible consequences.

There's no test being added here that proves it does not. (Rather the contrary; existing test coverage that ought to protect us from bad things from happening is adjusted in a way that seems to make the entire test obsolete, because it doesn't really test anything special anymore.)

This needs to go in so we can remove the classloader's reliance on namespaces and instead make it rely on kernel parameters.

Rewording: so we can remove the plugin system relying on the classloader storing the namespaces. Instead, the kernel / container can store them.

I just spent the last hour and a half or so staring at this patch. I think there's a variable name I could quibble about, and a sentence that requires an extra comma in it, but I won't tell you where they are because that might delay this patch getting committed. :-) This is a huge step forward, although admittedly not the last one, and I think unblocks a number of other issues either directly or indirectly.

I believe sun's concerns are unfounded. As chx notes, this lets us put the active module list and the active namespace list in the DIC, where they are 1) As fast as humanly possible to access; 2) decoupled from the actual classloader implementation itself. That is, far from having "terrible consequences" for the plugin/classloader dependency, it resolves it in a clean and performant fashion.

This is one step in the right direction. But, after this patch, the bootstrap reads:

require_once DRUPAL_ROOT . '/core/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_CONFIGURATION);
$kernel = new DrupalKernel('prod', FALSE, drupal_classloader());
$request = Request::createFromGlobals();
$response = $kernel->handle($request)->prepare($request)->send();

... which is still very problematic. The main issue is that the request is actually implicitly used by DRUPAL_BOOTSTRAP_CONFIGURATION to load the proper configuration folder. What we most likely want is something like this:

$request = Request::createFromGlobals();
$kernel = DrupalKernel::createFromRequest($request);
$response = $kernel->handle($request)->prepare($request)->send();

And in addition, we also want a way to create a kernel outside of a request (which is completely impossible right now, but necessary for, for example, Drush), with something like this:

$kernel = DrupalKernel::createFromURL($url);

You are right but that's a followup.

I never said the contrary. I'm fine with this as a step in the right direction.

#63: 1784312.unbork-bootstrap.63.patch queued for re-testing.

Just a reroll to keep up with HEAD putting back in the RTBC bin.

Since this is a touchy patch.

Eep.

This has catch written all over it. :)

I don't want to mess with the RTBC status of this issue, so instead, I posted a fix to the ClassLoaderTest regression to #1846376-1: Namespaces for disabled modules are registered during the first request after a module is disabled.

Didn't see anything much to complain about so I've gone ahead and committed/pushed this one. Hopefully we can continue to clean this all up but this feels like we got a bit further.

I'll unpostpone #1846376: Namespaces for disabled modules are registered during the first request after a module is disabled.

Sorry, for re-opening this, but this issue introduced a BootstrapConfigStorageFactory which I don't understand at all. It references $conf['drupal_bootstrap_config_storage'] but is the only place in core to do so. I also don't see that part discussed in this issue at all.

I'm hitting this in the D7 -> D8 upgrade path. BootstrapConfigStorageFactory is called from DrupalKernel::registerBundles which is called (a couple levels up in the stack) from DrupalKernel::boot() right in update_fix_d8_requirements(). Most importantly this is called before checking if settings.php exists. Excerpt:

  // Bootstrap the kernel.
  // Do not attempt to dump and write it.
  $kernel = new DrupalKernel('update', FALSE, drupal_classloader(), FALSE);
  $kernel->boot();

  // Check whether settings.php needs to be rewritten.
  $settings_exist = !empty($GLOBALS['config_directories']);

Therefore in BootstrapConfigStorageFactory the call to config_get_config_directory() happens with empty global $config_directories and everything blows up. I have no idea how the upgrade path even remotely works at this point (it doesn't for me locally). The reason I am not opening a new issue is that I am not able to make any sense at all of BootstrapConfigStorageFactory.

The attached patch resolves the fatal at least, but update.php still doesn't spit anything out.

Presumably we do not need the "avoid commit conflicts" tag on this reopened issue?

@tstoeckler would you mind opening a new bug report for this?

And if you do assign it to me...

Continued at #1943726: BootstrapConfigStorageFactory and update.