This is a follow-up bug report to #1535868: Convert all blocks into plugins. I mainly found this when working with the new pluggable block system in contrib.

Drupal\Component\Plugin\Exception\PluginException: The plugin (plugin.core.block.bartik.will_this_crash) did not specify an instance class. in Drupal\Component\Plugin\Factory\DefaultFactory->getPluginClass() (line 60 of /Applications/MAMP/htdocs/drupal-git/core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php).
The website has encountered an error. Please try again later.

Steps to Reproduce

  1. Enable something like Shortcut module.
  2. Add Shortcuts block somewhere.
  3. Disable Shortcut module.
    • Site is semi-functional, but mostly broken until Shortcut is re-enabled.

A user should be able to disable a module without having to remove block instances first (?).

Files: 
CommentFileSizeAuthor
#38 1881096-interdiff-do-not-test.patch1.07 KBEclipseGc
#37 1881096-37.patch7.32 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 50,820 pass(es). View
#34 1881096-34.patch6.3 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 50,618 pass(es). View
#32 block-1881096-32-tests.patch4.62 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 50,700 pass(es), 11 fail(s), and 1 exception(s). View
#32 block-1881096-32.patch7.76 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 50,744 pass(es). View
#32 interdiff.txt5.11 KBxjm
#30 block-1881096-30.patch6.83 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 50,540 pass(es). View
#30 interdiff.txt678 bytesxjm
#29 block-1881096-29.patch6.77 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 50,568 pass(es). View
#29 interdiff.txt1.29 KBxjm
#28 block-1881096-28.patch6.61 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#28 interdiff.txt3.13 KBxjm
#20 block-1881096-20.patch5.29 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 50,516 pass(es). View
#20 interdiff.txt1.08 KBxjm
#19 block-1881096-18.patch5.24 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 50,508 pass(es). View
#18 block-1881096-13.patch3.78 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#18 interdiff.txt3.62 KBxjm
#13 interdiff.txt559 bytesxjm
#13 block-1881096-13.patch3.78 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 50,510 pass(es). View
#11 block-1881096-tests.patch1.51 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 50,576 pass(es), 2 fail(s), and 0 exception(s). View
#11 block-1881096-combined.patch3.77 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 50,587 pass(es), 1 fail(s), and 0 exception(s). View
#3 1881096-module-disable-throws-exception-for-block-instances.patch1.47 KBmradcliffe
FAILED: [[SimpleTest]]: [MySQL] 50,594 pass(es), 1 fail(s), and 0 exception(s). View
#2 1881096-module-disable-throws-exception-for-block-instances.patch3.52 KBmradcliffe
FAILED: [[SimpleTest]]: [MySQL] 50,541 pass(es), 1 fail(s), and 0 exception(s). View

Comments

tim.plunkett’s picture

Component: plugin system » block.module
Issue tags: +Blocks-Layouts

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

mradcliffe’s picture

Status: Active » Needs review
FileSize
3.52 KB
FAILED: [[SimpleTest]]: [MySQL] 50,541 pass(es), 1 fail(s), and 0 exception(s). View

Thanks, tim. I think that's the appropriate action to take too.

Here's a throwaway patch with a quick test to demonstrate fail.

mradcliffe’s picture

FileSize
1.47 KB
FAILED: [[SimpleTest]]: [MySQL] 50,594 pass(es), 1 fail(s), and 0 exception(s). View

Oh crap, I forgot about that other change. Never work on dinner and core at the same time. :(

I always fail the first time when I get back to working on core.

xjm’s picture

#1 sounds sane to me.

sun’s picture

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.

xjm’s picture

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.

xjm’s picture

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.

sun’s picture

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

Status: Needs review » Needs work
xjm’s picture

Status: Needs work » Needs review
FileSize
3.77 KB
FAILED: [[SimpleTest]]: [MySQL] 50,587 pass(es), 1 fail(s), and 0 exception(s). View
1.51 KB
FAILED: [[SimpleTest]]: [MySQL] 50,576 pass(es), 2 fail(s), and 0 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, block-1881096-combined.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
3.78 KB
PASSED: [[SimpleTest]]: [MySQL] 50,510 pass(es). View
559 bytes

Duh.

tim.plunkett’s picture

+++ 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

xjm’s picture

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.

sun’s picture

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

xjm’s picture

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. :)

xjm’s picture

FileSize
3.62 KB
3.78 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Now with proper documentation and some more thorough assertions.

xjm’s picture

FileSize
5.24 KB
PASSED: [[SimpleTest]]: [MySQL] 50,508 pass(es). View

Uh. Uploaded the wrong patch.

xjm’s picture

FileSize
1.08 KB
5.29 KB
PASSED: [[SimpleTest]]: [MySQL] 50,516 pass(es). View

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

xjm’s picture

xjm’s picture

chx’s picture

Status: Needs review » Reviewed & tested by the community

Criticals fixed is a good thing in my books.

xjm’s picture

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.

sun’s picture

+++ 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().

xjm’s picture

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.

xjm’s picture

Assigned: Unassigned » xjm
Status: Reviewed & tested by the community » Needs review

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

xjm’s picture

Assigned: xjm » Unassigned
FileSize
3.13 KB
6.61 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

How's this?

xjm’s picture

FileSize
1.29 KB
6.77 KB
PASSED: [[SimpleTest]]: [MySQL] 50,568 pass(es). View

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.)

xjm’s picture

FileSize
678 bytes
6.83 KB
PASSED: [[SimpleTest]]: [MySQL] 50,540 pass(es). View

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

xjm’s picture

xjm’s picture

Assigned: xjm » Unassigned
FileSize
5.11 KB
7.76 KB
PASSED: [[SimpleTest]]: [MySQL] 50,744 pass(es). View
4.62 KB
FAILED: [[SimpleTest]]: [MySQL] 50,700 pass(es), 11 fail(s), and 1 exception(s). View

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

sun’s picture

Status: Needs review » Reviewed & tested by the community

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

EclipseGc’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
6.3 KB
PASSED: [[SimpleTest]]: [MySQL] 50,618 pass(es). View

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.

sun’s picture

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.

xjm’s picture

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.

EclipseGc’s picture

FileSize
7.32 KB
PASSED: [[SimpleTest]]: [MySQL] 50,820 pass(es). View

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

Eclipse

EclipseGc’s picture

Interdiff

xjm’s picture

Status: Needs review » Reviewed & tested by the community

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.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Configuration system, -Blocks-Layouts

The last submitted patch, 1881096-37.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Configuration system, +Blocks-Layouts

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

xjm’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

catch’s picture

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

xjm’s picture

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. :(

sun’s picture

@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.

catch’s picture

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

xjm’s picture

Issue tags: +Block plugins

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