Refactor search_reindex() into separate functions

The issue summary here is incorrect in a few respects.

a) The main problem with this function is just its name, which has been the same since Drupal 5.x I think... Its functionality has always been to clear the search index, and I agree that is name is bad. We could change that.

b)

search_reindex(null, null, $langcode)
// Clears the index for all languages

That is true and documented -- if the first two params are null the language code is ignored and I do not think this is a problem.

c)

search_reindex(null, $type, null)
// Passes $sid = null to SQL queries (I need to check what this actually results in)

Yeah that is a problem. The queries will not work. You cannot do this.

d)

search_reindex($sid, null, null)
// Passes $type = null to SQL queries (I need to check what this actually results in)

It is documented that if you include $sid you also need to include $type, so if someone is calling it this way they deserve what they get (which is that the query will not work I believe).

Anyway... until we agree on the problem I do not agree with the proposed resolution. We have traditionally had an API function for doing this. As long as search_index() is an API function, I believe we do need functions to clear the index. We can't just put this on the plugins.

So, let's step back here.

We have a little API for search plugins that use the search index to use, and hypothetically for makers of UI dashboards to use.

It consists of a few functions:

a) search_index($sid, $type, $text, $langcode), which indexes $text and stores it with (sid/type/langcode) in the tables. [to be called by search plugins]

b) search_mark_for_reindex($type, $sid), which changes the timestamp on all items (all languages) matching type/sid to the current time, to indicate that they need to be reindexed sometime soon. [to be called by hooks or entity save operations related to search plugins, such as when nodes are updated]

c) The poorly-named search_reindex($sid, $type, $langcode), which performs two unrelated functions:
1. Clear an item from the search index, or a group of items with the same language (in this case, sid/type must be provided and langcode is optional). [to be called by hooks or entity delete operations related to search plugins, such as when nodes are deleted]
2. Request that all existing plugins reindex their items from the search index, if this function is called with no arguments [this is called when you click the "reindex everything" button on the search settings page, and it is done by calling the plugins' resetIndex() methods, which is also kind of poorly named.]

I think we need all 4 of these pieces in the search API.

So what I think we should actually do is to break search_reindex up into two functions:

1. search_index_delete($sid, $type, $langcode = NULL) ==> this would delete an item from the index, and would require $sid and $type to be provided

2. search_index_reset() ==> this would invoke all of the plugin resetIndex() methods and have no arguments

Thoughts?

I just noticed #2211259: Move search_index() to ConfigurableSearchPluginBase and marked as a duplicate of this issue.

So, moving these API functions should be done as a group if at all. But moving them to ConfigurablePluginBase is not the right move -- they are only useful for index-using plugins, which is not the same as configurable plugins.

We could potentially make them into a Trait (see #2134513: [policy, no patch] Trait strategy) or make indexing a Service. Probably making it a Service provided by the search module would make more sense. Don't we have an issue for that already actually? I guess not.

There will be contrib plugins using the search index that are not connected in any way to NodeSearch. (I maintain one for 7, so assuming I port the module to 8, I know this is the case). So we absolutely do not want the API for plugins to call to be located on NodeSearch, and my plugin (and other contrib plugins) that use the index should not be using NodeSearch in any way.

So we need search_index() and the other functions in this API as outlined in #4. And we need these functions either to be in search.module or to be provided as a "search.index" service.

I think functions that are deleting items from the index should have "delete" in the name, not "reset". We may also need to rename methods on the plugin interface/classes.

So, that's a good thought about what should be in search.module vs. what should be in the plugin. Let's think about this. Here are some guiding principles:

a) "API functions" discussed here should either be functions located in search.module, or methods in a "search.index" service provided by the search module.

b) Indexing is definitely more complex than just a database query, and needs to be centralized in an API function for plugins to call.

c) Anything that code outside the search plugin (entity hooks, etc.) would need to call should be provided in an API function:
- Deleting an item of specified (type, sid, langcode) from the search index (some plugins may want to do it this way)
- Deleting all language items with the same (type, sid) from the search index (node delete needs to do this for all languages)
- Marking an item (type, sid, langcode) in the search index as "updated, needs reindexing" (needs to be done on entity update)
- Marking all language items with the same (type, sid) as "updated, needs reindexing (node wants to do it this way)
- Clearing out the entire search index (e.g., because it is corrupted; there should be a UI button for this on the search settings page and we have an issue about it)
- Marking the entire index for reindexing (there is an existing UI button for this on the search settings page)

