Issue #2211241 by jhodgdon, ianthomas_uk: Refactor search_reindex() into separate functions

core/modules/node/src/Entity/Node.php

@@ -120,10 +120,10 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
   public static function preDelete(EntityStorageInterface $storage, array $entities) {
     parent::preDelete($storage, $entities);
 
-    // Assure that all nodes deleted are removed from the search index.
+    // Ensure that all nodes deleted are removed from the search index.
     if (\Drupal::moduleHandler()->moduleExists('search')) {
       foreach ($entities as $entity) {
-        search_reindex($entity->nid->value, 'node_search');
+        search_index_clear('node_search', $entity->nid->value);
       }
     }
   }

core/modules/node/src/Plugin/Search/NodeSearch.php

@@ -357,19 +357,27 @@ protected function indexNode(NodeInterface $node) {
         $text .= $t;
       }
 
-      // Update index.
-      search_index($node->id(), $this->getPluginId(), $text, $language->getId());
+      // Update index, using search index "type" equal to the plugin ID.
+      search_index($this->getPluginId(), $node->id(), $language->getId(), $text);
     }
   }
 
   /**
    * {@inheritdoc}
    */
-  public function resetIndex() {
-    $this->database->update('search_dataset')
-      ->fields(array('reindex' => REQUEST_TIME))
-      ->condition('type', $this->getPluginId())
-      ->execute();
+  public function indexClear() {
+    // All NodeSearch pages share a common search index "type" equal to
+    // the plugin ID.
+    search_index_clear($this->getPluginId());
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function markForReindex() {
+    // All NodeSearch pages share a common search index "type" equal to
+    // the plugin ID.
+    search_mark_for_reindex($this->getPluginId());
   }
 
   /**

core/modules/search/search.module

@@ -121,42 +121,38 @@ function search_preprocess_block(&$variables) {
 /**
  * Clears either a part of, or the entire search index.
  *
- * @param $sid
- *   (optional) The ID of the item to remove from the search index. If
- *   specified, $type must also be given. Omit both $sid and $type to clear
- *   the entire search index.
+ * This function is meant for use by search page plugins, or for building a
+ * user interface that lets users clear all or parts of the search index.
+ *
  * @param $type
- *   (optional) The plugin ID or other machine-readable type for the item to
- *   remove from the search index.
+ *   (optional) The plugin ID or other machine-readable type for the items to
+ *   remove from the search index. If omitted, $sid and $langcode are ignored
+ *   and the entire search index is cleared.
+ * @param $sid
+ *   (optional) The ID of the items to remove from the search index. If
+ *   omitted, all items matching $type are cleared, and $langcode is ignored.
  * @param $langcode
- *   (optional) Language code for the operation. If not provided, all
- *   index records for the $sid and $type will be deleted.
+ *   (optional) Language code of the item to remove from the search index. If
+ *   omitted, all items matching $sid and $type are cleared.
  */
-function search_reindex($sid = NULL, $type = NULL, $langcode = NULL) {
-  if ($type == NULL && $sid == NULL) {
-    /** @var $search_page_repository \Drupal\search\SearchPageRepositoryInterface */
-    $search_page_repository = \Drupal::service('search.search_page_repository');
-    foreach ($search_page_repository->getIndexableSearchPages() as $entity) {
-      $entity->getPlugin()->resetIndex();
+function search_index_clear($type = NULL, $sid = NULL, $langcode = NULL) {
+  $query_index = db_delete('search_index');
+  $query_dataset = db_delete('search_dataset');
+  if ($type) {
+    $query_index->condition('type', $type);
+    $query_dataset->condition('type', $type);
+    if ($sid) {
+      $query_index->condition('sid', $sid);
+      $query_dataset->condition('sid', $sid);
+      if ($langcode) {
+        $query_index->condition('langcode', $langcode);
+        $query_dataset->condition('langcode', $langcode);
+      }
     }
   }
-  else {
-    $query = db_delete('search_dataset')
-      ->condition('sid', $sid)
-      ->condition('type', $type);
-    if (!empty($langcode)) {
-      $query->condition('langcode', $langcode);
-    }
-    $query->execute();
 
-    $query = db_delete('search_index')
-      ->condition('sid', $sid)
-      ->condition('type', $type);
-    if (!empty($langcode)) {
-      $query->condition('langcode', $langcode);
-    }
-    $query->execute();
-  }
+  $query_index->execute();
+  $query_dataset->execute();
 }
 
 /**
@@ -372,19 +368,19 @@ function search_invoke_preprocess(&$text, $langcode = NULL) {
 /**
  * Updates the full-text search index for a particular item.
  *
- * @param $sid
- *   An ID number identifying this particular item (e.g., node ID).
  * @param $type
  *   The plugin ID or other machine-readable type of this item,
  *   which should be less than 64 bytes.
- * @param $text
- *   The content of this item. Must be a piece of HTML or plain text.
+ * @param $sid
+ *   An ID number identifying this particular item (e.g., node ID).
  * @param $langcode
  *   Language code for text being indexed.
+ * @param $text
+ *   The content of this item. Must be a piece of HTML or plain text.
  *
  * @ingroup search
  */
-function search_index($sid, $type, $text, $langcode) {
+function search_index($type, $sid, $langcode, $text) {
   $minimum_word_size = \Drupal::config('search.settings')->get('index.minimum_word_size');
 
   // Multipliers for scores of words inside certain HTML tags. The weights are
@@ -474,7 +470,7 @@ function search_index($sid, $type, $text, $langcode) {
     $tag = !$tag;
   }
 
-  search_reindex($sid, $type, $langcode);
+  search_index_clear($type, $sid, $langcode);
 
   // Insert cleaned up data into dataset
   db_insert('search_dataset')
@@ -507,22 +503,41 @@ function search_index($sid, $type, $text, $langcode) {
 }
 
 /**
- * Changes the timestamp on an indexed item to 'now' to force reindexing.
+ * Changes the timestamp on indexed items to 'now' to force reindexing.
  *
- * @param $type
- *   The plugin ID or other machine-readable type of this item.
- * @param $sid
- *   An ID number identifying this particular item (e.g., node ID).
+ * This function is meant for use by search page plugins, or for building a
+ * user interface that lets users mark all or parts of the search index for
+ * reindexing.
+ *
+ * @param string $type
+ *   (optional) The plugin ID or other machine-readable type of this item. If
+ *   omitted, the entire search index is marked for reindexing, and $sid and
+ *   $langcode are ignored.
+ * @param int $sid
+ *   (optional) An ID number identifying this particular item (e.g., node ID).
+ *   If omitted, everything matching $type is marked, and $langcode is ignored.
+ * @param string $langcode
+ *   (optional) The language code to clear. If omitted, everything matching
+ *   $type and $sid is marked.
  */
-function search_mark_for_reindex($type, $sid) {
-  db_update('search_dataset')
+function search_mark_for_reindex($type = NULL, $sid = NULL, $langcode = NULL) {
+  $query = db_update('search_dataset')
     ->fields(array('reindex' => REQUEST_TIME))
-    ->condition('type', $type)
-    ->condition('sid', $sid)
     // Only mark items that were not previously marked for reindex, so that
     // marked items maintain their priority by request time.
-    ->condition('reindex', 0)
-    ->execute();
+    ->condition('reindex', 0);
+
+  if ($type) {
+    $query->condition('type', $type);
+    if ($sid) {
+      $query->condition('sid', $sid);
+      if ($langcode) {
+        $query->condition('langcode', $langcode);
+      }
+    }
+  }
+
+  $query->execute();
 }
 
 /**

core/modules/search/src/Form/ReindexConfirm.php

@@ -34,7 +34,7 @@ public function getQuestion() {
    * Overrides \Drupal\Core\Form\ConfirmFormBase::getDescription().
    */
   public function getDescription() {
-    return $this->t('The search index is not cleared but systematically updated to reflect the new settings. Searching will continue to work but new content won\'t be indexed until all existing content has been re-indexed. This action cannot be undone.');
+    return $this->t("This will re-index content in the search indexes of all active search pages. Searching will continue to work, but new content won't be indexed until all existing content has been re-indexed. This action cannot be undone.");
   }
 
   /**
@@ -63,8 +63,12 @@ public function getCancelUrl() {
    */
   public function submitForm(array &$form, FormStateInterface $form_state) {
     if ($form['confirm']) {
-      search_reindex();
-      drupal_set_message($this->t('The index will be rebuilt.'));
+      // Ask each active search page to mark itself for re-index.
+      $search_page_repository = \Drupal::service('search.search_page_repository');
+      foreach ($search_page_repository->getIndexableSearchPages() as $entity) {
+        $entity->getPlugin()->markForReindex();
+      }
+      drupal_set_message($this->t('All search indexes will be rebuilt.'));
       $form_state->setRedirectUrl($this->getCancelUrl());
     }
   }

core/modules/search/src/Plugin/SearchIndexingInterface.php

@@ -8,12 +8,19 @@
 namespace Drupal\search\Plugin;
 
 /**
- * Defines an optional interface for SearchPlugin objects using the index.
+ * Defines an optional interface for SearchPlugin objects using an index.
  *
  * Plugins implementing this interface will have these methods invoked during
  * search_cron() and via the search module administration form. Plugins not
- * implementing this interface are assumed to use alternate mechanisms for
- * indexing the data used to provide search results.
+ * implementing this interface are assumed to be using their own methods for
+ * searching, not involving separate index tables.
+ *
+ * The user interface for managing search pages displays the indexing status for
+ * search pages implementing this interface. It also allows users to configure
+ * default settings for indexing, and refers to the "default search index". If
+ * your search page plugin uses its own indexing mechanism instead of the
+ * default search index, or overrides the default indexing settings, you should
+ * make this clear on the settings page or other documentation for your plugin.
  *
  * Multiple search pages can be created for each search plugin, so you will need
  * to choose whether these search pages should share an index (in which case
@@ -32,22 +39,40 @@ interface SearchIndexingInterface {
    * own indexing mechanism.
    *
    * When implementing this method, your module should index content items that
-   * were modified or added since the last run. PHP has a time limit
-   * for cron, though, so it is advisable to limit how many items you index
-   * per run using config('search.settings')->get('index.cron_limit'). Also,
-   * since the cron run could time out and abort in the middle of your run, you
-   * should update any needed internal bookkeeping on when items have last
-   * been indexed as you go rather than waiting to the end of indexing.
+   * were modified or added since the last run. There is a time limit for cron,
+   * so it is advisable to limit how many items you index per run using
+   * config('search.settings')->get('index.cron_limit') or with your own
+   * setting. And since the cron run could time out and abort in the middle of
+   * your run, you should update any needed internal bookkeeping on when items
+   * have last been indexed as you go rather than waiting to the end of
+   * indexing.
    */
   public function updateIndex();
 
   /**
-   * Takes action when the search index is going to be rebuilt.
+   * Clears the search index for this plugin.
+   *
+   * When a request is made to clear all items from the search index related to
+   * this plugin, this method will be called. If this plugin uses the default
+   * search index, this method can call search_index_clear($type) to remove
+   * indexed items from the search database.
+   *
+   * @see search_index_clear()
+   */
+  public function indexClear();
+
+  /**
+   * Marks the search index for reindexing for this plugin.
+   *
+   * When a request is made to mark all items from the search index related to
+   * this plugin for reindexing, this method will be called. If this plugin uses
+   * the default search index, this method can call
+   * search_mark_for_reindex($type) to mark the items in the search database for
+   * reindexing.
    *
-   * Modules that use updateIndex() should update their indexing bookkeeping so
-   * that it starts from scratch the next time updateIndex() is called.
+   * @see search_mark_for_reindex()
    */
-  public function resetIndex();
+  public function markForReindex();
 
   /**
    * Reports the status of indexing.

core/modules/search/src/SearchPageListBuilder.php

@@ -203,16 +203,16 @@ public function buildForm(array $form, FormStateInterface $form_state) {
       '#title' => $this->t('Number of items to index per cron run'),
       '#default_value' => $search_settings->get('index.cron_limit'),
       '#options' => $items,
-      '#description' => $this->t('The maximum number of items indexed in each pass of a <a href="@cron">cron maintenance task</a>. If necessary, reduce the number of items to prevent timeouts and memory errors while indexing.', array('@cron' => \Drupal::url('system.status'))),
+      '#description' => $this->t('The maximum number of items indexed in each pass of a <a href="@cron">cron maintenance task</a>. If necessary, reduce the number of items to prevent timeouts and memory errors while indexing. Some search page types may have their own setting for this.', array('@cron' => \Drupal::url('system.status'))),
     );
     // Indexing settings:
     $form['indexing_settings'] = array(
       '#type' => 'details',
-      '#title' => $this->t('Indexing settings'),
+      '#title' => $this->t('Default indexing settings'),
       '#open' => TRUE,
     );
     $form['indexing_settings']['info'] = array(
-      '#markup' => $this->t('<p><em>Changing the settings below will cause the site index to be rebuilt. The search index is not cleared but systematically updated to reflect the new settings. Searching will continue to work but new content won\'t be indexed until all existing content has been re-indexed.</em></p><p><em>The default settings should be appropriate for the majority of sites.</em></p>')
+      '#markup' => $this->t("<p>Search pages that use an index may use the default index provided by the Search module, or they may use a different indexing mechanism. These settings are for the default index. <em>Changing these settings will cause the default search index to be rebuilt to reflect the new settings. Searching will continue to work, based on the existing index, but new content won't be indexed until all existing content has been re-indexed.</em></p><p><em>The default settings should be appropriate for the majority of sites.</em></p>")
     );
     $form['indexing_settings']['minimum_word_size'] = array(
       '#type' => 'number',
@@ -329,12 +329,14 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
     parent::submitForm($form, $form_state);
 
     $search_settings = $this->configFactory->get('search.settings');
-    // If these settings change, the index needs to be rebuilt.
+    // If these settings change, the default index needs to be rebuilt.
     if (($search_settings->get('index.minimum_word_size') != $form_state->getValue('minimum_word_size')) || ($search_settings->get('index.overlap_cjk') != $form_state->getValue('overlap_cjk'))) {
       $search_settings->set('index.minimum_word_size', $form_state->getValue('minimum_word_size'));
       $search_settings->set('index.overlap_cjk', $form_state->getValue('overlap_cjk'));
-      drupal_set_message($this->t('The index will be rebuilt.'));
-      search_reindex();
+      // Specifically mark items in the default index for reindexing, since
+      // these settings are used in the search_index() function.
+      drupal_set_message($this->t('The default search index will be rebuilt.'));
+      search_mark_for_reindex();
     }
 
     $search_settings

core/modules/search/src/SearchPageRepositoryInterface.php

@@ -29,7 +29,7 @@ public function getActiveSearchPages();
   public function isSearchActive();
 
   /**
-   * Returns all indexable search page entities.
+   * Returns all active, indexable search page entities.
    *
    * @return \Drupal\search\SearchPageInterface[]
    *   An array of indexable search page entities.

core/modules/search/src/Tests/SearchConfigSettingsFormTest.php

@@ -71,7 +71,7 @@ function testSearchSettingsPage() {
     $this->drupalPostForm('admin/config/search/pages', array(), t('Re-index site'));
     $this->assertText(t('Are you sure you want to re-index the site'));
     $this->drupalPostForm('admin/config/search/pages/reindex', array(), t('Re-index site'));
-    $this->assertText(t('The index will be rebuilt'));
+    $this->assertText(t('All search indexes will be rebuilt'));
     $this->drupalGet('admin/config/search/pages');
     $this->assertText(t('There is 1 item left to index.'));
 

core/modules/search/src/Tests/SearchMatchTest.php

@@ -36,10 +36,10 @@ function _setup() {
     \Drupal::config('search.settings')->set('index.minimum_word_size', 3)->save();
 
     for ($i = 1; $i <= 7; ++$i) {
-      search_index($i, SEARCH_TYPE, $this->getText($i), LanguageInterface::LANGCODE_NOT_SPECIFIED);
+      search_index(SEARCH_TYPE, $i, LanguageInterface::LANGCODE_NOT_SPECIFIED, $this->getText($i));
     }
     for ($i = 1; $i <= 5; ++$i) {
-      search_index($i + 7, SEARCH_TYPE_2, $this->getText2($i), LanguageInterface::LANGCODE_NOT_SPECIFIED);
+      search_index(SEARCH_TYPE_2, $i + 7, LanguageInterface::LANGCODE_NOT_SPECIFIED, $this->getText2($i));
     }
     // No getText builder function for Japanese text; just a simple array.
     foreach (array(
@@ -47,7 +47,7 @@ function _setup() {
       14 => 'ドルーパルが大好きよ！',
       15 => 'コーヒーとケーキ',
     ) as $i => $jpn) {
-      search_index($i, SEARCH_TYPE_JPN, $jpn, LanguageInterface::LANGCODE_NOT_SPECIFIED);
+      search_index(SEARCH_TYPE_JPN, $i, LanguageInterface::LANGCODE_NOT_SPECIFIED, $jpn);
     }
     search_update_totals();
   }

core/modules/search/src/Tests/SearchMultilingualEntityTest.php

@@ -49,6 +49,7 @@ protected function setUp() {
 
     // Check indexing counts before adding any nodes.
     $this->assertIndexCounts(0, 0, 'before adding nodes');
+    $this->assertDatabaseCounts(0, 0, 'before adding nodes');
 
     // Add two new languages.
     ConfigurableLanguage::createFromLangcode('hu')->save();
@@ -115,6 +116,7 @@ protected function setUp() {
 
     // Verify that we have 8 nodes left to do.
     $this->assertIndexCounts(8, 8, 'before updating the search index');
+    $this->assertDatabaseCounts(0, 0, 'before updating the search index');
   }
 
   /**
@@ -134,6 +136,7 @@ function testMultilingualSearch() {
     // function manually is needed to finish the indexing process.
     search_update_totals();
     $this->assertIndexCounts(6, 8, 'after updating partially');
+    $this->assertDatabaseCounts(2, 0, 'after updating partially');
 
     // Now index the rest of the nodes.
     // Make sure index throttle is high enough, via the UI.
@@ -145,6 +148,15 @@ function testMultilingualSearch() {
     $this->plugin->updateIndex();
     search_update_totals();
     $this->assertIndexCounts(0, 8, 'after updating fully');
+    $this->assertDatabaseCounts(8, 0, 'after updating fully');
+
+    // Click the reindex button on the admin page, verify counts, and reindex.
+    $this->drupalPostForm('admin/config/search/pages', array(), t('Re-index site'));
+    $this->drupalPostForm(NULL, array(), t('Re-index site'));
+    $this->assertIndexCounts(8, 8, 'after reindex');
+    $this->assertDatabaseCounts(8, 0, 'after reindex');
+    $this->plugin->updateIndex();
+    search_update_totals();
 
     // Test search results.
 
@@ -184,7 +196,7 @@ function testMultilingualSearch() {
 
     // Mark one of the nodes for reindexing, using the API function, and
     // verify indexing status.
-    search_reindex($this->searchable_nodes[0]->id(), 'node_search');
+    search_mark_for_reindex('node_search', $this->searchable_nodes[0]->id());
     $this->assertIndexCounts(1, 8, 'after marking one node to reindex via API function');
 
     // Update the index and verify the totals again.
@@ -216,6 +228,37 @@ function testMultilingualSearch() {
       ->execute()
       ->fetchField();
     $this->assertEqual($result, $old, 'Reindex time was not updated if node was already marked');
+
+    // Add a bogus entry to the search index table using a different search
+    // type. This will not appear in the index status, because it is not
+    // managed by a plugin.
+    search_index('foo', $this->searchable_nodes[0]->id(), 'en', 'some text');
+    $this->assertIndexCounts(1, 8, 'after adding a different index item');
+
+    // Mark just this "foo" index for reindexing.
+    search_mark_for_reindex('foo');
+    $this->assertIndexCounts(1, 8, 'after reindexing the other search type');
+
+    // Mark everything for reindexing.
+    search_mark_for_reindex();
+    $this->assertIndexCounts(8, 8, 'after reindexing everything');
+
+    // Clear one item from the index, but with wrong language.
+    $this->assertDatabaseCounts(8, 1, 'before clear');
+    search_index_clear('node_search', $this->searchable_nodes[0]->id(), 'hu');
+    $this->assertDatabaseCounts(8, 1, 'after clear with wrong language');
+    // Clear using correct language.
+    search_index_clear('node_search', $this->searchable_nodes[0]->id(), 'en');
+    $this->assertDatabaseCounts(7, 1, 'after clear with right language');
+    // Don't specify language.
+    search_index_clear('node_search', $this->searchable_nodes[1]->id());
+    $this->assertDatabaseCounts(6, 1, 'unspecified language clear');
+    // Clear everything in 'foo'.
+    search_index_clear('foo');
+    $this->assertDatabaseCounts(6, 0, 'other index clear');
+    // Clear everything.
+    search_index_clear();
+    $this->assertDatabaseCounts(0, 0, 'complete clear');
   }
 
   /**
@@ -251,4 +294,34 @@ protected function assertIndexCounts($remaining, $total, $message) {
     $this->assertText($percent . '%', 'Correct percentage is shown on status report page at: ' . $message);
     $this->assertText('(' . $remaining . ' remaining)', 'Correct remaining value is shown on status report page at: ' . $message);
   }
+
+  /**
+   * Checks actual database counts of items in the search index.
+   *
+   * @param int $count_node
+   *   Count of node items to assert.
+   * @param int $count_foo
+   *   Count of "foo" items to assert.
+   * @param string $message
+   *   Message suffix to use.
+   */
+  protected function assertDatabaseCounts($count_node, $count_foo, $message) {
+    // Count number of distinct nodes by ID.
+    $results = db_select('search_dataset', 'i')
+      ->fields('i', array('sid'))
+      ->condition('type', 'node_search')
+      ->groupBy('sid')
+      ->execute()
+      ->fetchCol();
+    $this->assertEqual($count_node, count($results), 'Node count was ' . $count_node . ' for ' . $message);
+
+    // Count number of "foo" records.
+    $results = db_select('search_dataset', 'i')
+      ->fields('i', array('sid'))
+      ->condition('type', 'foo')
+      ->execute()
+      ->fetchCol();
+    $this->assertEqual($count_foo, count($results), 'Foo count was ' . $count_foo . ' for ' . $message);
+
+  }
 }