Stuck in failed module install process

Yes, its bad that you can end up in a bad situation, where you have Drupal in a kinda broken state
but I'm not sure what we exactly should do about that. If your code is broken, it can be broken, in multiple ways.

Maybe we could rearrange the process a bit to at least not fail when either the .install or .module code has syntax errors, but I'm not sure how much this really helps
with the general problem, that you should not use broken PHP code at random point in time.

Given that it seems to be more a major issue, to be honest, but no question, reducing the fragility would be kinda a big win in general.

I was actually discussing this a couple weeks ago and have a solution for this that should work, though it's a bit on the weird side. The idea is to use a system call to php itself.

  // Use system to check file parse integrity.
  $status = null
  exec('php scripts/inspector.php ' . path_to_file_to_inspect, $status);

inspector.php would be a very simple script that includes the file, then returns all newly defined constants and functions. The main script checks these new declarations against the current symbol table to insure there will be no collision, and if so proceeds with a normal require of the file.

Another possible solution is to create a 'complete' flag of the process, and set that dead last. If the complete flag doesn't get set we assume a parse error occurred during the install process and uninstall the module.

Another note - register_shutdown_function can be used to prevent this problem by catching even a parse error and cleaning up the incomplete install state, but Drupal's current use of the function needs to be completely and utterly redone before it can be used for this situation. I suppose the process could overwrite the default drupal register shutdown function, but the ramifications of that are hard to detect. This is one of the pieces of the full fault system I'd like to work on.

I can begin work on this if desired.

So what happened in D6/D7 if you started with a broken PHP file?

Yeah, the state probably shouldn't be saved until the end. There are plenty of ways to screw up your site with broken code, but letting it happen due to a syntax error just seems trivially avoidable...

Agreed that this is major since it will only occur when the module has broken code -- however, making it less fragile so that sites don't end up in a broken state sounds quite important. (I imagine there would be other ways to fix it than dropping the database, but we should explore that.) Thanks @dawehner and thanks @hass for the report.

Some more specific steps to reproduce this would also be helpful, e.g. with a small test module that reproduces the issue.

I don't think we need to check for syntax errors; that's a lot of babysitting. Module authors should check the syntax of their own code before deploying it to production (or pushing it to d.o).

How should I check if there are syntax errors if I cannot enable the module that depend on other modules and core? *confused*

hass, the disconnect here is that most (if not all) the core developers use PHPUnit to test the code before deploying or activating. It is recommended you write unit tests for all the classes of your modules - that will catch this sort of thing.

Long term the Fault System can handle this - register_shutdown_function exists for this reason. But that version of the fault system has to wait for 8.1.x because the current reg shutdown function system is messy.

Nope. That's actually backwards. Test Driven Design involves writing the test first then writing the code to fulfill the task set forth by the test.

Aki Tendo is referring to a methodology called test-driven development http://en.wikipedia.org/wiki/Test-driven_development where you write tests first to essentially document the behaviour you expect, show them to be failing, then fill in the code that makes them pass.

It should not be a requirement to do TDD in order to create Drupal modules, however. Plenty of people develop their code iteratively within their existing site, and we should not leave them in an unrecoverable state because of a simple typo.

+1 for webchick. To be accessible, it should not take to be software engineers to make your custom small piece of module for your personnal website. POO is surely already scarying some amateur, imposing a process in addition would kill the number of module contributers (at least on my opinion of course). Of course it is a nice step for industry processes, but Drupal should keep affordable for novices and amateur (or should they consider Wordpress instead :p )
@hass, I am surprise from this : "Are you a developer?" Because it does not sounds respectfull (at least translated in french, maybe it's ok in english). Moreover, as a developer, I am actually surprise that you don't know about LDD... just saying :)

Just for manual reproduction purpose, here is a zip containing the most trivial (and bugged) module ever.
Activating it will give you a syntax error. When done, you won't be able to uninstall the module, but it will keep activated.

Add a failing test patch. It checks that a wrong module is not installed.

NOTE: it catches the false-negative thought if someone knows how to fix this :

