Search results should bubble rendered entity cache tags and set list cache tags

There's one more thing too: if the Statistics module is enabled, and the node search ranking it provides is being used, then any time any node's statistics are changed (any time a node is viewed on its own page, I believe), the order of search results may change for any search, and therefore all node search results pages need to be invalidated.

Similarly, if Comment is enabled and "number of comments" ranking is being used, any time a comment is added or deleted, all node search results pages would need to be invalidated.

I do not think either of these would be included in the node_list tag?

I think this needs to be a child/blocker to #606840: Enable internal page cache by default also?

Anyway... It seems like the way to handle this might be to have a method on the Search plugin class that would ask each plugin "What cache tags do you need?".

Thus, NodeSearch could say "node_list", and also if the comment/statistics rankings are being used (which it would know as it depends on settings), also add some kind of a tag about that. And UserSearch could say "user_list".

Regarding the render pipeline... I'm not sure it's that easy to fix, but I'd definitely love it if we could! The rendering for search results output is really pretty lame. :( The problem is that the rendering of each NodeSearch result happens like this:
- Render the node with 'search_results' view mode
- Make an excerpt that highlights matching words
- Pass this and some meta-data to the Search controller, who themes it using the 'search_result' theme hook.

Rendering of user results is also done prematurely in the UserSearch plugin, but I just reviewed the code... It is not really rendering the User entity. It just assembles data for the 'search_result' theme hook, and in this plugin that data consists of a link title and link URL (user page), and the link title is either the user name or (if an admin is viewing) user name and email address. So it's not even using entity rendering, and probably does not need an entity rendering tag to be in there, I think?

That's the thing... Search result items need not be entities, which is why each plugin just returns information for a 'search_result' theme hook, rather than an entity to be rendered. I don't think we can/should change that. But definitely we should add render tags to that somehow.

Hope this rambling comment helps...

Oops I think this is a duplicate of #2265065: Second step in making search results pages cacheable: make sure each entity's search plugin bubbles up cache tags, not sure which one we want to keep?

Worked on this with swentel. This should bubble cache tags for NodeSearch and UserSearch.

Tagging

+++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
@@ -318,6 +319,10 @@ protected function prepareResults(StatementInterface $found) {
+      $nodeCacheTags = $node->getCacheTags();
+      $ownerCacheTags = $node->getOwner()->getCacheTags();

Talked with Wim, we should use #cachetags that come from the build array above

So... I think this is reasonable. But a few thoughts:

a)

+++ b/core/modules/search/src/Plugin/SearchPluginBase.php
@@ -98,13 +98,17 @@ public function getType() {
+      $built['cache_tags'] = array_merge($built['cache_tags'],$result['cache_tags']);

nitpick: space needed after ,

b)

+++ b/core/modules/search/src/Controller/SearchController.php
@@ -112,7 +117,7 @@ public function view(Request $request, SearchPageInterface $entity) {
-      '#items' => $results,
+      '#items' => $results['items'],
       '#empty' => array(

Possibly the fails are due to this? This has changed what is being passed to the theme function.

c) Also, don't we want the search results to have the node/user list cache tags too, or is that a separate issue still?

Also we don't need this issue to be related to itself. ;)

Fixed issues in #13 and #12. Test SearchConfigSettingsFormTest doesn't fail any more, let's see if this makes all test green.

Fixed the last test failure and added more information in the SearchInterface to better describe how the return array should look now.

So... A few concerns:

a) The patch as written is actually an API change, due to the change in SearchInterface::buildResults(). So, we will need to make a draft change record, update existing change records that mention SearchInterface, and also do a beta evaluation.

b) The docs update is weird... needs some work:

+++ b/core/modules/search/src/Plugin/SearchInterface.php
@@ -90,7 +90,8 @@ public function execute();
    *   An array of render arrays of search result items (generally each item
    *   has '#theme' set to 'search_result'), or an empty array if there are no
