Bundle services aren't available in the request that enables the module

I think module_enable() needs to cause an immediate rebuild of the container and make the kernel use it

I have a feeling this might be in principle impossible. Rebuilding the container mid-request has to mean instantiating a brand new container, right? But the very HttpKernel instance that is handling the request is managing the scope of the old container in doing so. Thinking of making it use the new container makes me think of an Escherian hand drawing itself. If anyone can see a way around this that I'm not seeing, I'd love to hear it.

The only way to make this work, I think, is to Enable the module, regenerate the DIC, and then do a self-redirect to run the install/update hooks now that the DIC is updated. Which of course only works in HTTP mode, not CLI mode, although I think there's some trick to "reboot" a CLI, too.

Is there a specific use case that cannot be otherwise solved, rather than a "nice to have" that we get now by virtue of playing fast and loose with system state (which is something we're trying to get away from)?

i think its not wise to execute code synchronously by a process that is installing said code. it will always be very hard to ensure a defined behavior in this case since its quite common for boot strap processes to make the assumption that all pieces are available. breaking this assumption will always add a lot of complications to very core logic and i dont really see what use case requires the behavior illustrated by the failing test ..

Agree with #4, maybe it's time for Drupal to rethink the way it handles modules. Modules probably should be installed/uninstalled without the need of being enabled, this cause various problems, at site install time where the site needs to be bootstrapped unfinished and when enabling multiple modules at once on a live site, etc.. due to all stuff that cores has to do, such as clearing all the caches, calling all possible hooks, enabling modules, rebuild the runtime environment at runtime...

In actual core, when you enable a module, it's hook_init() and hook_boot() has not been run, and so in bundles services are not registered, which seems totally valid to me. Modules should not need their own services when being installed, considering their aren't even configured yet.

Modules should not need their own services when being installed, considering their aren't even configured yet.

In that case we'd need to remove hook_install(), hook_schema(), hook_enable() and any other hook that's invoked at install time. On the other hand I don't think anyone loves the module install process as it is at the moment so rethinking it a bit sounds good.

I committed #1599108: Allow modules to register services and subscriber services (events) after having given it a lot of thought and discussion, but decided that we need to escalate this to 'critical' as a result of that. I've also moved it into the core issue queue. Please don't demote the priority of this issue. Let's keep working on a good solution. Thanks for all your help! :-)

I'm reasonably sure this is causing all the failures I'm seeing in Views when we try to use bundles.

Rerolled after #1727538: WebTest tests should have access to the correct DIC.

Just to be clear, Tim has rerolled the test that proves that we have a critical bug. We are not any closer to solving it.

I'm curious what bundle services are added by Views and needed at module_enable() time.

Currently, most Views provide either a page callback or a block. hook_menu and hook_block_info are triggered at that time, and need to load Views.

See #1727266: Lazy load plugin managers with dependency injection for the code we're attempting.

So I guess the essence of #12 is that drupal_flush_all_caches() is called in the same request in which the new module is enabled/disabled.

I also guess we only have two options:

1) Rebuild the service container within the same request, before invoking hook_enable() + friends, and recompiling it at the end of the request (or in the next).

2) Try to set a flag/marker or something to signify the enabling/disabling of a module for the next request and deleting the currently compiled container before the request ends, and lastly, also try to handle race conditions on large-scale/high-traffic sites appropriately (which sounds close to impossible to me).

A twin (might need to be enjoined, even) #1759582: Figure out when to delete compiled DIC files

Another use case is ban module to hook into request object #1161486-25: Move IP blocking into its own module

alippai wrote this http://privatepaste.com/9eec5e910b module, one wonders whether we could use the idea here.

Edit: this is essentially the same as 2.) from #13 , implemented in a module. As for the thundering herd problem, can't we use a lock / semaphore / something?

#1763974: Convert entity type info into plugins is also blocked on this.

Clarifying the title as I understand it. It's not just about bundles provided by the module being enabled, it's all bundles.

@katbailey asked for some more info on how we were using this, and how it was breaking.

Views provides many plugin types, currently all controlled by the ViewsPluginManager.

In order to get a plugin instance, we had a procedural function that looked like this:

