Regression: Site broken when disabling module without removing block instances.

We already handle uninstall with hook_modules_uninstalled(), we should probably just move everything to the disabled region in hook_modules_disabled().

Also, will be easier with #1871696: Convert block instances to configuration entities to resolve architectural issues

#1 sounds sane to me.

Sounds like another case for #1826602: Allow all configuration entities to be enabled/disabled to me.

Normally, we do not touch the configuration of a module when it is disabled or enabled. If you'd move all block instances of a module to the disabled region, then we're actually destroying the configuration that existed for the disabled module. Thus, for many modules, the net result would be the same as if you had uninstalled the module.

As a result, I think we actually need a 'status' or 'enabled' property in the config of block instances.

Alternatively, the block system should be smart enough to figure out that the plugin-providing module of a block instance is not available and thus silently skip over the instance. That sounds like the proper answer, actually.

Hm, I changed my mind actually. #1 is the only reason so far to keep the disabled region, and it would result in having to manually put your blocks back in the right place when re-enabling the module. Edit: It might be okay as a stopgap fix though, given a followup to fix it properly.

This issue is actually a specific case of a more general problem: #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API.

So here's one way we could fix this:

1. #1871696: Convert block instances to configuration entities to resolve architectural issues
2. #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API
3. #1826602: Allow all configuration entities to be enabled/disabled
4. Hook implementation config_modules_disabled() disables the ConfigEntities.

Oh, I crossposted with @sun.

Alternatively, the block system should be smart enough to figure out that the plugin-providing module of a block instance is not available and thus silently skip over the instance. That sounds like the proper answer, actually.

This is very similar to Views' "broken" handlers... handling, which we're considering removing: #1822048: Introduce a generic fallback plugin mechanism.

AFAICS, block plugin instance configurations already contain a "module" key, so slap a module_exists() onto that, done?

Here's what that would look like. I'm not sure this is the right solution:

• This can happen with other kinds of plugins, so fixing it in block.module is questionable.
• This can happen with other kinds of configuration, so fixing it in block.module is questionable.

Also, _block_load_blocks() needs to die.

Duh.

