Dynamically-defined blocks are never removed from the site, even when they disappear from hook_block_info()

Giving this a slightly more descriptive title.

This bug can be reproduced with a lot of modules that define dynamic blocks (examples include Views, Webform, and Media Gallery). If the view/webform/gallery goes away, the block still stays in the database and is rendered on the site if it returns something from hook_block_view(). But it doesn't appear at admin/structure/block so you can't disable it (although there seems to be a backdoor way to disable it by visiting admin/structure/block/manage/[MODULE]/[DELTA]/configure).

Here are some related issues:
#1273544: Media Gallery Block remains after deleting node
#1373360: Fatal error if optional Block module is not enabled
#1378354: Properly remove dependency of aggregator module on block module

Another way to come across this is to disable a module, then turn it back on again later. All the module's blocks will reappear on your site exactly where you left them. Actually in some ways that may be a feature (and at least for D7 backport we probably have to preserve that).

Info hooks--

Bumping this up to major since it's causing problems in contrib, we already found a bug in aggregator trying to make up for it, and it's leaving the db in an inconsistent state.

So, it turns out there are a lot of places in core that try to poke their nose into {block} and {block_role}, whereas if _block_rehash() actually worked correctly they could leave it to do its job on its own.

This patch isn't fully tested, but it seems to work, and I think it's on the right track.

It contains a fair amount of cleanup though - all of it made possible by fixing _block_rehash() - so it's possible it could be broken up into two patches if necessary.

Note that I incorporated the tests from #1 and expanded them a bit, although they could probably be expanded more (the {block_role} behavior is pretty tricky).

In some ways that test failure is expected. The Block module was basically treating its own dynamic blocks (a.k.a. custom blocks) differently than dynamic blocks provided by any other module, and then testing that special behavior...

That said, this is starting to get a bit more complicated than necessary, so I'm going to go ahead and split this into separate patches like I said above.

The attached patch only fixes deletion of the record in the {block} table, which is basically the major part of the bug. It does not fix deleting records in {block_role}, nor does it deal with any cleanup in other parts of the codebase that is possible as a result of this. Hopefully we can save that for a followup instead.

For now, let's see if this one passes tests.

Actually, #6 works fine; merely needed some minor tweaking in the custom block delete form submit handler.

Along with that, I've simplified the code for determining whether block_role records can be deleted. We can execute multiple count queries there, because a) blocks rarely need to be deleted and b) the count query is fast, as it hits an index.

+++ b/core/modules/block/block.moduleundefined
@@ -255,9 +255,11 @@ function block_block_save($delta = 0, $edit = array()) {
   $block = db_query('SELECT body, format FROM {block_custom} WHERE bid = :bid', array(':bid' => $delta))->fetchObject();
-  $data['subject'] = NULL;
-  $data['content'] = check_markup($block->body, $block->format, '', TRUE);
-  return $data;
+  if (isset($block->body)) {
+    $data['subject'] = NULL;
+    $data['content'] = check_markup($block->body, $block->format, '', TRUE);
+    return $data;

It's nitpicking but I'm not sure about this check. If the block exists, then $block->body should always be set even if it's an empty string no? So can we just do if ($block) there?

+++ b/core/modules/block/block.moduleundefined
@@ -482,31 +482,64 @@ function _block_rehash($theme = NULL) {
+    // Remove blocks that are no longer defined in code from the database.
+    // However, keep blocks belonging to a disabled module, to retain their
+    // settings in case the module is re-enabled.
+    // Blocks of uninstalled modules are deleted separately.
+    // @see block_modules_uninstalled()
+    $blocks_to_delete = db_select('block', 'b')
+      ->fields('b', array('bid', 'module', 'delta'))
       ->condition('bid', $bids, 'NOT IN')
+      ->condition('module', module_list(), 'IN')
       ->condition('theme', $theme)
-      ->execute();

This is going to delete blocks in the situation where a module conditionally defines a block based on the presence of another module that's currently disabled. I don't see a way out of this though.

Re-created this patch manually against the current head of 8.x. I made a few tweaks:

1. Removed some bits that weren't related (like removing an unnecessary return;)
2. block_flush_caches() no longer exists; replaced with block_rebuild
3. Dashboard module no longer in code; removed changes to that module
4. Replaced if (isset($block->body)) { with simply if ($block) { as requested by catch

5. This hasn't been tested at all yet, it's just an update to the previous patch.

#12: 1227966_12.patch queued for re-testing.

Moving down to Drupal 7 based on #13.