-   *   results.
+   *   results. The array should have a key 'items' with the actual results and
+   *   a key cache_tags that holds the cache tags for the entities in the result.
    */

This documentation kind of contradicts itself... It says it's an array of render arrays, but it's not really any more.

c) This patch still does not add the node or user list cache tags.

d) [Edit: forgot code tag here]

+++ b/core/modules/search/tests/modules/search_extra_type/src/Plugin/Search/SearchExtraTypeSearch.php
@@ -74,10 +74,10 @@ public function execute() {
-    $output['prefix']['#markup'] = '<h2>Test page text is here</h2> <ol class="search-results">';
+    $output['items']['prefix']['#markup'] = '<h2>Test page text is here</h2> <ol class="search-results">';
 

'prefix' really does not belong under 'items'. It is not an item. Same with 'suffix' below. I'm really not sure that this change to buildResults works very well, given this... Can we possibly do this another way that doesn't involve a change to buildResults() interface? This is not really right.

Regarding c.

+++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
@@ -336,6 +336,7 @@ protected function prepareResults(StatementInterface $found) {
+        'cache_tags' => $build['#cache']['tags']

The adding of the tags happens here, so I think we're fine.

Regarding d) Yes, it looks very weird, but the same hack is used in HEAD, it's a really weird and ugly way of adding prefix and suffix there :)

I updated the docs, so they look less weird now.

Well, to be exact both the 'node_views_data' table and the search index are queried in a node search. Depending on the settings for ranking and whether it's a simple or advanced search, the following tables may be queried:
node_views_data (status, langcode, uid, sticky, promote, created)
taxonomy_index (for advanced search)
comment_entity_statistics (for ranking based on # of comments)
node_counter (for ranking based on # of views)

Changes in any of these tables could cause the output to change, independent of whether any of the particular nodes displayed in the previous results have changed. Some of these changes (but not all) will cause the node whose information changed to need to be reindexed, so at the next cron run, the search index will have its cache invaliated and pages will not be cached any more, so that is probably fine. But not all of them will. So... I'm not really sure what to do here? Especially for the ranking based on # of views, which will not be picked up by any of the proposed cache tags/contexts.

As you said, the query is also tagged with node access, so if there are cache tags for that, we definitely need that too.

For User search, the search index isn't used at all -- the 'users' table is queried directly, so this is definitely a user list query and needs that cache tag.

I added support for cache contexts and implemented the cache context for node Search.

This fixes the failures for SearchConfigSettingsFormTest, let's see if it fixes failures for the other tests as well.

This is definitely getting better!

We still don't have the list cache tag for users... and I'm still not convinced we have the right cache tags for node?

A few smaller issues too:

1. +++ b/core/modules/search/src/Plugin/SearchInterface.php
   @@ -88,9 +88,13 @@ public function execute();
   +   *   An array that has 'items', 'cache_tags' and 'cache_contexts' arrays.
   +   *   The 'items' array is an array of render arrays of search result items
   +   *   (generally each items has '#theme' set to 'search_result'),
   +   *   or an empty array if there are no results.
   +   *   The 'cache_tags' array holds all the cache tags associated with the items
   +   *   in the 'items' array. The 'cache_contexts' array holds the contexts for
   +   *   all the the search page.
       */
   

   This needs to be re-wrapped, but I think the text is clear enough now. It would probably be better to write it as a list though, something like:

   An array of search results information, with the following elements:
   - items: An array of render arrays, each one representing a search result item...
   - cache_tags: ...
   - cache_contexts: ...

2. +++ b/core/modules/search/src/Plugin/SearchPluginBase.php
   @@ -98,13 +99,24 @@ public function getType() {
   +      $built['cache_tags'] = Cache::mergeTags($built['cache_tags'], $result['cache_tags']);
   +      if (isset($result['cache_contexts'])) {
   +        $built['cache_contexts'] = Cache::mergeContexts(
   

   The assymetry here is a bit worrisome. We haven't documented that each item must contain cache_tags and may contain cache_contexts. Perhaps we should (a) document that $result can contain those pieces, and (b) assume that either one could be empty?

   Also should we remove these pieces from $result after merging them into the overall cache contexts?

   And shouldn't the cache tags be properly put on each $result item too for render caching at a lower level?

I changed the documentation to reflect that cache contexts are optional and have rewritten it to be a list rather than the text we had previously. I'll ask Wim if we need to add extra cache tags or contexts.

Sure! Definitely we can do the rest in yet another issue. There is test coverage for advanced searching and for the search ranking stuff... but obviously not for caching in it.

Regarding cache tags for the list... wouldn't it make sense to add a mechanism for that to the $results array also? I mean you're already changing the API, might as well make a change that lets us do it right rather than kludging?

I fixed the issues from #30

OK, every time I review these patches, I get more confused...

So, I'm going through this systematically, rather than in the order of the patch:

a) We've now made it so that SearchInterface::buildResults() is returning an array with cache tags, cache context, and items. That looks good to me. We still need beta eval and change notice for this API change.

But the docs have a small formatting problem:

+   *  An array of search results information, with the following elements:
+   *   - items: An array of render arrays of search result items

The bullet items should have the - lined up under the text above. It's got an extra space of indent.

b) SearchPluginBase::buildResults() is looking through the output of the execute() method, and collecting cache tags from each item in the array. Minor formatting issue/question here:

+      $built['cache_tags'] = Cache::mergeTags($built['cache_tags'], $result['cache_tags']);
+      $built['cache_contexts'] = Cache::mergeContexts(
+        $built['cache_contexts'],
+        $result['cache_contexts']
+      );

I think the mergeContexts() call can be one line like mergeTags(), and I don't really think we normally split function calls into multiple lines anyway?

c) It seems like if a plugin wants to say there are cache tags/contexts that apply to the results as a whole, to be included in this merging, it should have a way to do that other than putting them on each item in the results. Can't we just add a method to SearchInterface to do that, and make the base class implement it as an empty array, and the UserSearch class can then implement it to put in the user list tag, and the Node class can use it for whatever it needs on the other issue (and would we maybe want to, on this issue, make the Node class say "node access is important for the group of results" too?)? We're already making an API change, and actually this one has *less* impact than the one that is already on this issue, because the base class will contain a base implementation of the new method.

d) SearchExtraTypeSearch overrides buildResults() and it doesn't add the cache_tags and cache_context array elements, which are supposedly required?

e) It looks like you've implicitly said that every single result item in the results for the execute() method on SearchInterface must also have cache_tags and cache_contexts defined? If so, we need to document that on the execute() method, and put that in the change notice as well.

f) I guess I am a bit confused about render array cache bubbling... I thought that if I put a cache tag or context on a particular render array element at a low level (such as the individual search result items), the rendering process would automatically bubble it up as needed. If that is true, why would buildResults() need to explicitly do this bubbling/merging up the render array for each result element -- shouldn't each one just set #cache and let the render pipeline handle the rest? Is there something I'm missing?

