The patch to be committed is at #85.

Summary:

We can't move the bootstrap DIC into the normal DIC because the normal DIC needs the list of modules which currently depend on keyvalue which in turn depend on the database which requires the DIC to be loaded... Aaaaargh! And then we have cache.

To break this chain, the proposed solution is to have the DrupalKernel read the list of module straight from the YAML file storing the module list (a followup to make this pluggable is filed and being worked on actively). Multiple web frontends are taken care of by storing the module list used to build the container in the container and checking it for freshness after reading the container.

Files: 
CommentFileSizeAuthor
#92 kernel.dic_.92.patch42.59 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#92 interdiff.txt13.94 KBsun
#85 1831350_84.patch38.54 KBchx
PASSED: [[SimpleTest]]: [MySQL] 47,995 pass(es).
[ View ]
#85 interdiff.txt2.31 KBchx
#83 kernel.dic_.83.patch38.35 KBsun
FAILED: [[SimpleTest]]: [MySQL] 48,009 pass(es), 3 fail(s), and 4 exception(s).
[ View ]
#83 interdiff.txt2.74 KBsun
#80 1831350_68.patch37.84 KBchx
PASSED: [[SimpleTest]]: [MySQL] 48,018 pass(es).
[ View ]
#70 interdiff.txt2.08 KBchx
#70 1831350_70.patch38.51 KBchx
FAILED: [[SimpleTest]]: [MySQL] 47,992 pass(es), 4 fail(s), and 5 exception(s).
[ View ]
#68 1831350_68.patch37.84 KBchx
PASSED: [[SimpleTest]]: [MySQL] 47,981 pass(es).
[ View ]
#68 interdiff.txt434 byteschx
#65 1831350_64.patch37.89 KBchx
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#65 interdiff.txt932 byteschx
#61 1831350_60.patch37.73 KBchx
PASSED: [[SimpleTest]]: [MySQL] 47,999 pass(es).
[ View ]
#61 interdiff.txt1.9 KBchx
#60 kernel-dic-unit.interdiff.59.txt5.16 KBsun
#57 kernel-dic-unit.interdiff.txt3.46 KBsun
#54 1831350_54.patch37.6 KBchx
PASSED: [[SimpleTest]]: [MySQL] 47,999 pass(es).
[ View ]
#54 interdiff.txt18.23 KBchx
#44 1831350_44.patch33.6 KBchx
PASSED: [[SimpleTest]]: [MySQL] 47,859 pass(es).
[ View ]
#44 interdiff.txt1.26 KBchx
#42 1831350_42.patch33.6 KBchx
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#42 interdiff.txt9.66 KBchx
#37 1831350_37.patch31.88 KBchx
PASSED: [[SimpleTest]]: [MySQL] 47,856 pass(es).
[ View ]
#37 interdiff.txt5.84 KBchx
#36 1831350_36.patch31.88 KBchx
PASSED: [[SimpleTest]]: [MySQL] 47,860 pass(es).
[ View ]
#34 1831350_34.patch31.86 KBchx
FAILED: [[SimpleTest]]: [MySQL] 47,804 pass(es), 54 fail(s), and 243 exception(s).
[ View ]
#32 1831350_32.patch29.11 KBchx
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#23 1831350_22.patch22.19 KBchx
PASSED: [[SimpleTest]]: [MySQL] 47,616 pass(es).
[ View ]
#23 interdiff.txt1.09 KBchx
#21 1831350_20.patch22.08 KBchx
PASSED: [[SimpleTest]]: [MySQL] 47,611 pass(es).
[ View ]
#21 interdiff.txt720 byteschx
#15 interdiff.txt1.86 KBchx
#15 1831350_15.patch22.08 KBchx
FAILED: [[SimpleTest]]: [MySQL] 47,572 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#14 1831350_14.patch20.64 KBchx
PASSED: [[SimpleTest]]: [MySQL] 47,586 pass(es).
[ View ]
#14 interdiff.txt2.63 KBchx
#13 1831350_13.patch23.02 KBchx
PASSED: [[SimpleTest]]: [MySQL] 47,588 pass(es).
[ View ]
#13 interdiff.txt3.88 KBchx
#11 1831350_11.patch19.84 KBchx
FAILED: [[SimpleTest]]: [MySQL] 47,572 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#7 1831350.bootstrap_in_php.7.patch20.5 KBchx
FAILED: [[SimpleTest]]: [MySQL] 47,462 pass(es), 20 fail(s), and 755 exception(s).
[ View ]
#5 1831350.bootstrap_in_php.5.patch31.07 KBchx
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#3 1831350.bootstrap_in_php.3.patch18.93 KBchx
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
bootstrap_in_php_easier_to_review.patch15.47 KBchx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch bootstrap_in_php_easier_to_review.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
bootstrap_in_php.patch18.79 KBchx
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch bootstrap_in_php.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

chx’s picture

Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch, bootstrap_in_php_easier_to_review.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new18.93 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1831350.bootstrap_in_php.3.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new31.07 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Moving drupal_system_listing to system.module.

Status:Needs review» Needs work

The last submitted patch, 1831350.bootstrap_in_php.5.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new20.5 KB
FAILED: [[SimpleTest]]: [MySQL] 47,462 pass(es), 20 fail(s), and 755 exception(s).
[ View ]

WE GOT TWIG IN CORE, WE GOT TWIG IN COREerm that's a different issue...

anyways. Here we go. I managed to run at least one test successfully. Eventually I can see the entirety of system_list becoming a call to system_list_load. It's not far from that...

chx’s picture

Note that I talked to effulgentsia and the understanding is that although we just added to the compiled DIC a mechanism to cope with enabling modules on a multiple webheads scenario that is not feasible anyways and we can remove it, rename the class to what the cache key is now -- which will remove the last dependency on cache.

Status:Needs review» Needs work

The last submitted patch, 1831350.bootstrap_in_php.7.patch, failed testing.

chx’s picture

Title:Break the bootstrap DIC circular dependency by moving to PHP» Break the bootstrap DIC circular dependency by going bare metal
Status:Needs work» Active

Disregard the patch above. Discussed with catch and

  1. The bundle boot method won't be supported. We can't find anything that implemented it, seems like Symfony cruft and if we want to move our hook_boot to anything we can use an event. (Better would be to make that hook die).
  2. When the container is not there, read the system.modules YAML file, scan the filesystem, prime the classloader and rebuild like that.
chx’s picture

Status:Active» Needs review
StatusFileSize
new19.84 KB
FAILED: [[SimpleTest]]: [MySQL] 47,572 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Hey bot, I just met you, and this is crazy, but here's my patch, so test it, maybe?

I talked to davidstrauss and msonnabaum tonight and we are considering adding a tombstone to the config system and the container both which we will read after the container is read (and thusly the config system is ready) and rebuild immediately if it doesn't match. This will allow for enabling modules on multiple web frontends.

Status:Needs review» Needs work

The last submitted patch, 1831350_11.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new3.88 KB
new23.02 KB
PASSED: [[SimpleTest]]: [MySQL] 47,588 pass(es).
[ View ]

Actually making that test pass is exactly what's needed for the multi-head operations. Also added bundleClasses to the container because effulgentsia yesterday suggested using that for faster module builds. Probably a followup but the groundwork is there.

chx’s picture

StatusFileSize
new2.63 KB
new20.64 KB
PASSED: [[SimpleTest]]: [MySQL] 47,586 pass(es).
[ View ]

I have reverted the testbase changes and added some more doxygen. Hopefully still passes.

chx’s picture