function views_get_plugin($type, $plugin_id) {
  $manager = new ViewsPluginManager($type);
  return $manager->createInstance($plugin_id);
}

Now, as a deprecated wrapper (we have yet to phase it out), we have:

function views_get_plugin($type, $plugin_id) {
  return drupal_container()->get("plugin.manager.views.$type")->createInstance($plugin_id);
}

However, as we add more test coverage, we're seeing fails like this:

 Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "plugin.manager.views.display" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 693 of /Users/tim/www/d8/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).

Here's the relevant call stack for one such occurence: https://privatepaste.com/ddf8081988
Note lines 13 and 14. l() is being called, which is calling theme_get_registry(), and then everything goes to hell.

So, I tried putting this into ViewTestBase:

  /**
   * Overrides TestBase::changeDatabasePrefix().
   *
   * This is the dirtiest hack. It is the only method called in
   * WebTestBase::setUp() that is after it resets $conf and before it calls
   * module_enable().
   */
  protected function changeDatabasePrefix() {
    variable_set('theme_link', FALSE);
    return parent::changeDatabasePrefix();
  }

And it all worked out.

Other issues, like #1763974: Convert entity type info into plugins, attempt to replace parts of hook_entity_info() with plugins. But entity_get_info() is called very early as well, and breaks similarly: https://privatepaste.com/f81a11a981

See also, #1786990: [Module]Bundle is registered too late in WebTestBase::setUp(), #1801082: drupal_flush_all_caches() assumes a web request - "The service definition "router.builder" does not exist. "

I agree with the title revert. Between this and the two issues I linked in #20, those are all of the problems we've seen.

#1786990: [Module]Bundle is registered too late in WebTestBase::setUp() was committed, which fixes half of VDC's problems with this.
Here is the current failure:

exception 'Symfony\Component\DependencyInjection\Exception\InvalidArgumentException' with message 'The service definition "plugin.manager.views.display" does not exist.' in /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php:690
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php(338): Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition('plugin.manager....')
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views/views.module(1297): Symfony\Component\DependencyInjection\ContainerBuilder->get('plugin.manager....')
#2 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views/lib/Drupal/views/ViewExecutable.php(579): views_get_plugin('display', 'default')
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views/lib/Drupal/views/ViewStorage.php(160): Drupal\views\ViewExecutable->initDisplay()
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views/views.module(660): Drupal\views\ViewStorage->initDisplay()
#5 [internal function]: views_block_info()
#6 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(957): call_user_func_array('views_block_inf...', Array)
#7 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/block/block.module(424): module_invoke('views', 'block_info')
#8 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/block/block.module(963): _block_rehash('bartik')
#9 [internal function]: block_rebuild()
#10 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(981): call_user_func_array('block_rebuild', Array)
#11 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/common.inc(6848): module_invoke_all('rebuild')
#12 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/install.core.inc(1706): drupal_flush_all_caches()
#13 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/batch.inc(427): _install_profile_modules_finished(true, Array, Array, '0 sec')
#14 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/batch.inc(329): _batch_finished()
#15 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/form.inc(4938): _batch_process()
#16 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/install.core.inc(533): batch_process('core/install.ph...', 'http://drupalte...')
#17 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/install.core.inc(429): install_run_task(Array, Array)
#18 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/install.core.inc(85): install_run_tasks(Array)
#19 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php(686): install_drupal(Array)
#20 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(629): Drupal\simpletest\WebTestBase->setUp()
#21 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(381): Drupal\simpletest\TestBase->run()
#22 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('515', 'Drupal\user\Tes...')
#23 {main}FATAL Drupal\user\Tests\UserCancelTest: test runner returned a non-zero error code (1).

Clarifying #23, this straight up blocks #1805996: [META] Views in Drupal Core. Whenever you enable Views--not only in tests but in the actual site environment--you get the error above.

Blocked Views is bad/

Related: #1331486: Move module_invoke_*() and friends to an Extensions class

Oopsie.