g) On the other hand, I think I do understand why SearchController would need to merge tags/contexts, because it has some potentially from the $results array (output of buildResults), and some that it needs to add, so that makes sense, although if the output of buildResults was a proper render array with #cache set up, and SearchController is putting that into #items on its outer render array, it seems like SearchController could just add its own cache tags at the top level and let the render pipeline merge in whatever is on the inner array from buildResults?

h)

+++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
@@ -336,6 +336,8 @@ protected function prepareResults(StatementInterface $found) {
         'score' => $item->calculated_score,
         'snippet' => search_excerpt($keys, $node->rendered, $item->langcode),
         'langcode' => $node->language()->getId(),
+        'cache_tags' => $build['#cache']['tags'],
+        'cache_contexts' => array('user.node_grants:view'),

Could the node plugin get the cache contexts from $build as well? If not, why not?

Disclaimer: I have not looked at the patch, but here are some comments:

#33:

c) What about having the plugins implement CacheableDependencyInterface and applying that to the render array / whole build as a last step:

  $renderer->addDependency($build, $plugin);

is all you need then.

f) The render pipeline should handle that - indeed - as long as it is rendered.

Given #33 and #34, I think this should have been set to Needs Work. If not, please explain. Thanks!

I added a beta evaluation to the IS, I think we're going to have to pick this issue up again before going into RC.