d) For convenience, we may also want to provide API-level functions for plugins to call that would:
- Run a query that deletes all items of a given $type from the search index
- Run a query that marks all items of a given $type as "updated, needs reindexing"
Most likely, both of these should notify plugins that this is happening, in case they have internal bookkeeping that needs to happen as well.

e) The API functions should not assume any knowledge of what $type or $sid means. For instance:
- relation between $type and $plugin->id, $entity->type, etc.
- relation between $sid and $entity->id.
Plugins should control these meanings for the $type string they've decided to use, and they shouldn't try to do anything with another plugin's index data.

f) The API functions and plugins should both understand that the 'reindex' column in search.dataset has this specific meaning: if it is zero, the item is deemed "updated and indexed", and if it is non-zero, it represents the request time for when it was marked "needs reindexing".

I also am not sure what "reset" means in a function name -- that seems a bit ambiguous at first glance.

Given that... I think we should have:

search_index($sid, $type, $text, $langcode) [index text for ($type, $sid, $langcode)]

search_index_clear($type, $sid, $langcode) [delete an item from the index]
search_index_clear($type, $sid) ==> omit langcode, means "do all of this type/sid"
search_index_clear($type) ==> omit $sid, means "do all of this type" [presumably called from a plugin]
search_index_clear() ==> omit $type, means "do it all", via query [notify all plugins this is happening]

search_mark_for_reindex($type, $sid, $langcode) [mark an item as "updated, needs reindexing"]
search_mark_for_reindex($type, $sid) ==> omit langcode, means "do all of this type/sid"
search_mark_for_reindex($type) ==> omit $sid, means "do all of this type" [presumably called from a plugin]
search_mark_for_reindex() ==> omit $type, means "do it all", via query [notify all plugins this is happening]

In which case, we would:
- Add $langcode arg to search_mark_for_reindex(), and make it work with those combinations of args
- Remove existing search_reindex() completely and replace by search_index_clear() with those combinations of args -- and these would all be based on queries, not calling plugin methods
- Add the button to the Search settings page that would clear out the entire index

And I think we would also modify the SearchIndexingInterface. We currently have this method:

  /**
   * Takes action when the search index is going to be rebuilt.
   *
   * Modules that use updateIndex() should update their indexing bookkeeping so
   * that it starts from scratch the next time updateIndex() is called.
   */
  public function resetIndex();

This is kind of silly -- NodeSearch's implementation of this just does a query that search_mark_for_reindex(NULL) could be doing itself.

So I think we should remove this, and instead add these two functions:

  /**
   * Responds to the entire search index being cleared.
   *
   * When a request is made to delete all items from the search index, this
   * method will be called on all plugins, so that they can update their own
   * indexing bookkeeping.
   *
   * @see search_index_clear()
   */
  public function searchIndexClearAll();

  /**
   * Responds to the entire search index being marked for reindex.
   *
   * When a request is made to mark the entire search index for reindexing, this
   * method will be called on all plugins, so that they can update their own
   * indexing bookkeeping.
   *
   * @see search_mark_for_reindex()
   */
  public function searchMarkAllForReindex();

In the case of NodeSearch, both of those methods would be empty. However, some plugins (such as my Search by Page module) will need these methods, if they have separate bookkeeping for indexing, and they correspond to hooks that used to live in search.api.php in D7. For convenience, we can add empty methods on SearchPluginBase, or make a SearchIndexingPluginBase with them added.

Thoughts? Is this clear and rational enough?

RE #10... That is a good question.

I think that you are right, we need to have functions/methods that can be triggered from the UI that are plugin-aware, probably for both clearing and marking for reindex, as opposed to the API functions above, which are dumb queries.

So... How about this:

1. We keep the API functions search_index(), search_index_clear(), and search_mark_for_reindex() outlined in #9.

2. Add API functions called:
search_index_clear_plugin($plugin_id) [optional $plugin_id ==> omitting would do all plugins]
search_mark_plugin_for_reindex($plugin_id) [optional $plugin_id ==> omitting would do all plugins]

