The basic pseudo code of _block_rehash is:

  1. Build a list of blocks defined in code.
  2. Retrieve any code-defined blocks that are also in the database and update the list with the info from the database. At the same time, build a list of existing database bid's.
  3. drupal_alter the list from the database.
  4. Process the list from the database, validating the region and status, writing updates and new blocks to the database. Remove each bid from the list of bid's.
  5. If there are any bid's left, delete all bids that aren't in the list.

This last step 5 makes absolutely no sense:

1) There will (almost, see below) never be any blocks in the bid list because every block that is defined in code in the database will be removed from the list, leaving nothing. The same block list that is used to build the list of bids is used to empty it back out. And that is a good thing, since the code does an awful thing if this weren't true.

2) Should a module implement hook_block_info_alter and remove a block from the list, this would leave that block's bid in the list of bid's. Then _block_rehash would execute this disastrous code:

  if ($bids) {
    // Remove disabled that are no longer defined by the code from the
    // database.
    db_delete('block')
      ->condition('bid', $bids, 'NOT IN')
      ->condition('theme', $theme)
      ->execute();
  }

This would delete every single block (for the given theme), other than the one that the alter function actually wanted deleted.

At a minimum, the above code should be deleted. Or better, the unset of the $bid list should be removed (EDIT: and new bid's added as they are written to the database) and the code changed to something that does what was intended:

  // Remove disabled that are no longer defined by the code from the
  // database.
  $q = db_delete('block');
  if ($bids) {
    ->condition('bid', $bids, 'NOT IN')
  }
  $q->condition('theme', $theme)
    ->execute();
}

I deliberated on the priority of this issue. The mere fact that the code doesn't work when blocks are no longer available in code is priority normal. But the latent loss of all the blocks on your site should a developer innocently try to work around the bug by implement the alter is horrendous, leading me to consider this major.

Comments

DanChadwick’s picture

Issue summary: View changes
donquixote’s picture

This seems to be a problem indeed.
I wonder why noone else has noticed it.

donquixote’s picture

I would say this cleanup should happen in a dedicated function outside of _block_rehash(), and across all themes.

db_delete('block')
  // db_not() does not exist, but..
  ->condition(db_not($or))
  ->execute();
donquixote’s picture

I had to manually delete facetapi block settings after disabling facetapi.

  db_delete('block')
    ->condition('module', 'facetapi')
    ->execute();

Version: 7.32 » 7.x-dev

Core issues are now filed against the dev versions where changes will be made. Document the specific release you are using in your issue comment. More information about choosing a version.