Marking tentatively as revisit before release. I think this can go in during RC phase as it should not block the RC.

I implemented what #33 and #34 suggested about the CacheableDependencyInterface and ::addDependency.

This wouldn't be a API change and wouldn't need change record updates or a change record.

Unit tests seem to pass, let's see the other tests pass as well.

Updated SearchPageCacheTagsTest.

The previous patch didn't have test coverage for a user cache tag on the node search. The current patch does have this.

"revisit before release" is a tag that is no longer used, and the "revisit before release candidate" issues have all been handled.

Given:

Disruptive for contributed and custom modules because it will require a BC break. Luckily, the API break will only be important for a site that has written a custom implementation of a search plugin. This is not something that is very frequently done I think.

I actually don't think this should go in during RC. Before RC would be better.

@xjm, this API change was introduced in the patch written by @swentel and I at drupal dev days (#32). The current patch however (#42) is not an API change but hasn't been reviewed yet.

Walked this through with @borisson_

+++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
@@ -353,6 +353,8 @@ protected function prepareResults(StatementInterface $found) {
+      $this->addCacheableDependency($node);
+      $this->addCacheableDependency($node->getOwner());

The call to add the owner as well seemed weird, but it makes sense as the user is not rendered in NodeSearch::prepareResults() on line 335 so no cache tags will be bubbled there.

Another fear we had was then that if an entity reference lives on the node and that reference is changed, this node wouldn't be flushed, but since line 335 does a render, it's all fine, so I don't think we need additional tests for that.

Since there's no API change anymore and we have additional tests, tentatively marking as RTBC!

Retest makes the patch green again, back to RTBC per #47.

The issue summary here is unfortunately lacking, but this patch does not add or test all the required cache tags. For one thing, the node_list tag is not added, and it needs to be. Also see comment #3 for additional tags.

I also think the patch is a bit suspect in how it is doing its coding. In SearchPluginBase, there is:

+        '#cache' => [
+          'tags' => $this->getCacheTags(),
+          'contexts' => $this->getCacheContexts(),
+          'max-age' => $this->getCacheMaxAge(),
+        ]

but it doesn't seem that either the NodeSearch or UserSearch plugin is implementing any of these methods to say what cache tags they need for the overall Results array, like the user_list or node_list... isn't that the right way to add these tags? I think it is, because where UserSearch is adding the user_list cache tag, it is only doing it if there are results to report, and a "no results" search result should also have that user_list cache tag. Right?

All of that should be tested... Definitely this is not ready to go in yet. Sorry...

There are now tests to check user_list and node_list. Not all of them are passing right now, the current failures are happening when $request->query->has('keys') is FALSE. The buildResults method is only called when that is TRUE.

So currently it's not very easy get the list cache tags from the plugin in the controller.

@Wim Leers helped me figure this out, we're now adding the default list cache tags in the plugin __constructmethod. All tests are green again.

This looks *** much *** better.

I do have a question about the tests though. It seems that the node tests are looking for:

+    $this->assertCacheTag('node:1');
+    $this->assertCacheTag('user:0');

Is user 0 really the author of that node, or are we just not testing to verify that the author of the node is cache tagged?

If user 0 is the author... maybe the test should be modified so that a real user account (not anonymous) is the author instead?

Not sure, so I just set this back to Needs Review instead of Needs Work. Feel free to override, but it looks a bit odd to me since the code is supposedly adding a tag for the node author.

Uses another user now, this is now user:2.

Excellent, thanks! Assuming the tests pass, this looks good to me now.

+++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
@@ -354,6 +356,14 @@ protected function prepareResults(StatementInterface $found) {
+

This is going to miss referenced entities too, seems very incomplete doing it like this.

Yeah, true... So I think that the User search patch here is correct. Node search is the problem, because in addition to picking off a few specific properties of the node and putting them into the result information array directly (like the time updated and the content type), it also renders the node with the "search results" view mode to use in making the excerpt.

So, let's see. The search_excerpt() function itself just takes $text as an input, so it shouldn't/doesn't know about cache tags. But NodeSearch::prepareResults() does render the node using the standard entity rendering pipeline before doing this:

      $rendered = $this->renderer->renderPlain($build);
      $rendered .= ' ' . $this->moduleHandler->invoke('comment', 'node_update_index', array($node, $item->langcode));
...
       'snippet' => search_excerpt($keys, $rendered, $item->langcode),

So couldn't it somehow take the cache tags that I assume would have been put into $build, plus some for the comments (which also are not part of this patch), and put those on its returned $result array?

I think that's probably our best option, at least until #2212323: Make a way to better control search result display.

#64:

Well spotted, the problem is this:

      $rendered = $this->renderer->renderPlain($build);

renderPlain means to render in isolation, which means you need to take care of cacheable metadata and assets yourself.

What you want to do afterwards is something like this:

$metadata = BubbleableMetadata::createFromRenderArray($build);
$metadata->applyTo($result);

// ...

$result['#cache']['tags'][] = 'node_list';

Why do we need to early render anyway and cannot just put a child item in $result at this point?

I wrote a test that proves this doesn't work yet.

RE #65, the reason we are rendering is that the result of rendering the node and its comments is passed through the search_excerpt() function to make an excerpt that highlights the search keywords, and that excerpt is saved as the "snippet" element on the returned search result information array.

We have other issues open to improve the process, such as:
#2212323: Make a way to better control search result display
#2552497: [PP-1] Convert search_excerpt() to a filter
etc. Search 8.x for search component -- it's only about a page of issues. Could definitely be improved.

Nice test in #66 by the way!

Just doing an early render skipping the Renderplain() might work.

This is kind of the reverse use case to Renderplain. We want the cacheability but ditch most of the rendered output.

RE #65, the other thing we need to get cache tags for is comments, which are added in a really stupid way to the rendered node before passing it all into the search_excerpt() function. And before you ask, the reason they are being added that way and not in some more intelligent way is that Search needs to override the default rendering of comments on the node, and it can't because comment rendering settings are not part of the formatter settings for the comment field (see #1920044: Move comment field settings that relate to rendering to formatter options about that...). Anyway. Long story, and I am not at all sure what to do about the comment cache tags.

Related issue: #2324719: Node indexing - should use view mode for comments, not hook regarding comments

Yes, lets try to just use render() instead of renderPlain() and have the RenderContext take care of the rest.

Um.... We do need to render the node and comments into plain text, so as to pass the result into search_excerpt(), which operates on plain text and does not know anything about cache tags (and shouldn't).

So the NodeSearch plugin does need to decouple "figure out what cache tags/context is needed" and put this onto its result (which contains the search excerpt as one component), and "render this stuff into plain text" (so that the excerpt can be calculated).

#73: render() will render to plain text / HTML so it works like renderPlain() - which was thought for emails and other isolated rendering that is _not_ shown on the page.

However render() will push all cache and attached data to the stack, which is then incorporated back into the array the next time something gets cached.

As I don't see the individual fragments cached here, this should work fine.

In the worst case its added back at the controller level - via the RenderContext.

So using render() here is correct and renderPlain() is not - unless you deal with BubbleableMetadata yourself.

Ah. Hm. OK, but the rendered node and/or comments are not actually put into the returned render array from this function. Does that matter?

This all looks pretty good to me, as long as the tests agree with that assessment!

So. We're still missing a bit. In NodeSearch::prepareResults(), I think we are now correctly accounting for this part:

     $build = $node_render->view($node, 'search_result', $item->langcode);
...
      $rendered = $this->renderer->renderPlain($build);

as well as the user name that is separately rendered.

But it's also doing:

      $rendered .= ' ' . $this->moduleHandler->invoke('comment', 'node_update_index', array($node, $item->langcode));

      $extra = $this->moduleHandler->invokeAll('node_search_result', array($node, $item->langcode));

And then $rendered is passed into the search_excerpt() function to produce the highlighted search keys excerpt, and $extra is added to the returned array directly for the theme system to possibly print out with the search result.

So we are not accounting for:

a) The cache tags arising from comment_node_update_index() -- this renders all the comments on the node, and the reason it isn't done with regular field rendering is that it wants to get *all* the comments, not just the ones that would normally be displayed on the node page, and ... separate issue anyway already filed.

b) The cache tags from hook_node_search_result(). Currently the only implementation in core is from the Comment module, which returns the formatted comment count. To get cache tags from this hook, we'd need to allow modules to return them with the hook. This is the only place the hook is invoked so it is a localized, not too disruptive change... so could be done without probably too much trouble.

Why do we not need to do anything about the comments? All of the comments connected to the node are rendered in that call to comment_node_update_index(), and I do not think their cache tags are being added. At a minimum, we should add to the test so that we're creating a node with a comment and verifying that the comment cache tag is there, right?

Glad to know about the comment count not being an issue.

Sorry for the confusion... There are two different hooks:

a) hook_node_update_index() - invoked specifically on just the comment module - this one renders all the comments and then the result is added to the $rendered stuff that gets passed into search_excerpt(). This is what I attempted to say in #78 (a).

b) hook_node_search_result() - called on all core/contrib implementing modules, which currently is just commentin core, and that is the one that calculates the comment count that we don't need to worry about. This is #78 (b).