These would load the plugin(s) and call methods on the plugin. So I think we would need these 2 methods now on the plugin, in place of what I suggested in #9 and the existing resetIndex():

 /**
   * Clears the search index for this plugin.
   *
   * When a request is made to delete all items from the search index, or to
   * delete all items related to this plugin, this method will be called. It
   * should call search_index_clear($type) to remove indexed items from the
   * search database, and do anything else necessary for plugin-specific
   * indexing bookkeeping.
   *
   * @see search_index_clear()
   * @see search_index_clear_plugin()
   */
  public function searchIndexClear();

  /**
   * Marks the search index for reindexing for this plugin.
   *
   * When a request is made to mark all items from the search index for
   * reindexing, or to mark all items related to this plugin for reindexing,
   * this method will be called. It should call search_mark_for_reindex($type)
   * to mark the items in the search database, and do anything else necessary
   * for plugin-specific indexing bookkeeping.
   *
   * @see search_mark_for_reindex()
   * @see search_mark_plugin_for_reindex()
   */
  public function searchMarkForReindex();

We might also want to add a "reindex" and a "clear" action to the Actions for each Search Page in the UI.

How's that?

Actually, I have one other thought.

I think maybe we should not have search_index_clear(NULL) and search_mark_for_reindex(NULL). We should instead do it by active plugin type and let the plugins have control over it, with search_index_clear_plugin() and search_mark_plugin_for_reindex(). You could call those for a particular plugin (and the plugin could call search_index_clear($type) to do the database query), or for all plugins, but we wouldn't ever run a query that would just clear out the tables.

Thoughts?

Sigh, one more thought. We should probably do the "by plugin" stuff instead by search page entity.

OK, at this point these last few comments are a mess and I'm going to update the issue summary. See what you think?

I do not understand your objections in #15 at all.

As stated in the issue summary:

- search_index_clear($type) is meant to be called **from plugins***, and is a handy way for them to get the database query done that clears their particular type of data from the index, so that we don't have to have every plugin writing the query itself (it has to clear data from two tables and I think there is value in centralizing this in one API function for plugins to use, rather than making them all reproduce this functionality).

- search_index_clear_search_page($entity) is going to be calling the clear method on the plugin. It is basically a wrapper at the API function level for calling that method. We also need the API function that calls the method on *all* plugins, and I think this shortcut is also useful to have. Is that a problem?

