If your module defines block_info dynamically, so removes blocks from it, the block table still has this entries.

Files: 
CommentFileSizeAuthor
#12 1227966_12.patch10.98 KBBrockBoland
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1227966_12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#10 drupal8.block-delete.10.patch12.6 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,405 pass(es).
[ View ]
#9 block-delete-1227966-9.patch5.02 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 34,377 pass(es).
[ View ]
#6 block-delete-1227966-6.patch13.1 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 34,369 pass(es), 1 fail(s), and 0 exception(es).
[ View ]
#1 1227966-block_rehash-delete.patch1.73 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 33,576 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Comments

dawehner’s picture

StatusFileSize
new1.73 KB
FAILED: [[SimpleTest]]: [MySQL] 33,576 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

In general this patch is expected to fail at the moment.

So what could be part of the problem: $bids is empty, if you have all code blocks in the database as well.
Additional:

[09:16] <dereine> i'm still confused about the code, please enlight me: if there is a new block(not in the database) $primary_keys is array()
[09:16] <dereine> so it's not part of $bids
[09:17] <dereine> so if this db_delete is executed, it should be removed?

This seems to be an indicator that the db_delete is never executed

dawehner’s picture

Status:Active» Needs review

Update status

Status:Needs review» Needs work

The last submitted patch, 1227966-block_rehash-delete.patch, failed testing.

David_Rothstein’s picture

Title:block_rehash doesn't update block according to changes in block_info» Dynamically-defined blocks are never removed from the site, even when they disappear from hook_block_info()
Issue tags:+needs backport to D7

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

catch’s picture

Priority:Normal» Major

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.

David_Rothstein’s picture

Status:Needs work» Needs review
StatusFileSize
new13.1 KB
FAILED: [[SimpleTest]]: [MySQL] 34,369 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

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.

David_Rothstein’s picture

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

Status:Needs review» Needs work

The last submitted patch, block-delete-1227966-6.patch, failed testing.

David_Rothstein’s picture

Status:Needs work» Needs review
StatusFileSize
new5.02 KB
PASSED: [[SimpleTest]]: [MySQL] 34,377 pass(es).
[ View ]

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.

sun’s picture

StatusFileSize
new12.6 KB
PASSED: [[SimpleTest]]: [MySQL] 34,405 pass(es).
[ View ]

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.

catch’s picture

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

BrockBoland’s picture

StatusFileSize
new10.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1227966_12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

dawehner’s picture

I don't think we need this anymore with #1535868: Convert all blocks into plugins

kerasai’s picture

Issue tags:-needs backport to D7

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

Status:Needs review» Needs work
Issue tags:+needs backport to D7

The last submitted patch, 1227966_12.patch, failed testing.

Gábor Hojtsy’s picture

Version:8.x-dev» 7.x-dev
Issue tags:-needs backport to D7

Moving down to Drupal 7 based on #13.