Please wait a moment. There are reasons it is being done this way, although they are not "good" reasons, and again what I said has been misunderstood because I failed to provide enough details... sorry about that! So here's a more complete explanation, starting back at the beginning, and only talking about the "render all the comments" stuff and how it affects caching for the search results page.

1. At index time, there is this hook called hook_node_update_index() that allows modules to add stuff to nodes to add to the text that is indexed/searchable. We want comments to be searchable as part of nodes; so currently in Core, comment is the only implementer: it renders **all** the comments for the node. It would be better if we were using standard view mode / formatter rendering instead of a hook, and we have an issue for that [that is what I meant by "we have an issue"] -- #2324719: Node indexing - should use view mode for comments, not hook -- which we currently cannot fix because of #1920044: Move comment field settings that relate to rendering to formatter options. (The problem is that the comment rendering settings are still on the comment field instead of on the comment formatter, so Search view modes cannot have their own settings like "render every single comment and don't include links". So we're left with this lame hook, for the moment anyway.)

2. At search results time, NodeSearch needs to render a node with all the comments in order to generate the search excerpt. Again because of #2324719: Node indexing - should use view mode for comments, not hook and #1920044: Move comment field settings that relate to rendering to formatter options, we cannot use the standard comment field rendering on our search results view mode to achieve this, although that would be the "right" way to do it. So instead, we already know that this hook has been implemented for indexing that does exactly what we need for the search results. So as a shortcut (rather than defining another hook and making comment implement it, when it's not even the right thing to do because we should be using view modes), we invoke hook_node_update_index() just on the comment module, in order to render all of the comments. This is added to the rendered node, and the concatentation is what is passed into search_excerpt().