+1 to webchick and to Dom. Also Dom., I wasn't implying we should force anyone to TDD. Indeed, I have my lazy days of iterative development, especially on real small projects (or untestable code like WordPress). I was just pointing out that the habit of most of the core devs using TDD is likely the reason they've not come across this problem before - and it is indeed a problem that needs to be addressed.

How should I check if there are syntax errors if I cannot enable the module that depend on other modules and core? *confused*

php -l foo.module

I love TDD, so it's my pleasure to make the test ! I just don't know how to suppress this false-negative in the test.

NOTE: it catches the false-negative thought if someone knows how to fix this :

Not sure how to best let bad_module cause a WSOD without itself falling afoul of the test suite... but an exit(1); call in the global scope might have the same effect as a fatal PHP error without actually registering as one for the test.

Otherwise, it might be hard to get simpletest to treat this specific error as exempt.

Try to throw an exception in hook_install() or something like that should have the same effect.

Adding this inside hook_install() :
throw new Exception("PHP syntax exception !");
But it is not catched, so changing test to :

// Enable bad_module module.
try {
  $edit['modules[Testing][bad_module][enable]'] = TRUE;
  $this->drupalPostForm('admin/modules', $edit, t('Save configuration'));
  $this->fail("bad_module should fail installation process.");
} catch(\Exception $e) {
  $this->pass("bad_module fails installation process.");
}

fails to "bad_module should fail installation process." with :
Uncaught PHP Exception Exception: "PHP syntax exception !" at core\modules\system\tests\modules\bad_module\bad_module.module line 9

@Berdir: did I miss your point ?

No, you didn't, but you can't catch the exception that's thrown in the page request like this, that's a completely different process :)

What you're seeing in the log is not actually an exception, it's the log of an exception that happened on the page request, which is transported through http headers and displayed by Simpletest. You need to prevent that in this case, because we expect this exception.

ErrorHandlerTest is doing something similar, by removing the assertion there. We also have other implementations, can't find them right now.

Hum... No more false-negative that way... But because the error is no more expected, the module is now installed nicely...

@haas +1 It happens, true story :)

Actually, it is really a nightmare for DX. For instance, I am struggle with with doing a module with some authentication provider override. It messes at install, the only way I found is to reinstall my whole D8 platform between each try.
Can someone help on this ?

I agree that this is a huge DX problem. The first time this happened to me, I was also left wondering what was going on. And I'm working on core full time.

Question - what are the ramifications if we overwrite the register_shutdown_function during this process? That function can catch parse errors and other fatals and perform a cleanup operation as necessary, so it's use is the best solution for this problem.

Part of my long term goal for 8.1 is to rewrite the functions that interact with it as part of the fault system - but to do that I need to know what handlers are currently registered on the shutdown process, if any. There are some shutdown related functions in bootstrap.inc, but they look like they are only used during batch processing.

Attached patch works locally, so lets see if it gets through a automated tests. There are a lot of question marks still with this patch, but I think it's a step in the correct direction. Someone more familiar with the module install process needs to review this.

The largest change is a considerable amount of refactoring. This was done to optimize the process, but also to allow me to granularly pull pieces of the uninstall process into the rollback process.

My largest concern is there are parts of the uninstall process that isn't being ran by the rollback because of PDO errors being thrown. I'm thinking this is occurring because the database connection is being closed by the time the shutdown handler executes. Further testing is needed.

Note - this patch includes the tests Dom uploaded at #28.

I'd recommend to not rewrite the code and fix the bug at the same time. It just makes it really hard to review, which is not something you want,
as otherwise you get stuck in your changes.

Agreed. Pulling it apart like that and putting it back together was part of getting a handle on how the system works though. Given the number of test errors I'll start afresh starting only with the rollback method and it's required hooks. I should have that ready by end of day tomorrow.

K, rebuilt with the previous bugs worked out (I think). There is now no overlapping code with the install and uninstall methods. No interdiff - this was a clean start over with only additions being made to the file.

Test patch at #28 included in this patch.

I'm unable to duplicate these fails locally, and after reading the code of the tests in question I have no idea why they would be affected by the addition of the rollback in ModuleInstall unless they have register_shutdown_function handlers that are being overwritten, although that shouldn't happen.