StatusFileSize
new22.08 KB
FAILED: [[SimpleTest]]: [MySQL] 47,572 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
new1.86 KB

Actually the parent profile was supported but not loaded, fixed.

Status:Needs review» Needs work

The last submitted patch, 1831350_15.patch, failed testing.

chx’s picture

Status:Needs work» Needs review

#15: 1831350_15.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1831350_15.patch, failed testing.

Gábor Hojtsy’s picture

Postponed #1831846: Help block is broken with language path prefixes on this one because arg() cannot use current_path() earlier in the bootstrap now, resulting in certain help text fails.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new720 bytes
new22.08 KB
PASSED: [[SimpleTest]]: [MySQL] 47,611 pass(es).
[ View ]

That this passes on PHP 5.4 and breaks on 5.3 only totally,absolutely threw me off track despite it's such a simple fix.

Status:Needs review» Needs work

The last submitted patch, 1831350_20.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new1.09 KB
new22.19 KB
PASSED: [[SimpleTest]]: [MySQL] 47,616 pass(es).
[ View ]

This IMO is more correct but who knows the dark depths of simpletest...

chx’s picture

Priority:Normal» Critical

Sorry but this blocks so much stuff now that I feel it's critical. So much bootstrap cleanup becomes possible when the first thing you do is have a dependency injection container.

katbailey’s picture

Status:Needs review» Needs work
+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -50,6 +51,18 @@ class DrupalKernel extends Kernel implements DrupalKernelInterface {
+  protected $dump;

This is ambiguous - I'd prefer something like "useCompiledContainer".

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -93,18 +96,63 @@ public function init() {
+        drupal_get_filename('module', $module, $module_data[$module]->uri);

This is just to warm the cache, right? I'd leave it out - this only happens on rebuilds and we want to avoid calls to procedural functions in The New Code (tm). In this case the function in question will soon be replaced by a method in the ExtensionHandler, which as I mention below will ideally get the module info passed into it, including filenames.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -93,18 +96,63 @@ public function init() {
+        drupal_classloader_register($module, dirname($module_data[$module]->uri));

Maybe we should inject the classloader into the kernel?

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -174,7 +238,11 @@ protected function initializeContainer() {
+    $container->setParameter('container.modules', array_keys($this->moduleList));

Once we have an ExtensionHandler service we can actually pass this parameter to it - so the more information it includes the better, but I guess I can fix that up in the ExtensionHandler patch itself when the time comes.

Other than those minor points and some missing doc blocks in the new classes, this is the most awesome patch I've seen in a long time :-D

catch’s picture

Discussed this in person with chx at BADCamp, patch looks great now.

Apart from a couple of comment nitpicks I only have one question at the moment:

+++ b/core/lib/Drupal/Core/SystemListingInfo.phpundefined
@@ -0,0 +1,54 @@
+      if (file_exists($info_file = dirname($file->uri) . '/' . $file->name . '.info')) {
+        // Get the .info file for the module or theme this file belongs to.
+        $info = drupal_parse_info_file($info_file);
+
+        // If the module or theme is incompatible with Drupal core, remove it
+        // from the array for the current search directory, so it is not
+        // overwritten when merged with the $files array.
+        if (isset($info['core']) && $info['core'] != DRUPAL_CORE_COMPATIBILITY) {
+          unset($files_to_add[$file_key]);
+        }
+      }

The non .info implementation of this doesn't (and can't) do this check, so then if there is an incompatible module won't it register the wrong location? I'm not sure we really need to support that any longer though.

catch’s picture

sun’s picture

Title:Break the bootstrap DIC circular dependency by going bare metal» Break the circular dependency between bootstrap container and kernel
Issue tags:+symfony, +Dependency Injection (DI), +kernel-followup

This definitely looks interesting, and I very much agree that this circular dependency needs to be resolved.

I looked through the patch, but I did not really understand the proposed changes.

Could you clarify what exactly this patch changes and which architectural problems are solved (and how)?
(if possible, elaborate a bit more, as this is a complex problem space; i.e., before vs. after + which problems will be solved and which still remain)

One thing I noticed:
There does not seem to be change to module_enable() contained in this patch, which apparently reboots the kernel currently for every module that gets enabled. That's our worst offender in terms of container interdependencies right now.

I'm mentioning this, because the current DrupalKernel boot assumes that it is allowed to refresh/rebuild drupal_container() with a completely new one. But that's actually a major problem, since unit tests are setting up a custom container with a range of essential service overrides, and the moment drupal_container() gets rebuilt from scratch (through the kernel reboot), all of those service overrides are gone and the system blows up.

If possible, I'd highly appreciate it if we could get the follow-up patch from #1774388-88: Unit Tests Remastered™ in first. That is, because it enables the kernel for DrupalUnitTestBase tests, and contains some essential test coverage for that, in order to prove that the kernel can be booted and rebooted from within a unit test. Due to the current state of kernel affairs, it has to work around the container reset problems caused by the kernel. My expectation for this patch here would be that those ugly workarounds would be removed with this patch, so as to demonstrate and prove that the horrible circular kernel/container dependency is indeed resolved.

I know that might be a lot to ask for, but my stance on the entire new bootstrap architecture generally is still the same: We should actually have implemented the DrupalUnitTestBase + its test from over there before we introduced the container/kernel, since that is what essentially proves that we are actually able to handle the dependencies correctly. So I'd really appreciate if we could get that in first, and afterwards, prove with this patch here that we can remove a ton of ugly workarounds.

Aside from that, I have a couple of implementation remarks, which are important from my perpective, but I'm not sure whether it is too early to mention them, so I only note them quickly:

- The system should not make the assumption that config is stored in files at any time. The config service has been designed to be swappable, and config may very well live in the database or somewhere else.

- The current drupal_system_listing() scans the files of the parent-site/test-runner profile within a test run. This patch here changes that to load the system.module config file of the test-runner. That involves two assumptions that are not right from my perspective:
1) The scan is only supposed to find available extensions in the parent site's installation profile; the list of enabled modules (extensions) is a completely different list and is not relevant in any way.
2) The system should never assume that there is an installed parent site (as we will remove that dependency).

- registerBundles() contains code to register a module's filepath and classloader namespace, but that is system_list()'s sole responsibility. I don't think the kernel should have any business with that.

Sorry, this post became longer than I originally intended. I hope you can take this as constructive feedback for the proposed changes and don't unassign yourself again. I think the general direction of this patch seems to make sense and after working on those unit tests, I certainly agree that we have a huge problem with the kernel boot right now. If anything is unclear or if there's disagreement with anything in the above, I'm happy to discuss.

katbailey’s picture

Could you clarify what exactly this patch changes and which architectural problems are solved (and how)?

This patch is the first step towards solving the problem described here: #1784312: Stop doing so much pre-kernel bootstrapping - I updated the issue summary there to explain exactly what we're aiming for.

We should actually have implemented the DrupalUnitTestBase + its test from over there before we introduced the container/kernel, since that is what essentially proves that we are actually able to handle the dependencies correctly

We absolutely should not be coding around our testing framework limitations. This bootstrap mess has been put off long enough and is delaying progress on other important issues - it makes no sense to hold it up on your unit tests remastered issue.

The system should not make the assumption that config is stored in files at any time

Heyrocker was perfectly fine with this idea. We need the enabled modules info accessible from somewhere that does not need the container.

registerBundles() contains code to register a module's filepath and classloader namespace, but that is system_list()'s sole responsibility. I don't think the kernel should have any business with that

