One Service Container To Rule Them All

I'm going to try to run a manual upgrade. I'm curious if this fixes some of the weird things I have been seeing.

Chx! You made my day. This removes all kinds of nasty stuff that happened during an upgrade. For the first time in weeks I was able to perform a smooth upgrade.

Tempted to mark this rtbc but I'm going to wait so others can have a look.

+++ b/core/includes/install.inc
@@ -421,6 +422,9 @@ function drupal_verify_profile($install_state) {
+  $kernel = new DrupalKernel('prod', FALSE, drupal_classloader(), FALSE);
+  $kernel->boot();

It looks like this makes all Drupal instances use the same kernel, whereas #1530756: [meta] Use a proper kernel for the installer is trying to give each front-end its own kernel. I'm not sure how well that works.

If we were to use the same kernel class, shouldn't we then have an alternate environment, "install", instead of "prod"? (Symfony full stack has prod, dev, and "test".) That would allow us to use different containers in different situations but with the same kernel, but allow them to overlap as needed.

Which, thinking about it, maybe that's all we need...?

This does not look bad, but I have a couple of larger concerns with regard to some of the changes:

+++ b/core/includes/bootstrap.inc
@@ -2274,6 +2286,18 @@ function _drupal_bootstrap_configuration() {
+function _drupal_bootstrap_kernel() {
...
+    $kernel = new DrupalKernel('prod', FALSE, drupal_classloader());

+++ b/core/update.php
@@ -448,20 +448,6 @@ function update_check_requirements($skip_warnings = FALSE) {
 drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);

As mentioned over in #1848998: Problems with update_module_enable(), I don't think we can boot a regular kernel in update.php.

At minimum, that kernel should probably use 'update' as environment identifier. But what's far more concerning is that update.php may very well not be able to boot a kernel with regular services + bundles at all, since service definitions/storages/schemata/namespaces/bundles/etc might have changed.

I actually think it would be better to leave the update.php changes out of this patch.

+++ b/core/includes/bootstrap.inc
@@ -2274,6 +2286,18 @@ function _drupal_bootstrap_configuration() {
+  if (empty($GLOBALS['kernel'])) {

+++ b/core/includes/install.core.inc
@@ -312,7 +315,11 @@ function install_begin_request(&$install_state) {
+  if ($install_state['settings_verified']) {
+    $kernel = new DrupalKernel('install', FALSE, drupal_classloader(), FALSE);
+    $kernel->boot();
+  }

@@ -1500,10 +1498,6 @@ function install_bootstrap_full(&$install_state) {
   drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);

This will boot two kernels in the later installer steps, one with 'install' and one with 'prod' environment — I don't see why that is needed?

What would make sense to me in terms of separate environments/kernels is to have an 'installer' environment kernel for the first/initial installer steps that are operating in a crippled environment.

Lastly, I'm not really fond of introducing a new, magic global $kernel state variable. This makes the bootstrap harder to follow and can easily have unintended consequences.

+++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.php
@@ -83,6 +83,9 @@ function writeable() {
   function deleteAll() {
+    if (!function_exists('file_unmanaged_delete_recursive')) {
+      include_once DRUPAL_ROOT . '/core/includes/file.inc';
+    }
     return file_unmanaged_delete_recursive($this->directory, array(__CLASS__, 'filePreDeleteCallback'));

These changes reveal that PhpStorage\FileStorage is not properly decoupled from Drupal yet, so it shouldn't actually be in Drupal\Component or have an appropriate class extension/override in Drupal\Core.

Let's make sure to create a dedicated issue for that.

+++ b/core/lib/Drupal/Core/HttpKernel.php
@@ -42,6 +42,9 @@ public function __construct(EventDispatcherInterface $dispatcher, ContainerInter
     public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQUEST, $catch = true)
     {
+        // @todo Remove this once everything in the bootstrap has been
+        //   converted to services in the DIC.
+        drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE);

Essentially what I mentioned above already, there's no guarantee that this call to drupal_bootstrap() will not bootstrap a new kernel - which would inherently mean a new service container with a new HttpKernel.

+++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumTest.php
@@ -43,9 +43,9 @@ class ForumTest extends WebTestBase {
   /**
-   * An array representing a container.
+   * An array representing a forum container.
    */
-  protected $container;
+  protected $forumContainer;

D'oh. :)

It might make sense to split this fix out into a separate issue ;)

Created #1849636: Refer to the forum container as 'forum container', not just 'container', in forum tests to break out the forum variable rename part.

That won't work, it needs to happen after the container has been built - why can't we leave it inside the kernel's boot() method?

#22 was in response to #20, not #21 - I still don't understand why it couldn't have been left where it was :-/

OK, chx explained to me in IRC that we sometimes want a kernel but do not want to bootstrap all the way, so I'm ok with the separate call to $kernel->boot() as a way around this. And it does in fact make things more consistent - seeing as we call boot() separately everywhere else we instantiate a kernel.

Note that #1798732: Convert install_task, install_time and install_current_batch to use the state system is much more pressing right now, because it unblocks a range of other features, whereas this patch does not.

Regardless of that, here's another review:

+++ b/core/includes/bootstrap.inc
@@ -2274,6 +2286,18 @@ function _drupal_bootstrap_configuration() {
+    $kernel = new DrupalKernel('prod', FALSE, drupal_classloader());

Note that all of the kernel instantiations are not respecting the constructor docs of DrupalKernel, which say that TRUE should be passed for the second $debug argument.

+++ b/core/includes/file.inc
@@ -558,7 +560,7 @@ function file_save_htaccess($directory, $private = TRUE) {
     // Private .htaccess file.
-    $htaccess_lines = "SetHandler Drupal_Security_Do_Not_Remove_See_SA_2006_006\nDeny from all\nOptions None\nOptions +FollowSymLinks";
+    $htaccess_lines = MTimeProtectedFastFileStorage::HTACCESS;

Thanks for trying to decouple the PhpStorage, but I don't think this change is correct — at least I can't see the relationship from file_save_htaccess() to MTimeProtectedFastFileStorage. The former is used for many other functionality that isn't related to PhpStorage.

We should create an issue to convert file.inc into a FileSystem component, so this dependency can be properly resolved.

+++ b/core/includes/install.core.inc
@@ -312,7 +315,11 @@ function install_begin_request(&$install_state) {
+  if ($install_state['settings_verified']) {
+    $kernel = new DrupalKernel('install', FALSE, drupal_classloader(), FALSE);

+++ b/core/includes/install.inc
@@ -421,6 +422,9 @@ function drupal_verify_profile($install_state) {
 function drupal_install_system() {
...
+  // Immediately boot a kernel to have real services ready.
+  $kernel = new DrupalKernel('install', FALSE, drupal_classloader(), FALSE);

I think that both of these should specify 'prod' as environment. The fact that they are instantiated from install.php is not relevant - instead, what matters is that these kernels are used to install a production environment, so whatever meaning the environment identifier has or will have, it should trigger the same effects that would be triggered for the prod environment.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
@@ -128,6 +129,11 @@ public function containerBuild($container) {
+    if (!$container->has('keyvalue')) {
+      $container
+        ->register('keyvalue', 'Drupal\Core\KeyValueStore\KeyValueFactory')
+        ->addArgument(new Reference('service_container'));
+    }

Under which circumstances would there be no keyvalue service? This looks odd to me, so we should make sure to document it in a comprehensible way (or remove it if it's not needed).

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -966,11 +965,13 @@ protected function tearDown() {
+    if (($container = drupal_container()) && $container->has('keyvalue')) {
+      $captured_emails = state()->get('system.test_email_collector') ?: array();

+++ b/core/modules/system/lib/Drupal/system/Tests/DrupalKernel/DrupalKernelTest.php
@@ -30,11 +30,6 @@ public static function getInfo() {
   function testCompileDIC() {
-    // Because we'll be instantiating a new kernel during this test, the
-    // container stored in drupal_container() will be updated as a side effect.
-    // We need to be able to restore it to the correct one at the end of this
-    // test.
-    $original_container = drupal_container();

@@ -113,8 +102,5 @@ function testCompileDIC() {
-    // Restore the original container.
-    drupal_container($original_container);
   }

These changes look uncomfortable to me. It looks like the container can no longer be trusted.

I don't understand why the patch is catering for a possibly non-existing keyvalue service, but not any other service. I.e., IF the container is not reliable anymore, then there is also no guarantee that there is e.g. a database service in it, which in turn would have serious consequences.

I wasn't able to review the latest patch and read latest comments yet, sorry.

However, before we further continue to implant "the kernel" each and everywhere and dock our entire architecture onto it, I think that the comment in #1849700-11: DrupalKernel's container.modules param should contain module filenames, not full namespace paths could use a serious and well-founded answer.

#1849636: Refer to the forum container as 'forum container', not just 'container', in forum tests is now commited.

Rebased and fixed a merge conflict.

While I do side with sun and beejeebus on 'prod' versus 'install' for the $env parameter, it's not something I feel strongly about. I think all of the other concerns raised here have been adequately addressed, so I reckon this is good to go.

This resolves a critical bug. In HEAD, the plugin manager explodes the upgrade path, and it's consequently impossible to run update.php with Views installed. This patch fixes that.

Is @katbailey's (and @sun's and @beejeebus') concern regarding $env covered by #1850438: Establish DrupalKernel argument good practices?

Oh, yes indeed it is. I should not have brought it up again - forget I said anything about that ;-)

bootstrap.inc scares me. :) I'm hoping it scares catch less. :D

+++ b/core/includes/bootstrap.inc
@@ -2407,91 +2431,31 @@ function drupal_get_bootstrap_phase() {
  * @param bool $rebuild
  *   (optional) Internal use only. Whether to enforce a rebuild of the container.
  *   Used by the testing framework to inject a fresh container for unit tests.
...
 function drupal_container(Container $new_container = NULL, $rebuild = FALSE) {

The $rebuild argument seems to be obsolete.

+++ b/core/includes/bootstrap.inc
@@ -2407,91 +2431,31 @@ function drupal_get_bootstrap_phase() {
- * @return Symfony\Component\DependencyInjection\Container
+ * @return Symfony\Component\DependencyInjection\Container|bool
  *   The instance of the Container used to set up and maintain object
- *   instances.
+ *   instances or FALSE if none exist yet.
...
-  static $container = NULL;
...
+  static $container = FALSE;

Any particular reason for why this defaults to FALSE instead of NULL?

The only caller that checks the value seems to be _drupal_bootstrap_kernel(), and it does not perform a strict type comparison, so FALSE == NULL.

+++ b/core/includes/file.inc
@@ -558,7 +560,7 @@ function file_save_htaccess($directory, $private = TRUE) {
     // Private .htaccess file.
-    $htaccess_lines = "SetHandler Drupal_Security_Do_Not_Remove_See_SA_2006_006\nDeny from all\nOptions None\nOptions +FollowSymLinks";
+    $htaccess_lines = MTimeProtectedFastFileStorage::HTACCESS;

This still looks incorrect to me. My earlier comment wasn't really meant to say that we need to fix the coupling of PhpStorage in this issue/patch. I'd rather prefer to go back to the previous/original change, and decouple/fix PhpStorage properly in a separate issue.

+++ b/core/includes/install.core.inc
@@ -288,6 +288,9 @@ function install_begin_request(&$install_state) {
+  // Check existing settings.php.
+  $install_state['database_verified'] = install_verify_database_settings();
+  $install_state['settings_verified'] = $install_state['config_verified'] && $install_state['database_verified'];

@@ -312,7 +315,11 @@ function install_begin_request(&$install_state) {
-  if (!$install_state['config_verified']) {
+  if ($install_state['settings_verified']) {
+    $kernel = new DrupalKernel('install', FALSE, drupal_classloader(), FALSE);
+    $kernel->boot();
+  }
+  else {
...
     $container = new ContainerBuilder();

@@ -1104,11 +1107,6 @@ function install_settings_form_submit($form, &$form_state) {
-  drupal_container(NULL, TRUE);

@@ -1500,10 +1498,6 @@ function install_bootstrap_full(&$install_state) {
-  // Instantiate the kernel.
-  $kernel = new DrupalKernel('prod', FALSE, drupal_classloader(), FALSE);

+++ b/core/includes/install.inc
@@ -421,6 +422,9 @@ function drupal_verify_profile($install_state) {
 function drupal_install_system() {
...
+  $kernel = new DrupalKernel('install', FALSE, drupal_classloader(), FALSE);

I do not see an indication in this issue that all possible situations/permutations of the interactive installer have been manually tested. However, that is required for larger installer bootstrap changes like this.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
@@ -129,6 +130,11 @@ public function containerBuild($container) {
+    if (!$container->has('keyvalue')) {
+      $container
+        ->register('keyvalue', 'Drupal\Core\KeyValueStore\KeyValueFactory')
+        ->addArgument(new Reference('service_container'));
+    }

Can we add a comment here that explains the conditional registration? (it's not easy to follow and understand why it is done)

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -985,7 +986,9 @@ protected function tearDown() {
     // Restore original globals.
-    $GLOBALS['theme_key'] = $this->originalThemeKey;
+    if (isset($this->originalThemeKey)) {
+      $GLOBALS['theme_key'] = $this->originalThemeKey;
+    }

This should be obsolete. #347988: Move $user->data into own table performed the necessary changes to prepareEnvironment() already.

Don't RTBC your own patch, please.

I can confirm the patch does its job on various situations. I updated and installed several times with the previously rtbc patch.

we seemingly agreed we prefer if to ?:

Sorry, but no. There's no reason to universally disallow or not use ternary operators anymore.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -819,7 +820,9 @@ protected function prepareEnvironment() {
-    $this->originalThemeKey = isset($GLOBALS['theme_key']) ? $GLOBALS['theme_key'] : NULL;
+    if (isset($GLOBALS['theme_key'])) {
+      $this->originalThemeKey = $GLOBALS['theme_key'];
+    }
     $this->originalTheme = isset($GLOBALS['theme']) ? $GLOBALS['theme'] : NULL;

@@ -985,7 +988,9 @@ protected function tearDown() {
-    $GLOBALS['theme_key'] = $this->originalThemeKey;
+    if (isset($this->originalThemeKey)) {
+      $GLOBALS['theme_key'] = $this->originalThemeKey;
+    }
     $GLOBALS['theme'] = $this->originalTheme;

I can't see what's wrong with the existing code. It is simple and consistent. Can we just leave those lines alone?

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -966,11 +967,13 @@ protected function tearDown() {
+    if (($container = drupal_container()) && $container->has('keyvalue')) {

Why do we check drupal_container() here, instead of $this->container?

Minor point:

   // This override is reverted as soon as the config directory has been set up
   // successfully.
   // @see drupal_install_config_directories()
-  if (!$install_state['config_verified']) {
+  if ($install_state['settings_verified']) {

Is that code comment and @see really correct after the corresponding code change?

Looks to me like it needs to refer to the writing of a valid settings.php in install_settings_form_submit() instead.

I only have minor comments, looks pretty close to RTBC to me.

+++ b/core/includes/bootstrap.incundefined
@@ -128,39 +129,44 @@
 
 /**
- * Second bootstrap phase: try to serve a cached page.
+ * Second bootstrap phase, initalize a kernel.
  */
-const DRUPAL_BOOTSTRAP_PAGE_CACHE = 1;
+const DRUPAL_BOOTSTRAP_KERNEL = 1;

I dread to think what's happened to page caching performance in the past year, but it's not introduced by this patch.

+++ b/core/includes/bootstrap.incundefined
@@ -2407,91 +2431,28 @@ function drupal_get_bootstrap_phase() {
-  if (!isset($container)) {
-    // This is only ever used by the installer and by run-tests.sh.
-    // @todo Remove this entire section once these have been converted to use a

This hunk is great!

+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.phpundefined
@@ -129,6 +130,20 @@ public function containerBuild($container) {
+      // service but when a test is happy with the superminimal container put
+      // together here it seems to need a keyvalue storage still -- that's why

Only seems to?

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -966,11 +967,13 @@ protected function tearDown() {
+    if (($container = drupal_container()) && $container->has('keyvalue')) {
+      $captured_emails = state()->get('system.test_email_collector') ?: array();
+      $emailCount = count($captured_emails);
+      if ($emailCount) {
+        $message = format_plural($emailCount, '1 e-mail was sent during this test.', '@count e-mails were sent during this test.');
+        $this->pass($message, t('E-mail'));

Oh is this why?

This will make my work on the ExtensionHandler patch so much easier - let's get it in! :-)

Alright. Committed/pushed to 8.x.

I've created #1864292: Installation in non-English language fails which is probably a bug introduced by this patch. Any help is appreciated.