I woke up this morning thinking we could just get rid of the *_page* functions that I proposed in the previous issue summariy. They were intended to be just helpers/wrappers that would call into the plugin methods. I think we should just drop them, and (as I think you've been trying to get through my thick head :) ), let the plugins be the primary (and only) API for keeping track of what $type corresponds to what plugin.

Updated the issue summary accordingly...

If we're going to do this, it should be pre-beta

@jhodgdon, are you sure? It isn't a public API change for anything other than search API. Generally only things that change data schemata or the critical APIs are beta deadline.

Edit: I'd suggest beta target, but not beta deadline. Unless you as component maintainers prefer not to change this after beta.

OK.

I think I'll take this one on, in any case.

Here is a patch. It does what the issue summary says, and also adds tests.

I think the API is considerably clearer. The function now called search_mark_for_reindex() marks nodes for reindexing [as it did before], and the function now called search_index_clear() clears items completely out of the index. Both of these use database queries entirely and do not involve plugins directly (so they're mostly to be called from plugins and entities and such).

Someone wanting to use plugins can use the plugin manager to find the plugin and call the new methods on the plugins.

The new tests pass locally... let's see if there's anything else that this breaks (shouldn't be) and then I'll write a change node, which should make things even clearer.

Let's actually upload a patch. :)

Looks like I didn't break anything...

I have one more revision to make on this though. I think that the "reindex" button on the Search settings page should ask each plugin to reindex its own stuff, rather than using the DB directly -- so it should call the newly renamed method on the plugin, as it was actually doing before. This will mean that something akin to Solr could implement "indexable" but really be using its own index database, and still be able to participate in the "reindex" action from the admin UI. Added some more tests for this too, and rewrote some of the UI text about reindexing.

Also decided the new method on the indexable plugin interface had a redundant name, so I renamed it.

I'm going to draft a change notice now too... or probably two separate ones actually, one for the search.module functions and one for the interface.

I took a look at change notices... three existing ones need updates; updated issue summary with notes.

Drafted new change notices:
https://www.drupal.org/node/2326575 for search_reindex() refactor/removal
https://www.drupal.org/node/2326581 for the interface changes

Also in drafting the 2nd change notice I again realized that a method name was kind of redundant, so I decided to change the name of the new index clear method on the interface. So here is one more patch.

Oh, and one of the tests needed a tweak for the UI changes in the previous patch.

Thanks for the reroll and excellent review!

I think I have addressed your comments:
- I added documentation to the two functions in search.module stating that they're either meant for use by plugins or for building a UI. We currently have a UI that calls the search_mark_for_reindex() function, and we have a long-standing feature request for a UI that would call search_index_clear() as well. So I don't want to exclude that possibility by saying those functions are only for plugins to call.
- I added a paragraph to the SearchIndexingInterface documentation to suggest that if you're making a plugin that uses its own settings or its own index, you should make that clear to users.
- Changed code comments to say "default" instead of "Core" index for consistency with the UI
- Added some explanations to the UI to explain what "default" means for settings and index. Hopefully.

Thoughts?

Reroll.

Further reroll for changes in URL stuff that has been in flux lately in Core.

reroll.

Good catch! Thanks for the review and new patch.

I believe that this should properly be classified as a bug, because the name of the search_reindex() function is just wrong, either that or a major task because it "needs to be done".... if not, it at least definitely falls into the "reduces fragility" section of the flow chart on https://www.drupal.org/contribute/core/beta-changes ... I've added a "beta" section to the issue summary.

I'm fine with this being marked as a bug.

1. function search_index($sid, $type, $text, $langcode) {
   

   vs

   function search_index_clear($type = NULL, $sid = NULL, $langcode = NULL) {
   

   It is unfortunate that the argument order is changing. I would support changing the order of search_index to match all the new methods ie. so it is like

   function search_index($type, $sid, $langcode, $text) {
   

   There are only 4 usages in core - 3 of which are tests. That way search_index(), search_index_clear(), search_mark_for_reindex() are all consistent. search_reindex() used to be sort of consistent with search_index()

2. +++ b/core/modules/search/search.module
   @@ -507,22 +503,41 @@ function search_index($sid, $type, $text, $langcode) {
   + * @param strong $langcode
   

   That langcode might be strong but it certainly is a string :)

That diffdiff is not very readable... Here's a proper interdiff file. The new patch looks good, and testbot agrees, so thanks! Good idea to take that one extra step and make all the API functions for Search consistent.

I updated one of the draft change notices for this new change: https://www.drupal.org/node/2326575

I also updated the issue summary to include this change, and I updated the instructions on which change notices need to be updated and how when this patch is committed.

+++ b/core/modules/search/search.module
@@ -121,42 +121,38 @@ function search_preprocess_block(&$variables) {
+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);
+      }
     }
   }

This looks weird why is it not:

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);
  }

  $query_index->execute();
  $query_dataset->execute();
}

I know you've got this documented and this was similar to the way the original function works but why?

RE #48: $sid is meaningless without $type -- each search plugin defines what $sid means within its own $type, so if $sid is provided, you need to provide $type. Similarly, $langcode is also meaningless without $type, and we wouldn't want someone to try and clear out all the index for just one language. That would screw up plugins like NodeSearch, which (as described by Ian in #49) assumes that all languages are going to be indexed or all unindexed for a particular node.

But... I disagree with the conclusion of #49 that we should remove $langcode from our API functions. The Node module is using the Search index/database in the way you describe in its NodeSearch plugin, but that doesn't mean that the all possible plugins would use it that way, or that the generic API functions we provide should assume that someone should never want to clear out just one language/ID combination rather than the entries for all languages.

So I think if we remove $langcode we are making assumptions... Besides which, having $langcode as an argument for all three functions makes them look more similar, and also costs us next to nothing. I don't think we should make assumptions -- we want the search indexing and its database to be as generic as possible -- and I think having consistency in the API is desirable.

Provisionally setting back to RTBC, in hopes...

Back to Alex...

This issue is a normal bug fix, the changes are disruptive to a small part of the search module and reduce fragility by making sense and being consistent. I feel that the win for DX and additional testing this patch adds is big enough to be allowed per https://www.drupal.org/core/beta-changes. Committed f5d180e and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

Thanks!!!!!

I went through and updated all the change notices as noted in the issue summary. Drafts were published and there were edits to three others.