3. So I think that because (2) is what we are doing now, and because we cannot currently fix it, for now when we render these comments (SPECIFICALLY calling comment_node_update_index(), not invoking it on all modules), we need to make sure the cache tags for comments are added to the search result item's cache tags, which we are generating in this issue here.

Right?

And by the way, the search index cache tags are a separate question, which we actually already took care of on another issue:
#2460911: Search reindexing should invalidate cache tags
I am pretty sure we did the right thing there, which is basically "whenever we index/reindex a node, or remove one from the search index, any page depending on the node search index is now invalid". That is already in place, with tests.

Also... There is now *no* issue summary for this issue, only a Beta Evaluation... was that on purpose?

Also, hook_node_search_result() is for something completely different, and we already decided in a previous issue that we wouldn't worry about it [currently it is only giving the comment count].

3. So I think that because (2) is what we are doing now, and because we cannot currently fix it, for now when we render these comments (SPECIFICALLY calling comment_node_update_index(), not invoking it on all modules), we need to make sure the cache tags for comments are added to the search result item's cache tags, which we are generating in this issue here.

Right?

No, that is not necessary, Comment.php (irrelevant parts removed):

  /**
   * {@inheritdoc}
   */
  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
    // Always invalidate the cache tag for the commented entity.
    if ($commented_entity = $this->getCommentedEntity()) {
      Cache::invalidateTags($commented_entity->getCacheTagsToInvalidate());
    }
  }

