Search reindexing should invalidate cache tags

If it was not for the 'Needs tests' part, I would RTBC this now :).

Interesting. Approach looks good to me. Looks like we can't use the entity cache tag, because that is not always available?

https://www.drupal.org/project/search_api will need a similar issue I think, and it only has views integration to display search results. So I guess the query backend will need to add a similar cache tag, probably for the search index and server.

Speaking of that, search.module has views integration too. We should probably make sure that the corresponding cache tags also exist when a view with those filters is used?

1. +++ b/core/modules/search/src/Plugin/SearchIndexingInterface.php
   @@ -46,6 +46,9 @@
   +   *
   +   * @return bool
   +   *   TRUE if any updates happened, FALSE otherwise.
   

   This is an API change.

2. +++ b/core/modules/search/src/Tests/SearchPageCacheTagsTest.php
   @@ -25,11 +26,33 @@ class SearchPageCacheTagsTest extends SearchTestBase {
   +    // Create a node and update the search index.
   +    $this->node = $this->drupalCreateNode(['title' => 'bike shed shop']);
   

   ROTFL :-D

3. +++ b/core/modules/search/src/Tests/SearchPageCacheTagsTest.php
   @@ -40,26 +63,42 @@ function testSearchText() {
   +    $this->node->title = 'bike shop';
   +    $this->node->save();
   

   Don't we need to run cron first to make this happen?

Leaving at Code needs review.

We need a beta eval and approval of the API / Interface change and a change record for the API change.

Opened #2463715: Properly integrate with Drupal Core's caching system for search API.

This patch may work, sort of, for NodeSearch, but it is not really quite right.

+++ b/core/modules/search/src/Controller/SearchController.php
@@ -123,7 +124,7 @@ public function view(Request $request, SearchPageInterface $entity) {
-        'tags' => $entity->getCacheTags(),
+        'tags' => Cache::mergeTags($entity->getCacheTags(), ['search_index:' . $plugin->getPluginId()]),
       ),

This is not necessarily correct. Although NodeSearch uses the plugin ID as $type, that is not necessarily what all other plugins will do. In particular, I am aware of one contrib module that, when ported to 8, will be using a different $type for each *instance* of the plugin, not one shared by this plugin type. So this line needs to change to ask the plugin for what $type it is using.

The other thing is that NodeSearch and potentially other plugins are also removing data from the search index in other places. You've got an invalidate in search_index_clear, but ..

Actually, wouldn't it be simpler to just put the invalidate in all of the functions in search.module that are adding or removing from the index? This would be a *lot* easier. These functions are:
search_index()
search_index_clear()

These are the only two ways that plugins using the index are supposed to be adding/removing data from the search index, so if you invalidate there, you'll be gold. Then you don't have to worry about changing the API for the search plugins.

I agree with #14, thanks for chiming in.

As one of the maintainers of the D8 search module, it's my job to chime in. ;)

As one other note, not all Search plugins even use the index. Take a look at UserSearch as one example that doesn't.

So in SearchController, you need to ask the active search plugin instance which $type, if any, it's using in the search index, and invalidate that. I suggest adding a method to SearchInterface to get this information, implementing it with a '' or NULL return in the base class SearchPluginBase, and then overriding this in NodeSearch to return 'node_search'.

If you'd like me to take this over and make a patch, let me know...

Hm. So, in the case of a Node search, the following things would change which nodes are displayed as search results on a given results page:
- Updates to any node -- this issue should catch these, at least after cron runs, because they'll result in node reindexing. Language, published/unpublished status, taxonomy terms, and author directly go into the query in some cases.
- Any update to a node that affects node access, or turning on/off a node access module.
- Updates to factors that can affect search rankings. In Core, these are (in addition to editing the node, which will eventually filter into an update to the search index): number of comments, sticky flag, promoted flag, and number of times the node has been viewed if the Statistics module is turned on.

And then the page output should also depend on the display of the specific nodes that are in the search results.

User search is much simpler. It just matches on user name (and email address if an admin is searching), and the query also filters out blocked users (unless an admin is searching). So basically if any user account is edited, all user search pages should probably be invalidated.

Is that being taken care of elsewhere?

+++ b/core/modules/search/src/Controller/SearchController.php
@@ -123,7 +124,7 @@ public function view(Request $request, SearchPageInterface $entity) {
-        'tags' => $entity->getCacheTags(),
+        'tags' => Cache::mergeTags($entity->getCacheTags(), ['search_index:' . $plugin->getPluginId()]),

$plugin->getType();

and should check for NULL :)

+++ b/core/modules/search/search.module
@@ -152,6 +153,11 @@ function search_index_clear($type = NULL, $sid = NULL, $langcode = NULL) {
+  if ($type !== NULL) {
+    // Invalidate all render cache items that contain data from this index.
+    Cache::invalidateTags(['search_index:' . $type]);
+  }

Is this the same $type as $plugin->getType()?

This confuses me ...

This all looks correct to me now. Great work!

A few minor things I think should be addressed:

1. +++ b/core/modules/search/search.module
   @@ -152,6 +153,11 @@ function search_index_clear($type = NULL, $sid = NULL, $langcode = NULL) {
   +  if ($type !== NULL) {
   

   This can just be if($type) I think?

   Also if $type is actually NULL, then I think we would need to invalidate *all* cached search pages with any $type?

2. +++ b/core/modules/search/src/Controller/SearchController.php
   @@ -127,6 +128,13 @@ public function view(Request $request, SearchPageInterface $entity) {
   +    if ($plugin->getType() !== NULL) {
   

   Again, why !== NULL, why not just if($plugin->getType()) ?

3. +++ b/core/modules/search/src/Plugin/SearchInterface.php
   @@ -65,6 +65,17 @@ public function getAttributes();
      /**
   +   * Returns the used search index type.
   +   *
   +   * @return string|null
   +   *   The plugin ID or other machine-readable type for the search index this
   +   *   search uses. NULL if no search index is used.
   

   Perhaps this is better as:

   Returns the search index type this plugin uses.

   and for the @return docs:

   The type used by this search plugin in the search index, or NULL if this plugin does not use the search index.

4. +++ b/core/modules/search/src/Plugin/SearchInterface.php
   @@ -65,6 +65,17 @@ public function getAttributes();
   +   * @see search_index_clear()
   +   */
   

   Probably better (or in addition) to have @see to search_index() rather than search_index_clear() ?

5. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -2762,4 +2762,15 @@ protected function assertCacheTag($expected_cache_tag) {
   +   * Asserts whether an expected cache tag was absent in the last response.
   +   *
   +   * @param string $expected_cache_tag
   +   *   The expected cache tag.
   +   */
   +  protected function assertNoCacheTag($expected_cache_tag) {
   +    $cache_tags = explode(' ', $this->drupalGetHeader('X-Drupal-Cache-Tags'));
   

   Saying something in a "No" assertion is "expected" is a bit odd. ;)

   How about making the first line:

   Asserts that a cache tag is absent from the current page.

   And then change $expected_cache_tag to just $cache_tag

   and in the docs for the param: The cache tag to check.

6. Maybe add a code comment in search_index() right where it calls search_index_clear() to note that this operation clears the cache tags?

Looks great to me now! Assuming test bot agrees, I think this is ready to go. Thanks!

Needs beta evaluation added to summary though...

Looks good, thanks!

Committed and pushed to 8.0.x.

I had a moment of doubt about this, but just verified it is working correctly. Phew!

What I wanted to verify was that if I added my own Node search page at admin/configure/search/pages, that the cache tags are correct. And they are: it still has search_index:node_search in there, plus a config item for the particular machine name of the search page config. User searches are also working well. So, all good!

But I'm wondering if we should put this in the tests too? The reason I am wondering about this is that the default config entity for NodeSearch has machine name 'node_search', which happens to match the plugin type ID for the NodeSearch plugin. So, my worry that made me take a look at this was that in place of using the plugin type ID string 'node_search' in the search_index cache tag, we could have been mistakenly using the individual config entity's machine name (which is the same)... Should we do something about this in the tests or not worry?

That sounds like a good division of labor to me! I'll also (of course) volunteer to review the patch. :) Filed:
#2465111: Add tests for caching on user-added Search page