The namespaces need to be registered in order to register bundle classes - system_list() will be going away and in its place will be a method in an ExtensionHandler service (which gets registered to the container with this list as a parameter).

chx’s picture

I am glad for the review actually and I am not unassigning because it's not unreasonable perhaps misdirected which just calls for adding more comments to the patch.

The scan is only supposed to find available extensions in the parent site's installation profile; the list of enabled modules (extensions) is a completely different list and is not relevant in any way. <= check the patch, please. It uses the list of enabled modules only to figure out what's the parent profile:

<?php
      $modules_scanner
= new SystemListing(array_keys(array_intersect_key($module_list, $all_profiles)));
     
$module_data = $all_profiles + $modules_scanner ->scan('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.module$/', 'modules');
      foreach (
$this->moduleList as $module => $weight)
?>

See, $module_list is just intersected with all the profiles to get the actually relevant profiles, 1 or 2.

The system should never assume that there is an installed parent site <= we will work with the current assumptions whenever they change. Right now there is one, it's easy to change the behavior and I won't even try to change it to accomodate for some future change that might or might not happen.

> contains code to register a module's filepath and classloader namespace, but that is system_list()'s sole responsibility. <= #1833592: [META] The road out of module build circular dependency hell is about changing system_list so that it can run solely based on the container. However, that's the future. For now, there's a review here which asks for removing the registration of a module's filepath and, as I said in the above paragraph I will do that and until such time that meta gets solved -- we do not accomodate for future patches.

sun’s picture

FYI:

+++ b/core/lib/Drupal/Core/SystemListing.php
+        $dir_iterator = new \RecursiveIteratorIterator(
+          new \RecursiveDirectoryIterator($dir, \RecursiveDirectoryIterator::SKIP_DOTS)
+        );

This code runs into the same trap as I've outlined here:
#1451320-9: Evaluate Symphony2's Finder Component to simplify file handling

You can find a proper RecursiveDirectoryIterator filter implementation here:
#1833732-9: Find a way to skip ./config directories without skipping core/modules/config directory in drupal_system_listing()

However, the performance impact/decrease of the iterator-based approach is so heavy that I'd strongly recommend to detach and unblock this patch here from that topic and replace the code with the limited needed equivalent parts of the current file_scan_directory().

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new29.11 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Thanks for the warning, iterator nuked. This fails the http/https tests, I wonder whether it fails anything else.

Status:Needs review» Needs work

The last submitted patch, 1831350_32.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new31.86 KB
FAILED: [[SimpleTest]]: [MySQL] 47,804 pass(es), 54 fail(s), and 243 exception(s).
[ View ]

And what about this?

Status:Needs review» Needs work

The last submitted patch, 1831350_34.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new31.88 KB
PASSED: [[SimpleTest]]: [MySQL] 47,860 pass(es).
[ View ]

module_disable, eh.

chx’s picture

StatusFileSize
new5.84 KB
new31.88 KB
PASSED: [[SimpleTest]]: [MySQL] 47,856 pass(es).
[ View ]

Docs added, unnecessary code removed. Should be ready.

katbailey’s picture

Status:Needs review» Reviewed & tested by the community

OK, assuming this comes back green - I am happy to RTBC this. Awesome work, chx!

katbailey’s picture

Status:Reviewed & tested by the community» Needs review

Oops, got ahead of myself there, should really wait for the bot...

Status:Needs review» Needs work

The last submitted patch, 1831350_37.patch, failed testing.

sun’s picture

Status:Needs work» Needs review

Finally came around to review this some more in-depth. I have a lot of questions, which I think need to be answered in code comments, to clarify what is going on.

A before/after comparison, or at least a more elaborate explanation of how the pieces are wired together would still be appreciated. I can perfectly understand that you guys dived deeply into this space and everything might appear crystal clear to you, but I can assert that this is not the case for anyone else. ;)

Next to the remarks and questions below, there are also a lot of coding style and coding standards issues in the patch, of which the most should be more than obvious, so I did not comment on those.