Trying again after diffing against most current code possible. Still no local errors on failed tests so hopefully the last batch of fails was just a weird hiccup.

Something really weird is going on - #43 picked up changes I didn't expect to be there. Trying again, this time making sure the diff only has what it should have which should make it identical to #39. If this doesn't work then I'm going to be really confused cause again, I've ran all of these tests local with no failures.

The last five failing tests are running out of memory. Anyone have any idea why??

I have a theory that the register shutdown handler was getting registered repeatedly due to multiple calls to ModuleInstaller::install() Code adjusted to prevent this. If I'm right, we're green.

I have a theory that the register shutdown handler was getting registered repeatedly due to multiple calls to ModuleInstaller::install() Code adjusted to prevent this.

That totally makes sense!

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -52,6 +52,27 @@ class ModuleInstaller implements ModuleInstallerInterface {
   +  protected $module = '';
   

   what about naming the var like that: moduleInstallAttempted?

2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -291,6 +331,168 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +   * There is an existing drupal shutdown handler - it appears to be intended
   +   * for use with the batch system and it isn't even registered during normal
   +   * production. If that's not true then this handler will still run, but only
   +   * after the handlers set by that function - PHP executes shutdown handlers
   +   * in FIFO order.
   ...
   +   * At some point these need to be integrated together but since it's so close
   +   * to the end of the beta cycle an approach of do the least change is being
   +   * pursued.  For that same reason this code is duplicated, almost entirely,
   +   * from the uninstall function below. There are differences though - not the
   

   That kind of documentation seems a little bit odd. Do you really think that someone reading that, would understand it easier?

3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -291,6 +331,168 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +    if (static::$activeInstaller) {
   

   Should we check for an isset()? I guess it doesn't matter.

Updated issue summary to add the beta change template and additional remarks. Also, we're GREEN!

what about naming the var like that: moduleInstallAttempted?

When I read that I expect a boolean variable, not the name of the module currently being installed. Also, when the refactor is done this variable will carry the name of the currently being installed or uninstalled module between the various protected methods even when we aren't in the rollback method.

With that in mind should I still change it?

That kind of documentation seems a little bit odd. Do you really think that someone reading that, would understand it easier?

That's meant to help the reviewer of the code and should probably be dropped before actual release. Wait, that gives me an idea - separate ticket for that though.

Should we check for an isset()? I guess it doesn't matter.

Up to you guys.

I put a bad function call in bad_module and tried to install it to make sure the rollback will fire even in that eventuality. It did, but the rebuild script had to be ran before the system recovered and the error screen was non-standard. Additional changes are going to be needed and since the unit testers cannot currently negotiate a situation with a true parse error these circumstances will need to be hand tested.

K, I have it running nice and happy now. I've thrown every lead balloon at it I can think of - parse errors, bad function calls, type hint fails, everything but an engine crash and it's recovers just fine.

I also added proper event logging - any failure during the cleanup process will be logged and the fact a rollback occurred at all will be logged.

Finally the registration of the shutdown function now passes through drupal's drupal_register_shutdown_function()

Unfortunately I couldn't find a way to get the unit testers to automatically check the parse and fatal error conditions. The value of the exception catch check is dubious since of the three fatal failures that can happen that is the least likely. But it does check the integrity of the rollback function itself so I suppose that will have to do.

K, we're green. I noticed while reviewing the interdiff one last time that I was calling module clear twice in the rollback method. It passed tests despite this error, but the attached patch pulls that redundant call.

Tested and confirmed that the patch works as promised.

I think this needs to be reviewed by at least one more person with deep knowledge in this area. I think @dawehner would be a great candidate.

Also, why only do this for modules? Why not also for themes?

My review:

1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -52,6 +52,27 @@ class ModuleInstaller implements ModuleInstallerInterface {
   +  protected static $handlerRegistered = FALSE;
   

   "handler" is very generic, let's call it $shutdownHandlerRegistered.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -278,6 +313,11 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +        // uninstalled on script closure.
   

   "script closure" -> that's a term we don't use anywhere else.

3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -291,6 +331,210 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +   * install hook, crashing the system. register_shutdown_function provides us
   +   * one opportunity to clean house in this event.
   +   *
   

   s/provides us one opportunity to clean house in this event/allows us to revert to the state prior to the module being installed when that happens/

4. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -291,6 +331,210 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +   * We do not run any hooks from this method. We cannot risk a second fatal
   +   * error which would leave the system in an unrecoverable state.
   

   Excellent comment :)

5. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -291,6 +331,210 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
   +  protected function rollback() {
   

   This method makes me ask many questions:

   1. This is all new code. Isn't there any possibility of sharing code with the pre-existing uninstall() method, possibly with a flag to indicate rollback?
   2. Why do we catch exceptions on every single step individually? We log exceptions in the exact same way for every step. So why not just wrap the entire logic in a try { … } catch {… }?

6. +++ b/core/modules/system/src/Tests/Module/InstallTest.php
   @@ -80,4 +80,34 @@ public function testModuleNameLength() {
   +   * The module to be enabled will produce a PHP SynatxError during install.
   

   s/SynatxError/syntax error/

7. +++ b/core/modules/system/src/Tests/Module/InstallTest.php
   @@ -80,4 +80,34 @@ public function testModuleNameLength() {
   +    // Check the syntax error occured.
   

   s/Check the/Check that the/

8. +++ b/core/modules/system/tests/modules/bad_module/bad_module.module
   @@ -0,0 +1,21 @@
   +  // Prevent test from collecting errors.
   +  define('SIMPLETEST_COLLECT_ERRORS', FALSE);
   

   Interesting, why?

9. +++ b/core/modules/system/tests/modules/bad_module/bad_module.module
   @@ -0,0 +1,21 @@
   +  #call_to_no_func();
   ...
   +  #Random parse()-error generating text
   

   We don't ever write comments using #, we always use //. But I see why you want to do it here.

Also, why only do this for modules? Why not also for themes?

To my knowledge themes have not, for whatever reason, had a problem with getting into a locked up state where they can't be uninstalled or reinstalled. If that is not the case I'd be glad to do this with the theme installer, but that's a different issue ticket from this.

1. Done
2. Changed to "uninstalled at script termination by the shutdown handler."
3. Done
4. Thanks.

5.

This is all new code. Isn't there any possibility of sharing code with the pre-existing uninstall() method, possibly with a flag to indicate rollback?

That's coming, but will be done on the child ticket as part of an overall refactor of the code as recommended by dawehner at #36. The code used by rollback isn't all new - it all comes out of uninstall actually, just broken up into steps.

Why do we catch exceptions on every single step individually? We log exceptions in the exact same way for every step. So why not just wrap the entire logic in a try { … } catch {… }?

Each part of the uninstall process is independent - just because one fails does not mean the subsequent ones will and they should be ran. Now if I'm incorrect in that statement then yes, the block should be merged. Otherwise the breakup gives each part of the process a chance to run. Where they merged any steps following the one that kicked an exception would not be ran. It's wordy, but the method of consolidation - iterating over an array of function names, isn't particularly fun to maintain either.

Also, the repetition drills home to the reader that if this code is running then something bad has happened - hence the paranoia.

6. Done
7. Done
8. Dunno - Dom noted something about preventing a false negative in #28. He wrote the test code.
9. I've actually seen this one other place - default.settings.php - the block of code which links up the settings local file is commented out in Perl style. I presumed that, on the rare occasion we need to toggle code on or off by hand, we're supposed to use Perl comments to do that.

While PHP syntax errors do occasionally occur, would it be possible for this issue to also include tests for typos in entity/field config files and annotations? Those situations also result in the need to reinstall Drupal.

For example:

• A field.field.node.article.test_field.yml file could contain a typo in the entity_type resulting in an entity not found-like exception during installation.
• A ContentEntityType annotation could also have a typo in the base_table entry, which would probably result in a SQL-related exception.

If you or someone else could give me an example of such a flawed config I'll add a test just to be sure. But this patch should recover from that situation without further work. Still, it never hurts to check.

#60 could be addressed in a followup.

Also given the reported error and the complexity we're introducing with the current patch has anyone investigated why uninstall is not possible?

So I've just tested enabling the bad module provided by the patch without the fixes to the ModuleInstaller. It crashes on install as expected. I go to admin/modules/uninstall and uninstall it and hey presto everything works. I'm really not sure that the behaviour here is any different from d6/d7. Yes if I had a syntax error in the global namespace in the module's .install file I would have a problem that I'd need to fix - but the same would be true in d6 and d7.

phenaproxima had a similar problem getting the bad_module test to create a lock up situation. Try again introducing an outright parse error to the included bad module instead of just an exception throw.

In the meanwhile I'm going to look at this incongruity a bit closer. If the test scenario isn't spot on there could be a problem.

afaik if we have an outright parse error in the global php space in the install file you wouldn't be able to uninstall in d6/d7 either.

Ok, I installed the tests only patch again and tried to install bad_module. Got the exception as expected. Was able to uninstall. I then deleted the closing } of the install hook function and tried to install. Crashed with parse error as expected, and could not uninstall because the module isn't listed in the uninstall list.

I have hand tested the patch against this scenario and the patch prevents this lock up situation from forming.

The unit test needs to more closely replicate this, but since we're dealing with a deliberately introduced parse error I'm unsure of how it can be done.

Whether d6/d7 has this problem is irrelevant - it's still a bug. If anything it's an argument to backport the fix to d7 at least.

Also, I'll guess that d6/d7 allow you to disable the module. d8 doesn't have that option since it was removed.

Well... if you edit any file in core and break it - it is broken until you fix it. You fix the syntax error and you can uninstall the module.

Yes even if you don't know any PHP, you can just empty the .install file to <?php and uninstall.

And D6/D7 will load the .install file on disable iirc - worth checking, but I don't think that'll work where uninstall doesn't - will still be syntax error.

If you correct the error in the bad_module, does it then becomes uninstallable?
If confirmed yes, this could be :
1. okay for UX because you contact the module maintainer, ask for it to be corrected, and finger-cross this will happen some day.
2. okay for DX because also you don't have to crash your whole dev platform to solve the situation.

Well, I tried that out but correcting the parse error after the install attempt fails does not allow the module to be uninstalled.

Well... if you edit any file in core and break it - it is broken until you fix it.

I didn't edit a core file, I removed the closing } of the module's hook_install method.

@Aki Tendo thanks for providing instructions to reproduce. Here's a fix.

That will it allow it to be uninstalled and certainly needs to be applied, but wouldn't it be more intuitive from a UX perspective not to allow an install to occur to begin with?

Well, I've been studying ModuleInstaller::install() and related functions to figure out what's going on and here is a condensed version of what I have found out:

The important stuff starts with:

// Check the validity of the default configuration. This will throw
// exceptions if the configuration is not valid.
$config_installer->checkConfigurationToInstall('module', $module);

While the comment suggests that this function checks config validity, it does not. It merely checks if config dependencies are met. This means that config typos and such get much further into the install process.

The very next line:

$extension_config
  ->set("module.$module", 0)
  ->set('module', module_config_sort($extension_config->get('module')))
  ->save();

Sets the module list returned when you call Drupal::config('core.extension') and essentially marks the module as installed. Files from modules to be installed are then loaded next. This is where PHP syntax errors crash the install process.

Notably, that module list is used in system_rebuild_module_data() which is called by the ModulesUninstallForm, but is never actually consulted for a module's status. Only the schema state is checked. If an error occurred during the module install, this would never have been set.

So basically the patch in #73 replaces the schema check with a status check, which is what it should have been doing to begin with. I have also tested that it does in fact allow you to uninstall modules with invalid config files and would probably work with other install failure types as well since the status is set before anything bad can happen.

So I guess the rollback ability is no longer needed. Is it wanted?

Hmm, while that rollback implementation does a pretty good job of undoing a lot of potential things a module could do to the Drupal install, there is no way for it to be sure that every possibility was covered. The only way to do that is by backing up the database and active configuration prior to the install attempt and then restoring it afterwards.

For a developer, it might be nice during early development (and to that end you could create a replacement ModuleInstaller implementation for devel or something), but just fixing the uninstall page is usually good enough for most situations.

Includes #73 and a test for the uninstall page.

#64: That would be splendid, if we would be able to guarantee that a module that was unable to correctly/fully install, would always show up on the "Uninstall" tab. But as long as that is not guaranteed, we'll need to do something like this issue. The problem is that even despite silly things like parse errors, Drupal 8 chooses to list a broken module as "installed", even though it really isn't, and then does not offer the ability to uninstall! That surely doesn't make any sense?
Personally, I would also strongly prefer not making things more complicated, and instead offer a strong guarantee that modules either fully install or not install at all. In other words: atomicity.
But can we guarantee that?

Looks like #73 fixes it at the surface, but the crux of the problem is clearly explained in #75.

Curious to hear what Alex Pott has to say about #75.

The assumption here seems to be that this problem is only the result of developer error - a syntax error or the like. That's NOT the case (although I admit a syntax error is probably the most common cause). This has happened to me many times when core gets updated causing my syntactically valid and working code to fail, so this is something that can affect even a non-developer who is simply doing normal site maintenance and keeping things up-to-date.

As a recent example, just this week the install process crashed while trying to install one of my modules because of the core change #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols). Install failed yet my module was listed as installed and it couldn't be uninstalled. I don't want to pick on that issue - this has happened to me a hundred times, that's simply the most recent.

Attached is a test module without a syntax error that will fail during install and subsequently can't be uninstalled.

Patch #73 allows me to uninstall this test module after the install fails.

Marked #2467485: Unable to uninstall custom module as a duplicate.

I couldn't apply the patch in #78 so I remade it and fixed up some code styles.

re #79 I don't think at this point we can make a module install atomic and properly rollback-able - too much is possible and we have no control.

Personally I think we should get this in ASAP to prevent people getting frustrated developing on D8.

@alexpott for #82: +1 !! this happens indeed and indeed it gets frustrating !

Patch in #82 applies cleanly to HEAD and solves the issue by allowing modules to be uninstalled even if there was a fatal error during the install. The SimpleTest works to test the fix. I did not run the PHPUnit test.

Trying to make module installation transactional is perhaps a worthy long-term goal, but that's a few steps beyond what is needed here.

Thanks everyone. The latest patch is a nice self-contained fix with good test coverage. I think it's legitimate to test this using a specific exception during installation (rather than a syntax error) since the functional result is the same.

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x.

I added some slight docs polish on commit:

diff --git a/core/modules/system/src/Tests/Module/UninstallTest.php b/core/modules/system/src/Tests/Module/UninstallTest.php
index a13366f..8730522 100644
--- a/core/modules/system/src/Tests/Module/UninstallTest.php
+++ b/core/modules/system/src/Tests/Module/UninstallTest.php
@@ -120,8 +120,7 @@ function testUninstallPage() {
   }
 
   /**
-   * Tests that a module which fails to install is still shown on the uninstall
-   * page.
+   * Tests that a module which fails to install can still be uninstalled.
    */
   public function testFailedInstallStatus() {
     $account = $this->drupalCreateUser(array('administer modules'));
diff --git a/core/modules/system/tests/modules/module_installer_config_test/config/install/module_installer_config_test.type.missing_id.yml b/core/modules/system/tests/modules/module_installer_config_test/config/install/module_installer_config_test.type.missing_id.yml
index 2d941b0..4628b97 100644
--- a/core/modules/system/tests/modules/module_installer_config_test/config/install/module_installer_config_test.type.missing_id.yml
+++ b/core/modules/system/tests/modules/module_installer_config_test/config/install/module_installer_config_test.type.missing_id.yml
@@ -1,3 +1,3 @@
-# This should have 'id' and during install, you will get an
-# EntityMalformedException complaining about the missing key.
+# This entity is intentionally missing an 'id' key. During installation, an
+# EntityMalformedException should be thrown about the missing key.

This did break in D7, but I doubt its going to get fixed now,

#957998: Module install failure results in enabled module?

FWIW this (or a similar issue with same symptoms) persists into Drupal 8.6.x

If you're installing a large collection of modules - Commerce for example - Drupal will eat up all your PHP memory and die before it completes installation, but AFTER it updates config, and it won't roll back.
This kills the Drupal.

I'm still looking for an open issue on it, will post here if i find one.