Also related: #1759582: Figure out when to delete compiled DIC files, which is a follow-up for #1706064: Compile the Injection Container to disk, which is hopefully going to be RTBC'd shortly :-)
I'm planning to reroll #1331486: Move module_invoke_*() and friends to an Extensions class today. That's not a blocker for this issue, it's just that if we're rethinking the whole module_enable process, it probably makes more sense to do so with the work in that patch in mind.

This can't be solved without #1706064: Compile the Injection Container to disk but it's blatantly trivial with it so moving there.

Ok let's not merge this yet. It needs to be postponed on that one, however. http://drupal.org/files/interdiff_1320.txt is the patch that will solve this issue, likely.

wscci-bundles-module_enable.patch queued for re-testing.

oops !
sorry, I was trying to figure out how to make VDC works.
I get that error when I try to install the sandbox :

    Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "plugin.manager.views.display" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 690 of /var/www/vdc/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).
    Notice: Undefined index: backtrace in _drupal_log_error() (line 188 of core/includes/errors.inc).
    Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "plugin.manager.views.display" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 690 of core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).

Appliying http://drupal.org/files/interdiff_1320.txt work for me (with the last VDC sandbox)
even if it gives me that errors:

    Warning: Missing argument 1 for system_list(), called in /var/www/vdc/core/includes/module.inc on line 481 and defined in system_list() (line 142 of core/includes/module.inc).
    Notice: Undefined variable: type in system_list() (line 148 of core/includes/module.inc).
    Notice: Undefined variable: type in system_list() (line 263 of core/includes/module.inc).
    Notice: Undefined index: in system_list() (line 263 of core/includes/module.inc).

but now the views UI are accessible !!! I can use Views, which was impossible before this patch.
my frontpage is back also :-)

Thanks for testing @ybabel! And yep, this is the issue currently blocking the sandbox from working smoothly.

Re-postponing on #1706064: Compile the Injection Container to disk. Once that is in, this issue can be fixed, and then we can continue work on the sandbox issues. You can help us discuss anything you encounter in the sandbox queue. :)

#9: drupal-1708692-9.patch queued for re-testing.

Rebased #9 and tweaked the docs.

Merged the test and a slight modification of @chx's solution from #30 and simplified the test a bit to just check for the existence of the service id. Also added a test for module disable (which also fails). Tried to rebuild the container as well but for some reason it did still not pass. Suggestions welcome :)

PS: I am not 100% sure if it is okay to go with $modules_installed instead of doing a fresh system_list/module_list call. It should be the same actually.

Trying something.

Okay I am way out of my depth so I didn't get very far. But here's one fail fewer!

I tried to also do the rebuild in module_disable() but couldn't get that assertion to pass, so left that as a @todo for now.

We need to pass in all modules, not just the one that we just enabled. This patch is based on the approach of chx's interdiff in #30 and adopted to the way kernel compilation works now.

Oh, yes, that looks much nicer ;) Wasn't aware that the kernel is in the container as well aka forgot that.

Yes, there's an issue to make an install-time kernel/router and and clean up our current mess.

Also, I understand the problems outlined in those initial comments (I think) but changing module_install() to require a site redirect sounds like a huge change which would significantly change the way many things in Drupal work (like tests, for example). For me personally, that doesn't sound feasible at this point in time.

This looks great to me.

Fixing the apparent problem will not disallow us from rethinking in-process/intra-request extension management. However, I do agree with previous posters that it appears rather unlikely for us to be able to completely revamp this logic for D8, if that is even possible in the first place. If someone wants to investigate that, create an issue for it. If not, well, we're good.

Good job Alex. That's indeed much cleaner.

This looks great - my main concern had been that you'd end up with a new container that wasn't the same one being referenced by the HttpKernel when managing scope. But I did a quick test calling drupal_container()->isScopeActive('request') after a request that enabled a module and everything seems to be in order, which has to mean that leaveScope('request') was called on the correct container. Nice one Alex! :-)

Looks like we have a winner! :) Great work, folks!