+++ b/core/includes/common.inc
@@ -5159,81 +5160,8 @@ function drupal_cron_cleanup() {
+  $listing = new SystemListingInfo();

Is there any reason why this isn't registered as a service?

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -36,11 +38,11 @@ class DrupalKernel extends Kernel implements DrupalKernelInterface {
+   * An array of module data as returned by system_rebuild_module_data().
...
+  protected $moduleData;

How and when is system_rebuild_module_data() called from DrupalKernel?

Apparently I must be blind, because I don't see it in the code — if that is the case, what is $moduleData then? Somewhere in the code, I've seen a stdClass object with a single $uri property — what's that for?

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -60,26 +76,20 @@ class DrupalKernel extends Kernel implements DrupalKernelInterface {
+   * @param \Symfony\Component\ClassLoader\UniversalClassLoader $class_loader
+   *   (optional) A classloader object. Required when $storage is set and
+   *   also when $module_list is not set.
+   * @param \Drupal\Component\PhpStorage\PhpStorageInterface $storage
+   *   (Optional) An object handling PHP load and save.
    * @param array $module_list
    *   (optional) The array of enabled modules as returned by module_list().

Can we clarify what the effect of passing and omitting of these arguments is? We need to document what the flow is; i.e., how this is supposed work, and how it is not supposed to work.

All of the following questions should be answered by the phpDoc:

- How is the class loader used and involved? Is it polluted, and if so, with what, and under which conditions?

- What happens if I do not pass a class loader?

- How is the PhpStorage used? Is it only used for reading or is also written to it?

- What happens if I do not pass a PhpStorage?

- Do I have to pass a class loader if I pass a PhpStorage?

- How is the module list used?

- What happens if I do not pass a module list?

- Do I have to pass a class loader and/or PhpStorage when I pass a module list?

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -93,18 +103,60 @@ public function init() {
+      $module_list = $storage->read('system.module');
+      $module_list = $module_list['enabled'];
+      $this->moduleList = $module_list;

@@ -126,33 +181,49 @@ public function updateModules($module_list) {
+      $module_list = array_keys($this->moduleList ?: $this->container->get('config.factory')->get('system.module')->load()->get('enabled'));

I'm afraid, this is incompatible with (unit) testing. The currently existing DrupalKernel tests do not represent the full picture, which is why it's easy to meet their expectations.

I do not understand why we need the hard-coded dependency on config files.

Can we call module_list() in these cases?

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -93,18 +103,60 @@ public function init() {
+      // Find filenames and prime the classloader. First, we need to add
+      // profiles because modules migh tbe inside those.
+      $profiles_scanner = new SystemListing();
+      $all_profiles = $profiles_scanner ->scan('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.profile$/', 'profiles');
+      $profiles = array_keys(array_intersect_key($this->moduleList, $all_profiles));
+      // If a module is within a profile directory but specifies another
+      // profile for testing, it needs to be found it in the parent profile.
+      if (($parent_profile_config = $storage->read('simpletest.settings')) && isset($parent_profile_config['parent_profile']) && $parent_profile_config['parent_profile'] != $profiles[0] ) {
+        // In case both profile directories contain the same extension, the
+        //  actual profile always has precedence.
+        array_unshift($profiles, $parent_profile_config['parent_profile']);
+      }
+      $modules_scanner = new SystemListing($profiles);
+      $this->moduleData = $all_profiles + $modules_scanner ->scan('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.module$/', 'modules');

I do not understand why this is not part of the SystemListing class? It looks like duplicated code?

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -93,18 +103,60 @@ public function init() {
+    $namespaces = $this->classLoader->getNamespaces();
+    foreach ($this->moduleList as $module => $weight) {
+      $namespace = 'Drupal\\' . $module;
+      if (!isset($namespaces[$namespace]) && isset($this->moduleData[$module])) {
+        $this->classLoader->registerNamespace($namespace, dirname(DRUPAL_ROOT . '/' . $this->moduleData[$module]->uri) . '/lib');
+      }

Why would we have to re-register module namespaces in the class loader? Under which conditions would the namespace not be registered yet?

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -126,33 +181,49 @@ public function updateModules($module_list) {
+    if (!empty($GLOBALS['drupal_test_info']['test_run_id'])) {
+      $parts[] = $GLOBALS['drupal_test_info']['test_run_id'];
+    }
+    elseif ($prefix = drupal_valid_test_ua()) {
+      $parts[] = $prefix;
+    }

Normally, drupal_valid_test_ua() should be sufficient?

+++ b/core/lib/Drupal/Core/SystemListing.php
@@ -0,0 +1,169 @@
+class SystemListing {

+++ b/core/lib/Drupal/Core/SystemListingInfo.php
@@ -0,0 +1,61 @@
+class SystemListingInfo extends SystemListing {

The difference between these two is not clear to me. What is the difference?

Why is DrupalKernel not using SystemListingInfo?

chx’s picture

StatusFileSize
new9.66 KB
new33.6 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Thanks, excellent questions, allowed me to make the constructor much better. $module_list is gone, that was only used by the test and it can use updateModules just fine. The classloader became mandatory, it's not possible to reliably boot without one -- if the container needs a rebuild we need a classloader to find the module bundles.

Edit: in general, don't be fooled by the current code runs in a DRUPAL_BOOTSTRAP_CODE state. The goal is to be able to boot the kernel with DRUPAL_BOOTSTRAP_CONFIGURATION and also to merge the two containers.

> there are also a lot of coding style and coding standards issues in the patch, of which the most should be more than obvious,

Should be but I did a review run with katbailey and neither of us caught them. Edit: although now I see I added a few almost accidentally :)

>Is there any reason why this isn't registered as a service?

Added a comment.

> I do not understand why we need the hard-coded dependency on config files.

Given that every storage service we have depends by default on the database which needs to be in the container, there are not a lot of choices. You can use the PHP storage or parse flat files. This patch relies on both. There's nothing else I am aware of. And, the only fast one is the PHP storage so we try to rely on that.

> I'm afraid, this is incompatible with (unit) testing. The currently existing DrupalKernel tests do not represent the full picture, which is why it's easy to meet their expectations.

Great, let's file a follow-up then to write tests and fulfill those expectations. Note, however, that unit tests can inject a module list to be used via updateModules just fine as the Kernel tests now do. If you want to use / test the compiled DIC and the module change rebuild I recommend mocking the configuration service.

> I do not understand why this is not part of the SystemListing class? It looks like duplicated code?

Very little duplicated code with SystemListingInfo yes but see on that class below (hint: it's not going to last). registerBundles first uses SystemListing to find the directories where profiles $all_profiles array in case a .profile wants to register a bundle. Added comments.

> Why would we have to re-register module namespaces in the class loader?

We specifically never re-register, there's a condition protecting against that. Added comments and refactored for speed.

> Normally, drupal_valid_test_ua() should be sufficient?

Added a comment.

> The difference between these two is not clear to me. What is the difference? Why is DrupalKernel not using SystemListingInfo?

Because SystemListingInfo fires hooks! You can't fire hooks here! That's madness, currently the only reason that works because the lists static in system_list is directly populated before theme rebuild is called -- if it'd go into a separate variable and at the end assigned to the lists static, the theme discovery and the info alter hook would immediately break. The point of the whole exercise is to avoid any hooks like fire. Very, very soon this will be loaded well before the module system is loaded. If you follow the chain of the issues I outlined in #1833592: [META] The road out of module build circular dependency hell you will see that SystemListingInfo is heading for the chopping block. Oh, and it's slow to parse info files when all we are worried about is the locations of modules and classes. (If you remember I tried this a lot less clean already by keeping only the file parsing parts of _system_rebuild_module_data in that function and moving the alter elsewhere when we were removing the {system} table.)

Status:Needs review» Needs work

The last submitted patch, 1831350_42.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new1.26 KB
new33.6 KB
PASSED: [[SimpleTest]]: [MySQL] 47,859 pass(es).
[ View ]

D'oh, I ran the Kernel tests but forgot to run the installer.

sun’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -93,28 +106,90 @@ public function init() {
   public function registerBundles() {
...
+      $storage = new FileStorage(config_get_config_directory());
+      $module_list = $storage->read('system.module');
...
+  protected function moduleData($module) {
...
+      $storage = new FileStorage(config_get_config_directory());

I still dislike the hard-coded file storage. We've spent an enormous amount of time to design the config system in a way to make the storage swappable, and thus, it really throws me off to see all of that hard work being kicked out of the window here. :(

What I don't get is why we aren't injecting drupal_container() as a dependency into DrupalKernel instead of hard-coding a ranges of services within?

We could change the config service definition in drupal_container() to use the FileStorage only, and make the CoreBundle override the service with the current CachedFileStorage. Thus, you get the same net effect, but in a much cleaner architecture.

From my perspective, that would make the actual situation of dependencies a lot clearer.

AFAICS, it would also automatically resolve all of my unit testing concerns, because DrupalUnitTestBase can simply instantiate the kernel and pass a specially prepared container to it (which would then persist for the kernel instance and would be re-used as base container when the kernel's container is rebuilt in buildContainer()).

chx’s picture

> I still dislike the hard-coded file storage. We've spent an enormous amount of time to design the config system in a way to make the storage swappable, and thus, it really throws me off to see all of that hard work being kicked out of the window here. :(

There is some hilarity when *I* am accused of throwing pluggability out the window, isn't there :) ? But we need to start breaking the circle somewhere. Two things here: a) config_install_default_config uses FileStorage too albeit not for the active storage, I admit that b) I am sorry to say this but the only reason I have not added a $conf-based based pluggable class here because you were so massively against such a mechanism in a sister issue of this. I talked to catch at BADcamp and he suggested that all those settings.php only variables would go into a single $settings global (including $database!) and $conf becomes a CMI override only. What about adding a variable class name in a followup based on this new $settings? Should I add a @TODO? I agree this is not best but we need to use some mechanism that can specify a reader which doesn't depend on the service container. Bootstrap is ugly and hard.

> What I don't get is why we aren't injecting drupal_container() as a dependency into DrupalKernel instead of hard-coding a ranges of services within?

Please note that neither the class loader nor the php storage object is present in drupal_container(). Also, that's an abomination we want to kill. Edit: yes, I see you mentioned FileStorage pluggability but drupal_container() is not a mechanism we want to rely on. This class will be the very first to be loaded once settings.php is read and drupal_container() will cease to exist.

I have already pointed out that unit tests can mock everything: the services passed to the constructor, the module list and the single service used from the freshly-read container to grab the module list.

I hope I addressed the concerns -- although I am fairly sure we will need a followup with more testing, I would be happy to work with you on them and I hope we can go forward with this issue. katbailey already approved of this (#44 is green) and this issue blocks two chains of cleanup events: one that leads to changing the inital bootstrap to DRUPAL_BOOTSTRAP_CONFIGURATION which is much desired for say JSON replies. The other that removes the current madness of firing hooks while rebuilding the hook system...

Edit: I sincerely hope that my desire to work together again comes through. I feel really good so far about this issue, for the first time in a long, long time.

catch’s picture

Yeah the only way I can see to break the dependency on the hard-coded file storage is a variable classname in settings.php.

The short version of the problem is this:

1. Everything depends on the DIC (how else could it be)
2. The DIC depends on the list of currently enabled modules (an alien concept to any other project using a DIC I'm sure)
3. The list of currently enabled modules depends on the DIC.
4. Whoops.
5. There is a get-out, which is that the list of currently enabled modules resides on disk in a yaml file, which can be got at very simply.
6. When there is no DIC available, then the absolute minimum of systems needs to be activated to rebuild it. So to step out of the circular dependency we do a direct file access for the YAML, then go from there, being careful not to use anything which could be replaced via the DIC.

The only other thing I could possibly think of would be to initialize a bootstrap/maintainance DIC that respects definitions settings.php etc. but nothing else (i.e. the one this patch is trying to remove more or less).

This would have enough services to get as far as normal config()->get() and from there build the real DIC again. That works as long as no module's bundle is ever going to override a service like configuration storage in the DIC, 'cos then you're back to circular dependency hell again. If that's even viable though I think at this point it'd be worth opening a follow-up and accepting the limitation of some of the details here. If we're not able to get this one fixed, then we're going to have to roll back #1599108: Allow modules to register services and subscriber services (events).

Crell’s picture

The config system has a database dependency, too. That's too high in the stack for a rebuild, IMO.

catch’s picture

Talked to Crell and Kat Bailey in irc. I'd floated the idea of the 'crutch' container just for rebuilds, to avoid the hard-coded CMI call, but no-one else is particularly keen on that since it could be quite a big crutch. That makes sense to me as well - we're very clearly defining what we need to get to the container with the current patch by hard-coding everything.

So it comes back to saving pluggable CMI storage, #1784312: Stop doing so much pre-kernel bootstrapping has an issue summary with various approaches to this, so it's mainly a case of trying to pick one I think - it'll be one, special-case, outside of the DIC which is fine since that's exactly why we need it.

I'm happy for that to be a major (or critical if we want) follow-up to this issue so we're only tackling one nightmare at a time.

sun’s picture

Huge thanks for the explanations/clarifications — those were badly needed.

I think that idea of a minimal DIC rebuild container is more or less exactly the same I mentioned in #45. I may be mistaken, but isn't the PhpStorage pluggable and option-configurable, too? That would make it at least two services/parameters to inject already.

I'm well aware that @Crell and @katbailey aren't fans of the idea ;) but that "we need a DIC to build a DIC" situation is essentially the race condition/circular dependency that I kept on repeating in other issues.

Middle-term, a potential solution might be to have a drupal_rebuild_container() that includes a file_exists(./sites/default/container.php), which in turn could allow to set/override services. But yeah, the customization part we can discuss later, doesn't change the fundamental DIC-DIC dependency situation.

It would be great if we could at least try to confirm that the proposed changes here will not totally break unit-testability of kernel-dependent code by applying and executing the new test in #1774388: Unit Tests Remastered™ — as I already tried to communicate before, it is crystal clear to me in the meantime that we could have avoided a lot of pitfalls by doing exactly what I did over there: unit-testing functionality within a kernel-dependent environment. That inherently reveals each and every possibility of a dependency problem. Thus, if those tests work, and even better, if they don't need tons of hacks to work around quirks (like that patch has to do now), then we consequently know that we are in a pretty solid state.

Aside from that, the latest code contains some improved comments, but IMHO we're still lacking human-readable explanations à la #47. There are also still quite some coding style/standards things to clean up.

Tor Arne Thune’s picture

A little bird told me this needed a nitpick review (be careful what you ask for).
If you want I can reroll with these changes, not counting the missing param and return directives.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -60,26 +79,20 @@ class DrupalKernel extends Kernel implements DrupalKernelInterface {
+   * @param \Drupal\Component\PhpStorage\PhpStorageInterface $storage
+   *   (Optional) An object handling the load and save of the compiled

Optional should be all lowercase.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -93,28 +106,90 @@ public function init() {
+    // init container

A bit too terse.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -93,28 +106,90 @@ public function init() {
+   * @param $module
+   *   The name of the module.
+   * @return \stdClass|bool
+   *   Returns a stdClass object if the module data is found containing at
+   *   least an uri property containing the module path for example
+   *   core/modules/user/user.module.

There should be a blank line between the param and return directives. Also the second "containing" could probably be replaced by "with", if that doesn't change the meaning. It reads better, in my opinion. Also a comma before "for example".

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -93,28 +106,90 @@ public function init() {
+      $all_profiles = $profiles_scanner ->scan('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.profile$/', 'profiles');

Is the space between "$profiles_scanner" and "->scan" necessary?

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -93,28 +106,90 @@ public function init() {
+      // If a module is within a profile directory but specifies another
+      // profile for testing, it needs to be found it in the parent profile.

Typo. -it.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -93,28 +106,90 @@ public function init() {
+      if (($parent_profile_config = $storage->read('simpletest.settings')) && isset($parent_profile_config['parent_profile']) && $parent_profile_config['parent_profile'] != $profiles[0] ) {

No space needed at the end before the last closing parenthesis.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -93,28 +106,90 @@ public function init() {
+        //  actual profile always has precedence.

Unnecessary leading space (before "actual").

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -93,28 +106,90 @@ public function init() {
+      $this->moduleData = $all_profiles + $modules_scanner ->scan('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.module$/', 'modules');

Is the space between $modules_scanner and ->scan necessary?

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -174,7 +266,11 @@ protected function initializeContainer() {
+    // init bundles

Terse.

+++ b/core/lib/Drupal/Core/DrupalKernelInterface.phpundefined
@@ -25,6 +25,8 @@
+   * @param array
+   *   List of module paths, keyed by module name.

Missing variable name.

+++ b/core/lib/Drupal/Core/SystemListing.phpundefined
@@ -0,0 +1,181 @@
+ * $all_profiles = $profiles_scanner ->scan('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.profile$/', 'profiles');

Space between $profiles_scanner and ->scan.

+++ b/core/lib/Drupal/Core/SystemListing.phpundefined
@@ -0,0 +1,181 @@
+   * @param string $directory
+   * @return array

Blank line between param and return. They also need descriptions.

+++ b/core/lib/Drupal/Core/SystemListing.phpundefined
@@ -0,0 +1,181 @@
+   * @param array $files_to_add
+   *   The files found in a single directory.
+   * @return array

Same.

+++ b/core/lib/Drupal/Core/SystemListing.phpundefined
@@ -0,0 +1,181 @@
+   * @param $nomask
+   * @return array

Same.

+++ b/core/lib/Drupal/Core/SystemListingInfo.phpundefined
@@ -0,0 +1,71 @@
+    // The 'core/profiles' directory contains pristine collections of modules and

Surpasses 80 characters.

+++ b/core/modules/system/lib/Drupal/system/Tests/DrupalKernel/DrupalKernelTest.phpundefined
@@ -34,21 +35,25 @@ function testCompileDIC() {
+    // @TOOD: write a memory based storage backend for testing.

Spelling of todo. Could be important if someone is grepping for todos in core.

Other than these nitpicks, this looks great :)

Berdir’s picture

This is looking good to me too. A few notes on the documentation below, might duplicate stuff that was already mentioned by @Tor (usernames with spaces are weird to reference :p)

+++ b/core/includes/common.incundefined
@@ -5159,81 +5160,11 @@ function drupal_cron_cleanup() {
+  // As SystemListing is required to build a dependency injection container
+  // from scratch and SystemListingInfo only extends SystemLising, this
+  // functionality needs to be hardwired.
+  $listing = new SystemListingInfo();
+  return $listing->scan($mask, $directory, $key, $min_depth);

Typo: SystemLising.

Also wondering if class instead of functionality would make more sense here, as that is what's hardcoded? Functionality/code/method calls usually are and were so before.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -60,26 +79,20 @@ class DrupalKernel extends Kernel implements DrupalKernelInterface {
-  public function __construct($environment, $debug, array $module_list = NULL, CacheBackendInterface $compilation_index_cache = NULL) {
+   * @param \Symfony\Component\ClassLoader\UniversalClassLoader $class_loader
+   *   A classloader object. The classloader is only used if $storage is not
+   *   given or the load from storage fails and a container rebuild is

The first sentence seems obvious to me, we usually write stuff like that when we have nothing better to write but there's great documentation behind it :). Maybe rewrite the start of the second sentence a bit and remove the first? "This classloader is only used .... " would IMHO be enough.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -93,28 +106,90 @@ public function init() {
+      // When installing new modules, the module list passed to
+      // updateModules() does not yet have their namespace registered.
+      $namespace = 'Drupal\\' . $module;

it's moduleS, so should be "do not yet have", right?

+++ b/core/lib/Drupal/Core/SystemListing.phpundefined
@@ -0,0 +1,181 @@
+ * Returns information about system object files (modules, themes, etc.).
+ *
+ * This function is used to find all or some system object files (module files,
+ * theme files, etc.) that exist on the site. It searches in several locations,
+ * depending on what type of object you are looking for. For instance, if you
+ * are looking for modules and call:

This and everything below is he same documentation as in drupal_system_listing(), right? Wondering if we should remove most of the documentation in one of these places (I guess that in the function) and instead add a @see for this. I'm sure either of these will get out of date and contradict the other one otherwise :)

chx’s picture

Given that the unit test remastered issue over there is fixing its own test base and other parts of Drupal I would say that dragging that issue into this is very seriously off topic. We need to get these two issues merged, and I will help that but here it's a distraction. I understand your concern, but given your issue needs to patch the current DrupalKernel and might need to patch this one then sending this in is not worse for that issue and it helps a lot other issues.

The idea of a minimal DIC is simply horrific. Why are we insisting on that? What's wrong with a simple pluggable classname? What are your use cases that can't be covered by that? Don't forget that whatever is the configuration file reader it is such that it can't rely on the config system (obviously) and so it's quite limited in what Drupal code it might want to use anyways. I am thinking on maybe someone wanting a different format, maybe someone wants glip (I would love to see that happen), maybe some super fancy versioned database (and no I don't plan to store CMI files in MongoDB, that's for damned sure). Hardly anything needing existing services. There must be some low level things that are not in the service container.

chx’s picture

StatusFileSize
new18.23 KB
new37.6 KB
PASSED: [[SimpleTest]]: [MySQL] 47,999 pass(es).
[ View ]

Thanks for the doxygen reviews. I think this covers them all. And sun, I feel for you, you worked hard on the Drupal Unit Test and I appreciate that and I will help making this version of the kernel work well with it but not in this issue, OK?

katbailey’s picture

Status:Needs review» Reviewed & tested by the community

I loved it before and I love it even more now :-)

sun’s picture

Status:Reviewed & tested by the community» Needs work

Sorry. It took me only a few minutes to run this against the much more sophisticated kernel tests to find the following problems:

+++ b/core/includes/module.inc
@@ -507,7 +507,7 @@ function module_enable($module_list, $enable_dependencies = TRUE) {
       if ($kernel = drupal_container()->get('kernel', ContainerInterface::NULL_ON_INVALID_REFERENCE)) {
-        $kernel->updateModules(module_list());
+        $kernel->updateModules(module_list(), array($module => drupal_get_path('module', $module)));
       }

For some reason, this doesn't properly register the bundle of the newly enabled module.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -33,14 +35,17 @@ class DrupalKernel extends Kernel implements DrupalKernelInterface {
-  protected $moduleList;
+  protected $moduleList = array();

@@ -93,28 +106,90 @@ public function init() {
+    if (!$this->moduleList) {

The default must be NULL, so as to be able to differentiate between an undefined and empty module list, whereas the empty should not trigger a automatic config lookup, nor a filesystem scan. Thus, also !isset().

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -93,28 +106,90 @@ public function init() {
+      $storage = new FileStorage(config_get_config_directory());
+      $module_list = $storage->read('system.module');
+      $module_list = $module_list['enabled'];
+      $this->moduleList = $module_list;

Likewise:

- config will return FALSE if the system.module config object does not exist

- it should further be failsafe for the case in which it might return NULL, or an empty array (in which case the access to 'enabled' will trigger a warning)

Furthermore:

The major problem with this is that it breaks the fixed/locked module list.

This means it is not compatible with the installer (with and without kernel), nor update.php, site-offline/exception/maintenance pages, and also not with unit tests.

All of those cases would have to manipulate and write to the system.module config file, in order to be able to boot the kernel. Doing so overwrites a potentially existing config object. And for the installer, it means that modules cannot be installed anymore, because they already have an entry in system.module.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -93,28 +106,90 @@ public function init() {
+    foreach ($this->moduleList as $module => $weight) {
...
+  public function updateModules(array $module_list, array $module_paths = array()) {
     $this->moduleList = $module_list;

Apparently, the code makes entirely different assumptions on what $this->moduleList is in different locations — up to a level that I seriously wonder how this is able to pass tests right now.

First it is an array keyed by module names whose values are module weights.

Later it is the value of module_list(), which is not the same.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -126,33 +201,50 @@ public function updateModules($module_list) {
+    if (isset($this->container)) {
+      $module_list = array_keys($this->moduleList ?: $this->container->get('config.factory')->get('system.module')->load()->get('enabled'));
+      if ($module_list !== $this->container->getParameter('container.modules')) {
+        unset($this->container);
       }
     }

I do not understand what this part is doing, and why it is accessing config through the container, and why it is unsetting the internal copy of the container this late in the process?

sun’s picture

StatusFileSize
new3.46 KB

FWIW, these changes should get you started.

sun’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -126,33 +201,50 @@ public function updateModules($module_list) {
-    if ($this->compilationIndexCache) {
...
-        // First, try to load.
-        if (!class_exists($class, FALSE)) {
...
-        // If the load succeeded or the class already existed, use it.
-        if (class_exists($class, FALSE)) {
...
+    $class = $this->getClassName();
+    $cache_file = $class . '.php';
...
+    // First, try to load.
+    if ($this->storage && !class_exists($class, FALSE)) {
...
+    // If the load succeeded or the class already existed, use it.
+    if (class_exists($class, FALSE)) {

Previously, all of this was wrapped in the $this->storage condition, which we want to retain, since it doesn't make sense to check for the compiled class name if we don't have a storage.

chx’s picture

Status:Needs work» Needs review

> For some reason, this doesn't properly register the bundle of the newly enabled module.

I do not know what "properly" means and to me it seems standard profile gets installed just fine and the compiled container contains Views and Field SQL bundles. Also, previously I have seen "service missing" fatals when the views bundle was not picked up.,

> The default must be NULL, so as to be able to differentiate between an undefined and empty module list
> config will return FALSE if the system.module config object does not exist

There is no way the system.module object not existing or the module list to be empty. That's invalid, plain and simple, we have required modules and Drupal is totally broken without system anyways.

> The major problem with this is that it breaks the fixed/locked module list.

When you lock the module list just pass the same list to updateModules. Also this is scheduled to be cleaned up: if this finally can get in then Kat can do the extensionhandler which will make the fixed list capability much cleaner. If this can get in...

> This means it is not compatible with the installer.

Well, it installs for me and the testbot?

In #57 you are patching the newly introduced DrupalUnitTestBase heavily in a DrupalKernel issue. Please, please, please can we keep that in a follow up? Can we get this one so we can do the six or seven already filed followups done? (Some major, some critical)

As for what's in moduleList, only the keys are used so the values can be weights or the module weights, that's not relevant.

sun’s picture

Status:Needs review» Needs work
StatusFileSize
new5.16 KB

Ok, if you merge the attached changes into this patch, then I can live with it for now.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new1.9 KB
new37.73 KB
PASSED: [[SimpleTest]]: [MySQL] 47,999 pass(es).
[ View ]

Addressed #58.

Edit: X-post. Reviewing #60. The patch name shows I didn't intend to shun #60. It's just X-post, OK?

sun’s picture

Status:Needs review» Needs work

There is no way the system.module object not existing or the module list to be empty. That's invalid, plain and simple, we have required modules and Drupal is totally broken without system anyways.

Sorry, but I need to correct you here. This is no longer the case. Poor old Drupal had this requirement, D8 does not.

sun’s picture

Status:Needs work» Needs review

whoopsie, x-post.

sun’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.php
+  protected $newModuleList;

@@ -186,7 +196,7 @@ protected function moduleData($module) {
   public function updateModules(array $module_list, array $module_paths = array(), ContainerBuilder $base_container = NULL) {
-    $this->moduleList = $module_list;
+    $this->newModuleList = $module_list;

@@ -223,25 +233,27 @@ protected function getClassName() {
+    if (isset($this->moduleList) && isset($this->newModuleList) && array_keys($this->moduleList) !== array_keys($this->newModuleList)) {
+      unset($this->container);
+      $this->moduleList = $this->newModuleList;
+      unset($this->newModuleList);
+    }

These changes are necessary, too. Why did you omit them?

chx’s picture

StatusFileSize
new932 bytes
new37.89 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

I reviewed #60 but there is very little there I can roll in. We addressed #58 at the same time so that's a done. Removing the config service call kills our ability to enable a module on a webhead and get the other webheads rebuild. I added already a comment about that in #61 ( I know you couldn't see it before). I might be open to add the new module list but is that relevant still if we must keep the config read anyways? The DrupalUnitTestBase chunks are not against HEAD code AFAIK. So I added the module list emptiness checker.

Status:Needs review» Needs work

The last submitted patch, 1831350_64.patch, failed testing.

chx’s picture

Status:Needs work» Needs review

To clarify, again, because it was only a comment edit before: #61 was a cross post with #60 and was an answer to #58 only, I didn't see #60 before I posted it. #64 contains my answer to #60.

chx’s picture

StatusFileSize
new434 bytes
new37.84 KB
PASSED: [[SimpleTest]]: [MySQL] 47,981 pass(es).
[ View ]

D'oh!

sun’s picture

Well, we simply need to do both then.

First, check for a single webhead + $moduleList !== $newModuleList. If that is the case, then the new module list has been written by module_enable() already, so we don't need to re-read it, because $newModuleList is supplied by module_enable(), right after it wrote the new module list.

Second, additionally care for multiple webheads as failsafe, in case the first condition was negative — which would mean that the kernel is rebuilt without being called from module_enable() — is that even possible in the first place? How would the system/kernel boot identify that? It just reads in the compiled and dumped container, and I'm fairly sure we don't want to double-check the module list on every single request... So how is that case possible in the first place?

chx’s picture

StatusFileSize
new38.51 KB
FAILED: [[SimpleTest]]: [MySQL] 47,992 pass(es), 4 fail(s), and 5 exception(s).
[ View ]
new2.08 KB

OK, I have added the new module list. The second question is an easy one to answer: during installer, module_enable goes against a kernel which does not have the storage object passed in. Only index.php creates compiled containers. And the AJAX requests from install.php do hit index.php so every request as the install batch progresses will have a different module list. I added comments regarding this.

sun’s picture

Well, installer is a different topic than multiple webheads.

Is that second check for the module list in config really required for this issue right now, or just a premature optimization?

My patch in #1798732-54: Convert install_task, install_time and install_current_batch to use the state system (which is quasi-RTBC, just blocked on PIFR changes) adjusts the installer to always boot a kernel for all modules that get installed (including System module), thus, no difference to a regular module_enable(); i.e., the first condition for $newModuleList will succeed already, and there's no need to special-case the installer any further.

effulgentsia’s picture

I haven't read any of the recent patches here, but chx and I did discuss this issue at BADCamp, and this jumped out at me:

I'm fairly sure we don't want to double-check the module list on every single request

Why not? Can't we assume that the vast majority of the time, such a check is extremely fast?

sun’s picture

No, the Yaml parser is definitely not fast enough. This code runs in the super-critical path on every single request.

chx’s picture

Please, read this again: the AJAX requests from install.php do hit index.php so every request as the install batch progresses will have a different module list. If you remove the config countercheck then a normal install will be stuck with a container containing system and user. And it's not "premature optimization" to support multiple webheads, is it?

> thus, no difference to a regular module_enable();

There IS a difference, but I apparently fail to explain in English, what about PHP?

index.php:31:$kernel = new DrupalKernel('prod', FALSE, drupal_classloader(), drupal_php_storage('service_container'));
core/includes/install.core.inc:1507:  $kernel = new DrupalKernel('prod', FALSE, drupal_classloader());

module_enable in the installer runs against a kernel which does not compile to disk.

chx’s picture

> No, the Yaml parser is definitely not fast enough. This code runs in the super-critical path on every single request.

Absolutely not! We use the cached config service to read the module list once the container is up, we do not parse the YAML file every request. Parsing a YAML file happens on rebuild only and only if we do not have a module list to work from, aka. install.

chx’s picture

Note that I didn't consider parsing a YAML file a stellar idea either. https://twitter.com/forestmars/status/264486436701605888 and this patch is not doing it.

sun’s picture

the AJAX requests from install.php do hit index.php so every request as the install batch progresses will have a different module list

Sorry, but that is wrong. The batch requests in install.php are not hitting index.php. They are hitting core/install.php.

batch_process(install_redirect_url($install_state), install_full_redirect_url($install_state));

@see http://drupalcode.org/project/drupal.git/blob/HEAD:/core/includes/instal...

Status:Needs review» Needs work

The last submitted patch, 1831350_70.patch, failed testing.

chx’s picture

Status:Needs work» Needs review

OK, not installer, sure, but the multiple webheads is still relevant. The patch to review is #68, the new modules idea is broken and I disagree with it in the first place.

chx’s picture

StatusFileSize
new37.84 KB
PASSED: [[SimpleTest]]: [MySQL] 48,018 pass(es).
[ View ]

Repost of #68 to make it easier to see. If you want to mess with the module list then please mock the config service to return a messed module list.

sun’s picture

Status:Needs review» Needs work

The test failures seem pretty clear, you seem to be passing NULL instead of a module list:

array_keys() expects parameter 1 to be array, null given

array_keys(NULL)
Drupal\Core\DrupalKernel->initializeContainer()
Drupal\Core\DrupalKernel->boot()
Drupal\system\Tests\DrupalKernel\DrupalKernelTest->testCompileDIC()
Drupal\simpletest\TestBase->run()

Please double-check the code in the interdiff I provided in #60 — the logic was different, which likely explains why your patch is failing.

chx’s picture

Status:Needs work» Needs review

I am actively looking into this and trying to make the tests pass with this idea you are married to but it's very hard, I got the DrupalKernel test pass but not the Bundle test. But I still think it's simply unclean. We need to read the module list from config. It's not hard to write a class that mocks the list of enabled modules. Why are we so married to special casing something in the kernel instead of special casing something in a test class extending the config? Can we just use the patch that passes and works?

sun’s picture

StatusFileSize
new2.74 KB
new38.35 KB
FAILED: [[SimpleTest]]: [MySQL] 48,009 pass(es), 3 fail(s), and 4 exception(s).
[ View ]

Here.

Status:Needs review» Needs work

The last submitted patch, kernel.dic_.83.patch, failed testing.

chx’s picture

Status:Needs work» Needs review
StatusFileSize
new2.31 KB
new38.54 KB
PASSED: [[SimpleTest]]: [MySQL] 47,995 pass(es).
[ View ]

No that won't work, you need to populate moduleList.

sun’s picture

Status:Needs review» Reviewed & tested by the community

Thanks, works for me.

I still massively dislike how this patch literally destroys months of configuration system work. This is a total slap in the face for me, and I aggressively hate that this change is being done here. Even more so, because it is done for no good reason, given that there is an alternative solution.

I still think that injecting a minimal DIC into DrupalKernel would be architecturally cleaner and technically more correct.

Doing so would eliminate a ton of workarounds in this code and overall simplify the system a lot. I understand the concerns against it, but at the same time, I think those are too utopian/wishful thinking that is based on guidance that applies to applications different than Drupal and dismiss our actual architectural requirements.

chx’s picture

I filed #1837092: Make sure DrupalKernel can use alternate config storage and also -- please, it is not like I would want anyone to feel a patch is a slap in the face. I am not happy you take this or any other patch so seriously :( You would be hard pressed to find a more staunch supporter of making everything pluggable in Drupal than me. I still think this is our best compromise. We need to compromise here and having two containers is not proving to be a good one.

webchick’s picture

Assigned:Unassigned» catch

This looks like catch material.

pounard’s picture

Assigned:catch» Unassigned

Kernel needs config to have a module list yet config needs a kernel to be pluggable. $conf array is exactly here to solve that, and $conf array classname for early config object is, I think, the right way to go. Config cannot configure itself ayway, this is the real chicken and egg problem, so hardcoded (sorry) settings.php minimal config to configure config seems the right thing to do. chx++

aspilicious’s picture

Can someone assign this back to catch? I can't.

chx’s picture

Assigned:Unassigned» catch

I can.

sun’s picture

Assigned:catch» Unassigned
Status:Reviewed & tested by the community» Needs review
StatusFileSize
new13.94 KB
new42.59 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Just to prove that there is an alternative solution that is architecturally way cleaner, unit-testable, starts to eliminate a range of tasks just by itself, does not destroy months of config system work, and perhaps opens the eyes for some:

If the kernel builds a DIC, but the kernel itself needs a DIC, then we need to inject a DIC into a DIC-builder.

This essentially kills the current drupal_container(), which, long-term, would only remain as procedural helper to reach the kernel's DIC. It is replaced by drupal_bootstrap_container(), which is only invoked once, and gets injected into DrupalKernel, so, again, we have a DIC to (re)build a DIC.

Inherently cleanly compatible with unit tests, because unit tests can instantiate a different bootstrap container with mocked or memory service implementations.

One might argue that it's odd to inject an entire DIC, and the kernel could be registered as a service on the bootstrap DIC instead and instantiated from there, but that doesn't matter at all for me, since in the end, it's about building a DIC, so who cares.

Status:Needs review» Needs work

The last submitted patch, kernel.dic_.92.patch, failed testing.

chx’s picture

Assigned:Unassigned» catch
Status:Needs work» Reviewed & tested by the community

(the patch fails because install.core.inc is not converted) Sorry, but I need to stand firm: no way we are doing this in this issue. Just having a container is not a miracle that solves pluggability! The current drupal_container has a static which makes it possible to override services living in it in settings.php but this new one doesn't. You still hardwire the config storage just in a very elaborate way.

We can explore this approach in #1837092: Make sure DrupalKernel can use alternate config storage . I am not against it. I am against trying to solve it in this issue especially in a way that does not provide pluggability in fact.

chx’s picture

Note: #85 reached a rare agrement, that's what I reset to.

sun’s picture

I'm willing to agree and move forward here, if we will seriously consider #92 as a potential way to refactor this code. Definitely not if that turns into a uphill battle.

Also note that patches before #85 caused a serious testbot slowdown of more than 10 minutes (relative to other test runs on the same machine). #85 appears to be neutral, even though I don't really see why it should be different in terms of performance.

Lastly, manual debugging revealed that the PHP storage being used by the kernel during tests is not using the test's environment (i.e., each test run leaves a range of stray compiled PHP files in the parent/test-runner environment behind). That appears to be a separate, pre-existing bug though.

And finally, this patch refactors some essential code and introduces quite a range of expectations, which are not covered by tests in any way. (Again, the existing DrupalKernel tests are rather a joke.) This is only acceptable, if we commit the follow-up patch from #1774388: Unit Tests Remastered™ right afterwards, since it essentially is the test coverage for this code here (which means that it would only be implicit test coverage instead of explicit, but we might be able to live with that).

chx’s picture

First I revoke what I said in #54 about a minimal container -- if implemented and used this way, it might work.

I fully agree with #96: we are already working (together! yay!) on #1837092: Make sure DrupalKernel can use alternate config storage to make that happen (and I think I just came up with a simple, elegant and powerful solution there I am actually quite proud of -- let's hope I didn't overlook something fundamental) and I will definitely help the unit test issue.

katbailey’s picture

I've commented on the issue mentioned in #97 to the effect that we may need to do some sort of bootstrap container thing - but it will be a far cry from what's in place now. What's in place now definitely needs to go away as it is a big confused mess serving as 1) a replacement for the kernel-built container for the installer and run-tests.sh and 2) a bootstrap container that contains way more than what ought to be needed pre-kernel.

The main value in this issue and follow-up patches such as #1784312-26: Stop doing so much pre-kernel bootstrapping and indeed #1837092: Make sure DrupalKernel can use alternate config storage is we are figuring out exactly what we need and when we need it. Let's get this in and work together on these follow-ups.

catch’s picture

Assigned:catch» Unassigned

fwiw I'm happy with the approach here to get this un-stuck, given we've got active, viable follow-ups to address config pluggability and etc. However since I originally came up with the idea of reading the config files directly for this rebuild when discussing this issue in person with chx, I wanted to give the patch a little while to get more reviews before actually committing it. It seems there's general agreement so that's fine.

I'd like to give this one more look through before I commit it, but also unassigning me since i'm fine with the overall approach, I'm unlikely to get to this today (out for much of this afternoon then again in the evening), so if Dries or webchick beats me to it that's fine.

catch’s picture

Status:Reviewed & tested by the community» Fixed

Alright took another look through, apart from known issues in follow-ups I can't see anything else to complain about. Committed/pushed to 8.x!

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

jenlampton’s picture

Just adding a note here that this change made it impossible for me to run Drupal 8 in my local environment.
See #2096741: Installation failure: Missing configuration files

There's a temporary workaround patch in that issue in case anyone else is unable to get any work done either :)

jenlampton’s picture

Issue summary:View changes

upped summary

donquixote’s picture

Issue summary:View changes

Imo, "getClassName()" should rather be "getContainerClassName()" or "getServiceContainerClassName()".