Issue #1693336 by chx, David_Rothstein, catch | iamEAP: Block rehashing happens on every cron run.

CHANGELOG.txt

@@ -1,6 +1,11 @@
 
 Drupal 7.25, xxxx-xx-xx (development version)
 -----------------------
+- Performance improvement: Prevent block rehashing from writing blocks to the
+  database on every cache clear and cron run when the blocks have not changed.
+  This fix results in an extra 'saved' key which is added and set to TRUE for
+  each block returned by _block_rehash() that actually is saved to the database
+  (data structure change).
 - Added an optional 'skip on cron' parameter to hook_cron_queue_info() to allow
   queues to avoid being automatically processed on cron runs (API addition).
 - Fixed a bug which caused hook_block_view_MODULE_DELTA_alter() to never be

modules/block/block.module

@@ -401,23 +401,27 @@ function _block_rehash($theme = NULL) {
   }
   // Save the blocks defined in code for alter context.
   $code_blocks = $current_blocks;
-  $database_blocks = db_select('block', 'b')
+  $database_blocks = db_select('block', 'b', array('fetch' => PDO::FETCH_ASSOC))
     ->fields('b')
     ->condition($or)
     ->condition('theme', $theme)
     ->execute();
+  $original_database_blocks = array();
   foreach ($database_blocks as $block) {
-    // Preserve info which is not in the database.
-    $block->info = $current_blocks[$block->module][$block->delta]['info'];
+    $module = $block['module'];
+    $delta = $block['delta'];
+    $original_database_blocks[$module][$delta] = $block;
     // The cache mode can only by set from hook_block_info(), so that has
     // precedence over the database's value.
-    if (isset($current_blocks[$block->module][$block->delta]['cache'])) {
-      $block->cache = $current_blocks[$block->module][$block->delta]['cache'];
+    if (isset($current_blocks[$module][$delta]['cache'])) {
+      $block['cache'] = $current_blocks[$module][$delta]['cache'];
     }
+    // Preserve info which is not in the database.
+    $block['info'] = $current_blocks[$module][$delta]['info'];
     // Blocks stored in the database override the blocks defined in code.
-    $current_blocks[$block->module][$block->delta] = get_object_vars($block);
+    $current_blocks[$module][$delta] = $block;
     // Preserve this block.
-    $bids[$block->bid] = $block->bid;
+    $bids[$block['bid']] = $block['bid'];
   }
   drupal_alter('block_info', $current_blocks, $theme, $code_blocks);
   foreach ($current_blocks as $module => $module_blocks) {
@@ -456,7 +460,15 @@ function _block_rehash($theme = NULL) {
       else {
         $primary_keys = array();
       }
-      drupal_write_record('block', $block, $primary_keys);
+      // If the block is new or differs from the original database block, save
+      // it. To determine whether there was a change it is enough to examine
+      // the values for the keys in the original database record as that
+      // contained every database field.
+      if (!$primary_keys || array_diff_assoc($original_database_blocks[$module][$delta], $block)) {
+        drupal_write_record('block', $block, $primary_keys);
+        // Make it possible to test this.
+        $block['saved'] = TRUE;
+      }
       // Add to the list of blocks we return.
       $blocks[] = $block;
     }

modules/block/block.test

@@ -193,7 +193,7 @@ class BlockTestCase extends DrupalWebTestCase {
   }
 
   /**
-   * Test block visibility when using "pages" restriction but leaving 
+   * Test block visibility when using "pages" restriction but leaving
    * "pages" textarea empty
    */
   function testBlockVisibilityListedEmpty() {
@@ -899,3 +899,81 @@ class BlockInvalidRegionTestCase extends DrupalWebTestCase {
     $this->assertNoRaw($warning_message, 'Disabled block in the invalid region will not trigger the warning.');
   }
 }
+
+/**
+ * Tests that block rehashing works correctly.
+ */
+class BlockHashTestCase extends DrupalWebTestCase {
+  public static function getInfo() {
+    return array(
+      'name' => 'Block rehash',
+      'description' => 'Checks _block_rehash() functionality.',
+      'group' => 'Block',
+    );
+  }
+
+  function setUp() {
+    parent::setUp(array('block'));
+  }
+
+  /**
+   * Tests that block rehashing does not write to the database too often.
+   */
+  function testBlockRehash() {
+    // No hook_block_info_alter(), no save.
+    $this->doRehash();
+    module_enable(array('block_test'), FALSE);
+    // Save the new blocks, check that the new blocks exist by checking weight.
+    _block_rehash();
+    $this->assertWeight(0);
+    // Now hook_block_info_alter() exists but no blocks are saved on a second
+    // rehash.
+    $this->doRehash();
+    $this->assertWeight(0);
+    // Now hook_block_info_alter() exists and is changing one block which
+    // should be saved.
+    $GLOBALS['conf']['block_test_info_alter'] = 1;
+    $this->doRehash(TRUE);
+    $this->assertWeight(10000);
+    // Now hook_block_info_alter() exists but already changed the block's
+    // weight before, so it should not be saved again.
+    $this->doRehash();
+    $this->assertWeight(10000);
+  }
+
+  /**
+   * Performs a block rehash and checks several related assertions.
+   *
+   * @param $alter_active
+   *   Set to TRUE if the block_test module's hook_block_info_alter()
+   *   implementation is expected to make a change that results in an existing
+   *   block needing to be resaved to the database. Defaults to FALSE.
+   */
+  function doRehash($alter_active = FALSE) {
+    $saves = 0;
+    foreach (_block_rehash() as $block) {
+      $module = $block['module'];
+      $delta = $block['delta'];
+      if ($alter_active && $module == 'block_test' && $delta == 'test_html_id') {
+        $this->assertFalse(empty($block['saved']), "$module $delta saved");
+        $saves++;
+      }
+      else {
+        $this->assertTrue(empty($block['saved']), "$module $delta not saved");
+      }
+    }
+    $this->assertEqual($alter_active, $saves);
+  }
+
+  /**
+   * Asserts that the block_test module's block has a given weight.
+   *
+   * @param $weight
+   *   The expected weight.
+   */
+  function assertWeight($weight) {
+    $db_weight = db_query('SELECT weight FROM {block} WHERE module = :module AND delta = :delta', array(':module' => 'block_test', ':delta' => 'test_html_id'))->fetchField();
+    // By casting to string the assert fails on FALSE.
+    $this->assertIdentical((string) $db_weight, (string) $weight);
+  }
+}

modules/block/tests/block_test.module

@@ -56,3 +56,12 @@ function block_test_block_view_block_test_test_underscore_alter(&$data, $block)
 function block_test_block_view_block_test_test_hyphen_alter(&$data, $block) {
   $data['content'] = 'hook_block_view_MODULE_DELTA_alter';
 }
+
+/**
+ * Implements hook_block_info_alter().
+ */
+function block_test_block_info_alter(&$blocks) {
+  if (variable_get('block_test_info_alter')) {
+    $blocks['block_test']['test_html_id']['weight'] = 10000;
+  }
+}