Committed and pushed to 8.x (with a minor tweak to remove the () from (array) in the @param docs in DrupalKernelInterface)

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -110,6 +110,20 @@ public function registerBundles() {
+  /**
+   * Implements Drupal\Core\DrupalKernelInterface::updateModules().
+   */
+  public function updateModules($module_list) {
+    $this->moduleList = $module_list;
+    // If we haven't yet booted, we don't need to do anything: the new module
+    // list will take effect when boot() is called. If we have already booted,
+    // then reboot in order to refresh the bundle list and container.
+    if ($this->booted) {
+      drupal_container(NULL, TRUE);
+      $this->booted = FALSE;
+      $this->boot();
+    }
+  }

Naive question, might very well be completely non-applicable: is there a way to rebuild the container only when needed, rather than force-rebuild for each module that gets enabled ?
I'm thinking of cases where you enable N modules at once, leading to N successive calls to kernel->boot().

I.e "setting 'menu_needs_rebuild' rather than calling menu_rebuild()" pattern.

IIRC, views UI needs bundle services from views, so that wouldn't work for us, while this does.

Thanks to everyone who worked on this!

I have to reopen this one, as it caused a huge performance decrease.

It's the reason test runs now take 40++ minutes.

The changes in #512026-87: Move $form_state storage from cache to new state/key-value system from @effulgentsia seem to bring test runs back to 34 minutes but also seems to break the tests while doing so and leads to segmentation faults.

We should try to fix that in here I think, without the additional complexity added over there to see if those errors are caused by the keyvalue stuff or this.

I think it doesn't really matter if it's critical or major, it being major is right now actually a bigger problem because we "only" have 11 critical bugs but 102 major ones :)

Thanks for the clarification on compile vs. dump. I agree that it makes sense to first fix (and actually understand) the problem and only then optimize it. Still, we're talking about a 30% performance decrease on the test execution time, that's serious.

Related: #1530756: [meta] Use a proper kernel for the installer

The goal is to make the installer its own micro-app, which would, hopefully, eliminate some of those issues. (It wouldn't help with enabling modules post-install, of course.)

Re-uploading a slightly modified version of effulgentsia's interdiff/patch from the form_state that should pass the tests, to see how much the difference is of this.

array_diff() is ... interesting.

array_diff($this->getBundleClasses($this->moduleList), $this->getBundleClasses($old_module_list)) is not the same as array_diff($this->getBundleClasses($old_module_list), $this->getBundleClasses($this->moduleList)).

What array_diff() seems to do is return the array values of the first array that are not in the second, but not the other way round. "$this->getBundleClasses($old_module_list) != $this->getBundleClasses($this->moduleList)" on the other side, seems to be working as expected.

This includes an issue I've found with old compiled containers being used when enabling, disabling, then re-enabling a module that provides a bundle.
It fixes failures in EnableDisableTest for the blocks-as-plugins patch.

I also added doc blocks.

#58: only-reboot-kernel-if-necessary-1708692-58.patch queued for re-testing.

My change in #59 is obsolete now that #1831350: Break the circular dependency between bootstrap container and kernel is now in.

I guess that #58 needs a reroll now.

I tried to do that but failed :)

As far as I can see, a similar check is now built into initializeContainer() and we do not attempt to rebuild if we either already have a dumped container (I don't quite understand yet how that plays together with $this->newModules ?) or the new modules differ from the old ones.

Maybe @chx can clarify...?

As discussed in IRC with berdir, the check inside of initializeContainer needs to be moved to the updateModules to not rebuild the container in case nothing has changed.

So this patch is still valid.

Aside from the not applying part?...

not the patch itself is still valid, but the idea of it :)

Ok, here is an attempt to do that. If we don't want that, because we also want to execute the second check ther, then an alternative would be to keep that helper function (added api docs from timplunkett's patch, thanks!) and use it for the existing check + make sure that the current container is only thrown away when we actually want to rebuild. Or so...

BundleTest passed, let's see about everything else.

Minimal change, for testbot only:

This is definitely still wrong, but the logic is flawed for rebooting anyway:

    // Merge in the minimal bootstrap container.
    if ($bootstrap_container = drupal_container()) {

At this point however drupal_container() contains our complete rebuild container ... (which will be merged in and not the Bootstrap Container)

I vote for a container_needs_rebuild flag in the Kernel, which updateModules can set and initializeContainer can use and then call drupal_container() with the right flag.

for testbot

And another patch that might work better:

And another patch

Give me a green light ... please :)