Any time a comment is added or updated (I just noticed that we should also do that for deleting comments, but that's not this issue's problem), we already invalidate the commented entity's (e.g. the node's) cache tag. So, there is nothing that we need to do here as long as the node cache tag is added.

Aaaaaaah. Sorry, I missed that if it was in the previous discussion.

So do we have an issue about the comment deletes? You're right, not this issue's problem anyway.

Hah! I was just writing a summary when yours came in. Yours is fine. Thanks!!!

This seems like the best solution possible given the current architectural limitations. Committed 6b0840d and pushed to 8.0.x. Thanks!

diff --git a/core/modules/search/src/Controller/SearchController.php b/core/modules/search/src/Controller/SearchController.php
index a7dc051..f108bcd 100644
--- a/core/modules/search/src/Controller/SearchController.php
+++ b/core/modules/search/src/Controller/SearchController.php
@@ -51,6 +51,8 @@ class SearchController extends ControllerBase {
    *   The search page repository.
    * @param \Psr\Log\LoggerInterface $logger
    *   A logger instance.
+   * @param \Drupal\Core\Render\RendererInterface $renderer
+   *   The renderer.
    */
   public function __construct(SearchPageRepositoryInterface $search_page_repository, LoggerInterface $logger, RendererInterface $renderer) {
     $this->searchPageRepository = $search_page_repository;
diff --git a/core/modules/search/src/Plugin/SearchPluginBase.php b/core/modules/search/src/Plugin/SearchPluginBase.php
index 502533c..2f2cd14 100644
--- a/core/modules/search/src/Plugin/SearchPluginBase.php
+++ b/core/modules/search/src/Plugin/SearchPluginBase.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\search\Plugin;
 
-use Drupal\Core\Cache\Cache;
 use Drupal\Core\Cache\RefinableCacheableDependencyInterface;
 use Drupal\Core\Cache\RefinableCacheableDependencyTrait;
 use Drupal\Core\Form\FormStateInterface;

Minor fixes on commit.

+++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
@@ -333,6 +336,7 @@ protected function prepareResults(StatementInterface $found) {
       $extra = $this->moduleHandler->invokeAll('node_search_result', array($node, $item->langcode));

So the band-aid is great, but a question:

How would a contrib module add cacheability metadata to the indexed node?

Answering my own question:

One possibility is via:

$node->addCacheTags(['foo']);

As usually indexing is done via cron this should have not many side effects on the rest of the system.

However likely the prepareResults function should operate on a clone of the original node to explicitly allow this use case.

Though that is another issue ...

Just pointing out my thoughts.

RE #97 - There are no special node cache tags for the index -- cached data that depends on the index is invalidated based on generic node invalidation.

This issue is about making sure that the cache tags are right for the Results page, which now, finally, has tags for:
- the index [the index cache tag is invalidated when nodes change or are deleted/added, in the case of NodeSearch]
- node_list [because depending on what is being searched, there are node fields added directly to the query]
- node:N [for the nodes actually being displayed in the results page]
- user:N [for the authors of nodes being displayed]