Refactor module handling responsibilities out of DrupalKernel

kernel-module-handler-refactor.patch queued for re-testing.

Handing off handling module data to the module handler sounds a very good idea, indeed. The devil is in the details:

ModuleHandlerFactory can be further simplied by removing moduleData() as visible http://privatepaste.com/e7148fd096 here. However, this is only possible because the module data is not reused -- and that seems bad. As far as I can see, HEAD will scan once per Kernel and updateModules will not cause a re-scan as moduleData is updated on-the-fly. Don't forget that module_enable rebuilds the container per module... and I think we added a re-scan for each. That looks bad.

I see no reason to use the event dispatcher here. Maybe if it would be injected to everyone involved but of course that's not quite possible so at the end of the day we have

Drupal::getContainer()->get('event_dispatcher')->dispatch('drupal.update_modules'); 
// versus
Drupal::getContainer()->get('kernel')->rebuildContainer();

I can't see the former more desirable than the latter. We do not have a per-module enable/install hook so it's not like we are replacing / amending a hook here with events.

Minor: for further simplification, initializeCachedContainer could remove $container = NULL; and return $container. Fruther, if persistServices were to return $container one could writereturn $this->persistServices(new $fully_qualified_class_name, $persist);

+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.phpundefined
@@ -264,7 +264,7 @@ protected function enableModules(array $modules) {
+    Drupal::service('event_dispatcher')->dispatch('drupal.update_modules');

@@ -295,7 +295,7 @@ protected function disableModules(array $modules) {
+    Drupal::service('event_dispatcher')->dispatch('drupal.update_modules');

Missing leading \, I think that's most of the fails.

Related: usage of container in tests is not clear #2014015: Replace drupal_container() with Drupal::service() in the simpletest module

Here's a hilariously convoluted solution to the DUTB test problem (they were ending up with an empty module list after the 'drupal.update_modules' event was dispatched so namespaces were not being registered) - purely for experimental purposes, to see where it leaves us. I'm hoping there's an easier way to achieve the same thing...

+++ b/core/modules/simpletest/lib/Drupal/simpletest/ConfigMemoryStorage.phpundefined
@@ -0,0 +1,95 @@
\ No newline at end of file

new line

+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.phpundefined
@@ -95,7 +100,11 @@ protected function setUp() {
     \Drupal::state()->set('system.theme.data', $this->themeData);

@@ -263,13 +276,21 @@ protected function enableModules(array $modules) {
-    $this->kernel->updateModules($module_filenames, $module_filenames);
+    \Drupal::service('event_dispatcher')->dispatch('drupal.update_modules');

@@ -294,8 +315,14 @@ protected function disableModules(array $modules) {
-    $this->kernel->updateModules($module_filenames, $module_filenames);
+    Drupal::service('event_dispatcher')->dispatch('drupal.update_modules');

+++ b/core/update.phpundefined
@@ -187,7 +187,8 @@ function update_helpful_links() {
-  Drupal::service('kernel')->updateModules(Drupal::moduleHandler()->getModuleList());
...
+  Drupal::service('event_dispatcher')->dispatch('drupal.update_modules');

@@ -350,7 +351,7 @@ function update_access_allowed() {
-    drupal_container()->get('kernel')->updateModules($module_filenames, $module_filenames);
+    Drupal::service('event_dispatcher')->dispatch('drupal.update_modules');

so \Drupal or Drupal?
also maybe better use $this->container?

Let's try this...

Ugh, DUTB. It looks to me like a bunch of these tests should have been removed with this commit:
http://drupalcode.org/project/drupal.git/commitdiff/36fcf91cb57dc799790f...
module_enable() isn't properly supported for DUTB and these changes make the problem worse. Let's just insist that DUTB tests can only use $this->enableModules() and $this->installSchema().

1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -282,53 +276,15 @@ public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQ
   +   * Rebuilds the container during the request. Used when the list of modules
   +   * changes.
   

   Needs to be one line, or just make the second sentence its own "paragraph"

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -282,53 +276,15 @@ public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQ
   +    // If we haven't yet booted, we don't need to do anything. If we have already
   +    // booted, then reboot in order to refresh the bundle list and container.
   

   Over 80 chars

3. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -447,6 +386,11 @@ protected function initializeContainer() {
   +    $this->container->get('event_dispatcher')->addListener(
   +      'drupal.update_modules', array($this, 'rebuildContainer')
   +    );
   

   Nice!

4. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -469,12 +413,12 @@ protected function getServicesToPersist() {
       * Moves persistent service instances into a new container.
       */
   -  protected function persistServices(array $persist) {
   +  protected function persistServices($container, array $persist) {
   
   @@ -619,35 +563,31 @@ protected function storage() {
   +   * Registers a list of namespaces.
   ...
   +  protected function registerNamespaces(array $namespaces = array()) {
   

   We should have @param here...

5. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -619,35 +563,31 @@ protected function storage() {
   +   * @param $class
   +   * @param $persist
   

   Missing the one line doc, and the actual typehint/docs for @param, and @return.

6. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -65,6 +65,8 @@ class ModuleHandler implements ModuleHandlerInterface {
   +  protected $moduleData;
   
   @@ -141,6 +143,50 @@ public function setModuleList(array $module_list = array()) {
   +  public function getModuleNames() {
   ...
   +  public function getModuleFileNames() {
   ...
   +  public function getModuleFileName($module) {
   ...
   +  public function getModuleDirectory($module) {
   ...
   +   * Returns the file name for each enabled module.
   +   */
   +  public function findModuleFileNames($module_list) {
   ...
   +  public function getModuleNamespaces() {
   

   Docs needed (one line description, @param, @return)

7. +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerFactory.php
   @@ -0,0 +1,79 @@
   +class ModuleHandlerFactory {
   

   The whole class needs docs

8. +++ b/core/modules/simpletest/lib/Drupal/simpletest/ConfigMemoryStorage.php
   @@ -0,0 +1,86 @@
   +class ConfigMemoryStorage implements StorageInterface {
   +
   +  protected $moduleList;
   

   Needs docs

9. +++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
   @@ -62,6 +63,8 @@
   +  protected $bootstrapConfig;
   
   @@ -124,6 +132,10 @@ protected function setUp() {
   +  public function getBootstrapConfig() {
   
   +++ b/core/modules/system/lib/Drupal/system/Tests/DrupalKernel/DrupalKernelTest.php
   @@ -18,6 +18,8 @@
   +  protected $bootstrapConfig;
   
   @@ -114,4 +123,21 @@ function testCompileDIC() {
   +  public function getBootstrapConfig() {
   +    return $this->bootstrapConfig;
   +  }
   

   Docs. Are these worth moving to the base class?

10. +++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
    @@ -263,13 +275,21 @@ protected function enableModules(array $modules) {
    +    $module_config = array('enabled' => array());
    +    $weight = 0;
    +    foreach ($module_filenames as $module => $filename) {
    +      $module_config['enabled'][$module] = $weight++;
    +    }
    +    $this->bootstrapConfig->setModuleList($module_config);
    
    @@ -294,8 +314,14 @@ protected function disableModules(array $modules) {
    +    $module_config = array('enabled' => array());
    +    $weight = 0;
    +    foreach ($module_filenames as $module => $filename) {
    +      $module_config['enabled'][$module] = $weight++;
    +    }
    +    $this->bootstrapConfig->setModuleList($module_config);
    
    +++ b/core/modules/system/lib/Drupal/system/Tests/DrupalKernel/DrupalKernelTest.php
    @@ -28,8 +30,10 @@ public static function getInfo() {
    +    $this->bootstrapConfig = new ConfigMemoryStorage();
    +    $this->settingsSet('drupal_bootstrap_config_storage', array($this, 'getBootstrapConfig'));
    
    @@ -44,18 +48,20 @@ function setUp() {
    +    $module_config = array('enabled' => array());
    +    foreach ($module_enabled as $module => $weight) {
    +      $module_config['enabled'][$module] = $weight;
    +    }
    +    $this->bootstrapConfig->setModuleList($module_config);
    
    @@ -91,13 +96,17 @@ function testCompileDIC() {
    +    $module_config = array('enabled' => array());
    +    foreach ($module_enabled as $module => $weight) {
    +      $module_config['enabled'][$module] = $weight;
    +    }
    +    $this->bootstrapConfig->setModuleList($module_config);
    

    Is this worth wrapping/reusing? If not maybe an inline comment

So.

1. +++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
   @@ -263,13 +275,21 @@ protected function enableModules(array $modules) {
   -    $this->kernel->updateModules($module_filenames, $module_filenames);
   +    \Drupal::service('event_dispatcher')->dispatch('drupal.update_modules');
   
   @@ -294,8 +314,14 @@ protected function disableModules(array $modules) {
   -    $this->kernel->updateModules($module_filenames, $module_filenames);
   +    \Drupal::service('event_dispatcher')->dispatch('drupal.update_modules');
   

   why not $this->container->get('event_dispatcher')?

2. +++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/DrupalUnitTestBaseTest.php
   @@ -64,46 +65,13 @@ function testEnableModulesLoad() {
   -  function testEnableModulesInstall() {
   -    $module = 'node';
   -    $table = 'node';
   

   any reason to drop the test?

3. +++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/DrupalUnitTestBaseTest.php
   @@ -198,28 +166,29 @@ function testInstallConfig() {
   +    $module_handler = \Drupal::moduleHandler();
   ...
   -    $this->assertEqual($this->container->get('module_handler')->moduleExists('entity_test'), TRUE);
   +    $this->assertEqual($module_handler->moduleExists('entity_test'), TRUE);
   

   why $module_handler is taken from \Drupal and not $this->container->get()?

Re-roll

This should fix those tests. Basically we can't use the moduleHandler directly in DUTB tests because they are special. Whether this makes a mockery of those tests I don't know as I'm not familiar with the purpose of the tests in question. What I do know is that the monstrosity that is DUTB should not stand in the way of such an important refactoring of something so fundamental to Drupal core.

I still haven't addressed Tim's points 9 and 10 above but will do because that code is brittle as hell. Not sure it makes sense to put methods in the UnitTestBase class because the only non-DUTB test that needs this behavior is DrupalKernelTest, which is special. Special-- :(

This addresses points 9 and 10 in #23 - I added the bootrap config related stuff to UnitTestBase.

I am going to attempt to reroll / un-conflict this, now that #2016629: Refactor bootstrap to better utilize the kernel is in.
Not sure how far I get before I give up! It looks scary.
If someone else wants to take over or help ou, let me know on IRC.

Nope, I give up.
Rerolling would be more difficult than doing it from scratch, I'm afraid.

Going for a new attempt on top of #2279423: Low-level DIC for low-level services: Testing grounds. (I originally wanted to do it the other way round)

There has been no new work on this issue in quite some time. So I'm assuming the person assigned is no longer being actively pursuing it. Sincere apologies if this is wrong.

Getting a lots of conflicts while rerolling. Need an experienced developer to reroll.

Hi, i'm Matt Lambert from Redweb. I am going to reroll.

A lot of things has moved since the last patch has been done for 8.0.x.

This needs lot of work to apply on 8.4.x

Is this still relevant? Considering how much the DrupalKernel and ModuleHandler have changed?

Yes, a lot has changed, but I think maybe we can instead close this as a dup of #2941757: Extension System, Part IV: Properly register all installed extensions/namespaces during container generation.

I agree that the basic premise is the same though: "what we currently has is hardcoded and sucks; needs to be refactored to better handle [all] installed extensions"

I'll look at the patches here to see what I can salvage.