+++ b/core/modules/block/block.moduleundefined
@@ -476,7 +481,16 @@ function block_list($region) {
+    catch (PluginException $e) {
+      $config = config($plugin_id)->get();
+      if (module_exists($config['module'])) {
+        throw $e;

@@ -493,9 +507,16 @@ function _block_load_blocks() {
+      if (module_exists($config['module'])) {
+        throw new PluginException(sprintf('The plugin %s was not found.', $plugin_id));

Shouldn't there only be one module_exists()? Ideally, around the original throw, so it doesn't have to do config($plugin_id)->get() again. Alternately, we could subclass PluginException.

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.phpundefined
@@ -379,4 +379,31 @@ function testBlockRehash() {
+  function testBlockModuleDisable() {

public function

Shouldn't there only be one module_exists()? Ideally, around the original throw, so it doesn't have to do config($plugin_id)->get() again. Alternately, we could subclass PluginException.

Remember in IRC when I asked you to kill _block_load_blocks()? This is why.

public function

Since half of core does this and half does not, I see no reason to until a definitive standard is adopted.

Moving the module_exists() into a try/catch and conditionally re-throwing the exception is a neat idea. I like it. :)

Remember in IRC when I asked you to kill _block_load_blocks()? This is why.

To clarify for the non-Tims of the world, _block_load_blocks() and block_load() are not in the same call chain. So, we have to add the same fix twice until the API is cleaned up in #1871696: Convert block instances to configuration entities to resolve architectural issues. :)

Now with proper documentation and some more thorough assertions.

Uh. Uploaded the wrong patch.

Attached fixes the exception so that it is still thrown when no module key is defined for the config object.

See: #1881630: [meta] Determine how to respond to invalid plugins (plugin dependencies)

What would folks think of using #20 for now to fix the fairly spectacular critical, and then exploring more general solutions and best practices in the related issues? Those being:

• #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API and friends
• #1871696: Convert block instances to configuration entities to resolve architectural issues
• #1881630: [meta] Determine how to respond to invalid plugins (plugin dependencies)

Criticals fixed is a good thing in my books.

To reiterate, the slightly-different exception thrown in _block_load_blocks() is a stopgap fix until we kill that function in #1871696: Convert block instances to configuration entities to resolve architectural issues. The patch actually fixes two bugs currently:

1. The first is that a block instance for a disabled module blows up the site.
2. The second is that a fatal error is thrown when rendering the admin page, even after the first issue is fixed, because the admin page uses the special flower that is _block_load_blocks() to list the block instances.

Essentially, the API has two functions that do almost-but-not-quite the same thing, and while both exist, we have to fix the bug by doing almost-but-not-quite the same thing as well. :) This is why we have two separate exceptions and two separate sets of test assertions.

+++ b/core/modules/block/block.module
@@ -361,7 +363,11 @@ function _block_rehash($theme = NULL) {
   $block_configs = config_get_storage_names_with_prefix('plugin.core.block.' . $theme);
...
   foreach ($block_configs as $config) {
-    $blocks[$config] = block_load($config);
+    // Only list valid block instances.
+    if (!$block = block_load($config)) {
+      continue;
+    }

In the old world of Block module, silently skipping over a block like that had pretty critical + destructive (data loss) consequences - are we sure that we're not running into those issues too here?

(The test only goes as far as disabling, but not to the full extent of editing (others) + re-enabling + verifying that things still work.)

+++ b/core/modules/block/block.module
@@ -493,9 +515,18 @@ function _block_load_blocks() {
+        throw new PluginException(sprintf('The plugin %s was not found.', $plugin_id));

I'm relatively sure that @catch will complain about the sprintf() instead of t() here — especially in .module files (contrary to decoupled classes in Drupal\Component), there's actually no reason to not use t().

In the old world of Block module, silently skipping over a block like that had pretty critical + destructive (data loss) consequences - are we sure that we're not running into those issues too here?

All _block_rehash() does is fetch a list of existing block instances and sorts them by theme for the admin page; see block_admin_display_prepare_blocks(). Skipping over them to not list them makes sense. The other place _block_rehash() is used is in hook_rebuild(), and we don't need disabled modules' block instances to be in the cached list since they are disabled.

I thought exceptions were not supposed to be translated? I guess we could use format_string(). It's going to go away in any case in #1871696: Convert block instances to configuration entities to resolve architectural issues.

I agree about adding more test coverage though; that makes sense.

How's this?

Oops, #28 is asserting the wrong block link; I've cancelled its test run.

Attached also submits the block ordering form while the test module is disabled for good measure. If there were any place data loss might occur, I imagine it'd be when submitting the block admin form without the disabled block listed, from a submit handler naïvely saying "delete anything not listed!" (That's not how block instances get deleted, though.)

And confirm that the block admin reordering is actually submitted successfully.

Rerolling to use #1872540: Provide a test helper method for creating block instances.

Added additional coverage to ensure that re-enabled modules' block instances go back to the correct regions.

Looks great to me. RTBC if bot comes back green.

I took my own stab at fixing this through implementation of hook_modules_en/disabled(). I'm using the exact same test that xjm posted in #32.

I'm very scared by the idea of nilly-willy renaming configuration objects. The first and foremost thing that will break is configuration staging.

True, configuration of disabled modules is staged as-is currently, without triggering the required API calls to import them. However, that only clarifies how broken disabled modules really are. We'll have to fix #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed for D8.

Until then, I'd rather go ahead with #32, since it fixes the problem within the scope of the system that has it.

Yeah, we'd at least have to add some magic for handling the disabled stuff during import and synch operations, which would mean making that prefix canonical in CMI, and I don't think we could commit something like #34 without CMI supporting it for that reason. E.g., with #34, if you had a disabled.plugin.core.block.bartik.myblock, CMI would happily allow you to create another plugin.core.block.bartik.myblock. Then when you re-enable the owner module for the first one, ta-da, data loss. Which brings us back to #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API.

OK, so understanding the concern here, how about this modification to xjm's patch? (tests are unaltered)

Eclipse

Interdiff

Oh. Yeah I am cool with that. Instead of working around _block_load_blocks() being special, we make it slightly less special. :) RTBC assuming it passes tests.

#37: 1881096-37.patch queued for re-testing.

Committed and pushed to 8.x. Thanks!

This fix is really messy so while I've marked sun's issue as duplicate I've bumped the original one to critical.

This fix is really messy so while I've marked sun's issue as duplicate I've bumped the original one to critical.

What issue do you mean?

#1871696: Convert block instances to configuration entities to resolve architectural issues should help. :(

@xjm: #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed

@catch's comment surely wasn't meant to discredit the good and very smart code of this patch. The focus definitely was on the insanity that we need to care for this and implant runtime conditional code for it in the first place, which is not caused by this issue/patch, nor by the new block plugins, nor by the plugin system, but instead, by the weird feature and possibility of disabled modules only.

Yes sorry that comment was a bit curt. It's messy that we have to worry about this at all, not this specific patch.