Testbot: I need another result, pleeeeeeease?

Really, saving the moduleListChanged as a separate var is way cleaner and more inline with the original code, while still preventing the rebuild :-).

Is there a reason you left out the getBundleClasses() part? That allows us to only do a reboot if the changed modules actually are providing bundles.

#77: Yes, the getBundleClasses was not working and I was trying to understand the logic ...

Patch with re-included getBundleClasses(), but new logic from #76.

Passes the relevant tests locally and seems faster. Lets see about the rest of Tests.

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -203,11 +212,36 @@ public function updateModules(array $module_list, array $module_paths = array())
     if ($this->booted) {
-      drupal_container(NULL, TRUE);
-      $this->booted = FALSE;
-      $this->boot();
+      if ($this->moduleListChanged) {
+        drupal_container(NULL, TRUE);
+        $this->booted = FALSE;
+        $this->boot();

If you combine these two conditionals into one, then you don't need to indent. Otherwise this looks good. Do we have any benchmarks to prove it actually helps?

If you combine these two conditionals into one, then you don't need to indent. Otherwise this looks good. Do we have any benchmarks to prove it actually helps?

Good catch! Done :)

How do you benchmark _that_?

The issue title already states how this is to be benchmarked.

Unfortunately, the latest patch has zero impact on the total test suite time.

Actually, it works just fine. It's just that it doesn't have a big impact anymore* and the testbots aren't a reliable indicator.

Confirmed with xhprof, see attached screenshot. Did run the same test with and without this patch, both had 7 calls to this function and without the patch, this resulted in 7 boot() calls. With the patch applied, only 3 calls to boot(). The inclusive wall time difference however is just 3% to 1,5%. And this is a relatively fast test (Actions configuration, first in the list) so the actual setUp() makes up a large amount of the total time (70%, to be exact).

I think this can be changed to priority normal, atleast the simpletest part of it. And we don't have a clear definition for the "other problems" except that the the additional container rebuilds (which are necessary) might or might not be causing my segfaults.

The only thing that could maybe be improved here is saving the bundle classes list of the current module list so that we don't have to re-check for that, which should remove half of the calls to getBundleLList().

*The reason for this is that the kernel patch that went in already had a check for exactly this, we don't really save much here except of an unecessary rebuild of the bootstrap container.

I am fairly confident that if we want to speed up simpletest then we need to save the list of profiles and as much of modules as we can to stop the astonishing amount of file scanning we do. This would require to remove the info handling from system_rebuild_module_data , or , indeed the removal of system_rebuild_module_data as it is... stay tuned, working on that.

so what is needed here to fix?

Maybe this change would help to catch the actual errors that could be tested only manually for #1919002: Upgrade to D8 broken when D7 has more then one language enabled

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

#2033567: Remove TestBase::rebuildContainer() is a followup on the original issue here, though it doesn't seem that "module enable/disable changes are immediately reflected in drupal_container()" as it was expected in TestBase::rebuildContainer().

WTH? I didn't remove that tag.

#81: core--only-reboot-kernel-if-necessary--1708692--81.diff queued for re-testing.

Reroll of #81.

Seems that HEAD may be broken. On both my local dev machine and on simplytest.me I get this doing a fresh install of 8.x – I think that is why the patch fails.

This is the message I get:

#92: core--only-reboot-kernel-if-necessary--1708692--92.diff queued for re-testing.

Nope, 8.x works fine. The patch has a problem.

That's because a lot has changed since that patch was written, multiple times :)

Bundle classes don't exist anymore, instead we have services.yml files and service provider classes. And more importantly, the container contains a list of module namespaces, used to look for plugins. I don't think what this patch is trying to do is still possible.

Moved to simpletest.module per #2050763: Refine "base system" component.

@Berdir made the case that this doesn't actually have anything to do with SimpleTest, so we agreed in IRC to move it back to base system for now.

@effulgentsia is also going to reassess this issue in light of the complete change to the affected architecture, so assigning to him. Thanks everyone!

Is this still relevant?
If so, can we consider opening a new issue? This was committed in #51 (October 2012!), and it's hard to understand the current status of the issue.