Add search capability to help topics

The Search API project is not an OK plan for this, if we are going to put this in Core. We would instead need to use the generic Drupal core "Search" module.

So there are two possible strategies here:

a) Search -- add the text of topic pages to the Drupal Core search index, and then when someone does a help search, that index is used to find if there are matching pages.

b) Index -- each help topic would define its own entries for the help index (kind of like keywords). For instance, a topic about how to add fields to a content type might say that in the Help index, it wants to have entries for
Content type
Field
etc.

Then the Configurable Help module would build an index by collecting all the help topics' index entries and combining them alphabetically, and it would provide a page that would list the index entries, similar to the index you would find in the back of a programming book.

So... These two strategies are a bit different. With the Search strategy, each word in a topic would be indexed. With the Index strategy, topic authors would manually define what the important keywords were for indexing.

It seems to me that probably the search approach is probably best for this project.

The way the core Search API works in Drupal 8 is this... To tell the core Search module that you have some content to search, you define a config entity plugin of type SearchPage:
https://api.drupal.org/api/drupal/core%21modules%21search%21src%21Entity...

There's documentation on how to do D8 plugins here:
https://api.drupal.org/api/drupal/core%21core.api.php/group/plugin_api/8...
See the "Creating a plugin of an existing type" documentation. There are also two examples in Core. The most relevant is probably:
https://api.drupal.org/api/drupal/core%21modules%21node%21src%21Plugin%2...

You'll probably want to extend this class
https://api.drupal.org/api/drupal/core%21modules%21search%21src%21Plugin...

And you'll need to implement this interface, so that the help content can be indexed:
https://api.drupal.org/api/drupal/core%21modules%21search%21src%21Plugin...

That's actually basically it, except that it would probably be helpful to add a link somewhere on the Help page to the search page.

Let me know if you need more advice. I was heavily involved in setting up the D8 Search API, and of course I also wrote this module, so I can most likely answer any questions that come up. Thanks!

From discussions on #2920309: Add experimental module for Help Topics and #2592487: [meta] Help system overhaul, it is looking like what we really want is a search system. So, I'm changing the title here. Also it is higher priority than just "Normal".

Editing the issue summary also.

I'm going to take this on... Even though it doesn't need to be done before we can get #2920309: Add experimental module for Help Topics into Core. There's not much else waiting to be done. :)

I'm realizing that this should probably be a separate module:
- To use the core Search module's index/search capability (which seems like a good idea!), we would need to provide numeric IDs for each topic/page for the index. So, we'd need to have a database table to save a correspondence between a numeric ID and the config ID for the topic.
- It seems like we should make both the hook_help() module overviews and the configurable help topics searchable, plus (in principle) any other topics provided by other modules on the main Help page (admin/help).
- If we're doing that, we probably need either a set of hooks or a plugin or something that will allow each module to tell the search system about its topics.
- All of that just seems to say to put it in a separate module "Help Search" that depends on Search and Help, and then let each provider of help topics (including Config Help) implement the plugin/hook(s) so that if this help search module is enabled, searching will be there.

Anyway... I don't think I'm going to continue to work on this right now (seems better to wait until Config Help is in Core first), but I'm attaching the SearchPage plugin I started writing so I don't lose that work.

Moving this to Core queue as part of #3027054: Help Topics module roadmap: the path to beta and stable

I am not sure what you mean by a link index?

If you mark topic A as related to B, by using the 'related' meta-tag in the Twig file, then the link between A and B is automatically bi-directional. The Help Topics module takes care of that.

But the search module doesn't create an index of links.

I'm going to get back to working on this now that Help Topics are in Core.

Hi!

1. There is no component in the issue queue for help_topics, because we have plans to eventually merge help_topics into help when it gets stable. Check this issue for more information about the plans. #3027054: Help Topics module roadmap: the path to beta and stable
2. This issue is currently assigned to me and I plan to work on it next week. But yes, I will need to render the topic Twig templates and index the resulting HTML content. The patch here was from before. And no, it's not really related to that search result Twig template.

If you're looking for something to help with, please check #3027054: Help Topics module roadmap: the path to beta and stable for an issue that is not yet assigned to anyone. Thanks for your interest!

Here's a preliminary patch. It's totally untested, and not finished, but uploading here as a start. Things to do:
a) Write the findResults() method in the HelpSearch class (currently empty, see NodeSearch for example of how it should work).
b) Implement HelpSearchInterface in at least one HelpSection plugin (presumably the one in the Help Topics module).
c) Add configuration for a help search page, and put a search form at top of admin/help.
d) Does it need its own permission? I don't think so. There are already permissions for viewing help; presumably if you have this module enabled that is good enough to know you want people who can see help to be able to search it?
e) Test manually and get it working
f) Write a test for it
g) Write a help topic and update the hook_help for this new module based on how it actually works, once it's working. A rough idea is in the hook_help now but it may need revising.

A few notes on how it is supposed to work:

1. Modules that want to list their help on admin/help already create HelpSection plugins to tell the Help module about their topics.
2. Now they can also make that same plugin implement HelpSearchInterface. There are 3 methods to implement, which list and render topics.
3. If they do that and the Search, Help, and Help Search modules are installed, their help will be searchable. Site builders will need to create a "search page" (one will be provided by default) for searching help, and put a block on admin/help or wherever for searching help. (Or maybe this module will add this to the page already? that would be better.) Also they'll need to run cron to index stuff.

Thanks... I guess? This issue was assigned to me and I'm working on it.

The usual etiquette in such a case would be to wait at least a week to see if someone is still working on an issue, and then ask if you can take it over. In this case, I made the last patch 4 days ago and was planning to get back to it today. Now I don't know if you plan to take it to completion (you didn't assign it to yourself) or if I can get back to working on it. Please clarify! This is a high-priority issue and I have time to work on it today.

A few notes, looking at your interdiff -- which mostly looks good!

a) I am pretty sure we don't normally add @throws to doc blocks, unless that method explicitly throws an exception itself, which these ones don't. We don't document when a method is simply not handling exceptions that are thrown by things it calls. (Check examples in Core to confirm this, but I'm pretty sure.) If we did need a @throws, it would need an explanation as to why/when, I think? It's a pretty generic exception title. See
https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...

b) Converting simple queries to using select() is not really ideal. See
https://www.drupal.org/docs/8/api/database-api/static-queries
https://www.drupal.org/docs/8/api/database-api/dynamic-queries/introduct...
The queries in my patch that were ideal were copy/pasted nearly verbatim from
https://api.drupal.org/api/drupal/core%21modules%21node%21src%21Plugin%2...
so some of them should probably be restored.

c) You asked about config/install -- yes, that is correct!

Anyway, let me know if you're planning to take this to completion, and if so, assign it to yourself. Thanks!

I'm going to work on something else today... Anyway let me know if you plan to continue or if you want me to get back to this issue. Thanks!

No worries, and I definitely don't want to discard the patch (which was great work)! I was mildly annoyed (since I had planned to work on this today), but I'm happy to have someone so enthusiastic about contributing to this effort, so I'm over it. Feel free to take this on now -- I have plenty of other things to do. :)

Good explanation re dynamic queries. I personally prefer them anyway, it's just that I've heard we aren't supposed to use them unless we need them. But let's leave them in, and please go ahead and keep working on this issue. :)

Ah, I missed that in your last comment. If you're going to be away for a week, then I think I'll go ahead and make the next patch, since the other issue I was working on turned out to be pretty trivial. I am working on this today/now for sure, and then I'll unassign if I'm unavailable for a few days, and you can work on it again.

Sorry for being annoyed. It is never a great idea to get annoyed when people make good code contributions. :)

OK, here's the next patch!

Things I did:
a) Removed the @throws from the doc blocks (as discussed above). We definitely can't have inheritdoc + extra stuff in that one doc block (that doesn't work at all!), and I don't think we need them in the other doc blocks.

b) Changed one if() statement -- if we check that we implement the HelpSearch interface, then we don't also need to check that the methods exist (PHP will complain if you say you implement an interface but miss methods).

c) Implemented HelpSearchInterface for help topics, so they will get indexed. There are some @todo comments in HelpTopicSection about how to deal with languages... I am not sure how to do that.

d) Worked on the code until the indexing actually worked, and I could see that the help topics had their words in the search index.

Sorry, 2 interdiff files (in reviewing the patch, I found 1 more thing to change).

Still to do:

1. Write the findResults() method in the HelpSearch class (currently empty, see NodeSearch for example of how it should work). This will actually let the search part work.
2. Put a search form at top of admin/help.
3. Test searching manually and get it working
4. Write a test for it
5. Write a help topic and update the hook_help for this new module based on how it actually works, once it's working. A rough idea is in the hook_help now but it may need revising.

Wooot!

Here's a patch that gets search actually working! Setting this to Needs Review for the first time.

Still to do:
1. Fix the @todo items in the patch, mostly having to do with caching and translation.
2. Put a search form at top of admin/help (currently you have to go to search/help in order to search).
3. Write a test for it.
4. Write a help topic and update the hook_help for this new module based on how it actually works, once it's working. A rough idea is in the hook_help now but it may need revising.

Well, here's a problem... The URL for searching help is configurable (that is how the core Search module's Search Pages work). Also, the Search module only provides a block for using the default type of search that is configured on the site. So... We don't really have a way to set up a block with a search form in it for help, or to add a form that will search help from admin/search, because someone could go to the Search Settings page and edit the HelpSearch page that is provided in this patch to change the URL. Or even delete it.

I think the only way around this would be to make it so that the Search module provides a more configurable block, which allows you to specify which search page you want the search to go to. We'll probably need to spin this off to a different issue... unless we can think of another way to do it.

Anyway, updating the issue summary, and I'm probably done working on this for the moment.

The test errors seem to be:
- We forgot to add new module help_search to the core/composer.json file
- The dependencies might be incorrect, not sure about that? Maybe the composer fix will fix that other error
- There is something wrong with the search page config [although I can't figure out what?]
- 2 coding standards messages

So... let's try this...

Hm. That fixed one test fail, but the module install/uninstall test is still failing, as is the config test. I am going to try moving the config to the config/optional directory instead of config/install and see if that helps. Interdiff is difficult, but all I did was move that one file to a new location.

Oh, duh! We need a config schema file. Adding that, and I'll put the config item back where it was.

Hm, there was a test failure that looks unrelated. Hitting retest...

Here's the issue I created for the unrelated test fail, if anyone is interested (nothing to do with this patch):
#3067768: Random test fail in ManageGitIgnoreTest

So, I talked to @alexpott and @pwolanin (who is the Search module maintainer) in Slack today, and they said we should spin off a new issue to deal with the search block stuff. I created:
#3067943: Add capability for search blocks on non-default search

We can work on the To Dos here in the meantime, but we will not be able to complete this issue until that one is finished. I'll add it to the roadmap.

So, the @todo items in this patch. There are basically 3:

a) Cache tag stuff for the search page. Look at NodeSearch and do something similar.

b) Permissions. There are notes in that @todo on how to do that.

c) Language stuff when indexing and making snippets for search results... Thoughts:
- The titles of the HelpTopic topics are detected in HelpTopicDiscovery, and shoved into a TranslatableMarkup object (without the language being set).
- The body of HelpTopic topics is wrapped in {% trans %} in Twig (no language being set there).
- To render these into a desired language, it looks like we need to set the default language on the language.default service, and then all rendering/translation will happen in that language.

So, I think I'll work on (b) and (c) today. Also, I don't think we need keywords -- that could be a later enhancement -- so removing that from the issue summary.

OK, here's a new patch. I've taken care of two of the @todo items in the code (permission checking, and language stuff). One of them involved a change to HelpSearchInterface, as I decided it made more sense to render the item in the HelpSection plugin and not in HelpSearch. Now HelpSearch doesn't need the Renderer object.

The remaining @todo in the code (cache tags) -- I moved it around to 2 places and added some notes on how to do it. Decided to leave that to the next iteration.

Adding some notes about things to test to the issue summary.

Yeah, some of the To Dos will need to wait for #3067943: Add capability for search blocks on non-default search... interdiff looks good I think. I'm still not sure about the cache tags. Will need to think more on that.

Actually, I do have some thoughts about cache tags.

For Nodes (this may not apply to help topics), there are 2 relevant types of cache tags:

a) There's one that says "this is a list of nodes, so cache is invalid whenever any node is added, deleted, updated". That needs to be present on any search results page.

b) There's one that says "this page's content includes data from this particular node, so cache is invalid if this node changes in any way". That needs to be present on any search results page that includes that node.

The Search plugins have the capability to take care of both types of cache tags. For instance, the NodeSearch plugin does this in its constructor:

$this->addCacheTags([ 'node_list', ]);

(that's the type A cache tag), and then when building search results pages, the Search module adds the NodeSearch plugin as a cacheable dependency on the search results page.

And then NodeSearch, when building the snippet for the search results containing a given node, does this:

$this->addCacheableDependency(CacheableMetadata::createFromRenderArray($build));

So... It looks to me as though your patch adds the type A cache tags, but not in the same manner as was done on NodeSearch. And the type B ones are missing. I think it needs more work.

Well, presumably the caches are cleared when code is deployed, right? And we have set up the Help Search (or at least the Help Topic Search) to re-index when the cache is cleared. So... I think we are OK.

Setting to Needs Work for #45 (need to redo the cache tags stuff from previous patch).

I do think that a provider of help (as evidenced by having a section on admin/help and hence a HelpSection plugin) will need to declare that by implementing basically these methods. They would be just as useful for search_api contrib module as they would for the core Search module, although the return values might be used differently. But additional methods are definitely needed ... For instance, the list of topics to display on admin/help, in the case of Help Topics, is only the "top level" topics, but all topics should be searchable.

I do think it can be rearranged though. We could call it SearchableHelpInterface, put it in the Help module directly, and make it a bit more generic, such as not asking the plugins to prepare the search result, but instead to return just the information about the URL, title, cache tags, and text. Then the HelpSearch plugin could do the excerpt stuff (which uses the search_excerpt function).

Anyway, I'll work on this -- that might not make total sense in this comment but I have it in my head... I was going to make the next patch here anyway, and I have time today and the next few days.

I think that would be a possible future feature, but I don't think the base functionality requires it.

The base Search module (as implemented here) will automatically give more preference to higher headers, and some other HTML tags. So since we put the title in an H1, that will have highest priority, and headers will have next highest. It works pretty well. Right now we don't have any other relevant metadata, like keywords defined by the topic...

Before I get too far, I'm uploading some intermediate patches:
1. The patch from #43, rerolled so it applies cleanly.
2. An interdiff/patch from that that only moves some stuff around, so that we don't have a help_search module at all. I put all the help search stuff into help_topics for now, because adding it to the core Help module would be weird (since it doesn't itself have anything that is searchable).

Note that the interdiff is not all that useful or easy to see, because... well I'm not great at making interdiffs I guess. I made two patches and used the interdiff utility, but it didn't recognize that files were moved and instead said "you deleted this one and created this one". Anyway, I didn't really do anything new in this patch, just moved things around, combined the hook_help, and fixed namespaces.

Anyway. This will not work yet. I need to figure out the schema install stuff, plus the other To Dos. Right now the tables will not be installed.

OK!! Here's a new patch:
- I fixed up the cache stuff (I think?)
- I added the search block. I made it only visible on admin/help. Here is what it looks like:

- All the code is now in the help_search module (see previous patch/comment). Database tables are installed/uninstalled as needed when help_search and search are installed/uninstalled.
- I changed the name of the interface to SearchableHelpInterface, and made it more generic. Now it should be sufficient for both core Search and (hypothetically) Search API contrib module, or anything else that wants to index search. There are now only 2 methods: one to list the IDs, and the other to render a given ID into title, body, etc. (with no dependency on the Search module per se in the code there) (and less code duplication than in the old system).
- It works!!

We still need tests. Updating summary, which lists what tests are needed. But first lets see what I screwed up in the existing tests (hopefully not too much).

Another question (see summary): Do we want this search block visible on other admin pages besides the main admin/help page? I kind of think that is sufficient.

I don't think that is what we want to do. I put it into the Help region in the theme, with a weight that put it above the regular Help block. That region is all above the main page content. All the help sections would be in Main page content. So, I think it's correct as it is.

I'll take a look at fixing the test errors tomorrow, unless someone else wants to take the next steps. Basically, to get to Beta now on Help Topics, we just need to get this patch passing, and then write a lot of tests (on this issue and 2 others). I'll pick up this one or one of the other ones, doesn't matter too much to me. I should have some time to work on these issues for the next few days. Anyway, if someone else wants to take this one for a while, please assign it to yourself. Otherwise, I'll assign it to myself and work on it next.

I'm going to work on this today: fixing the test results problems (failed tests and 1 coding standards message), and adding tests.

First step: this should fix the test fails and coder errors, hopefully.

Since I'm not sure what you mean by comment #60, I think I will leave that to you. :) It is probably easier to communicate what you mean in a patch.

Meanwhile, here is a functional test that verifies:
- Search works in English and translated when searching for help topics from the test module
- Search works when searching for the fake help items from the test help section, English and translated.
- Help view permissions are enforced when searching
- Search block is installed automatically when you enable Seven theme, and works

As a note, I added a new HelpSection plugin to the help_topics_test module, for testing purposes. It returns information independent of the Twig/Topic plugin system, so it's simpler for testing. It may be useful for unit testing as well? It's pretty simple.

Updated the issue summary regarding things that are done/working/tested.

Anyway, I'll let you take a go at the service/test you are talking about in #60.

Whoever takes this next -- note there are 3 small coder problems on the last patch test results page. Other than that, tests pass.

One thing that I didn't test was the cache tags, so adding that to the summary. Also see #60.

I'm going to go ahead and do the coder fixes and hopefully the cache test too, if I can get it done in next 2 hours.

Here's a patch that adds a test for cache tags, and fixes the coder errors. Sorry, 2 interdiffs -- I made one and then thought of something else.

The test is failing locally to find the cache tag for the user who is searching. I'm not sure why that is, because in HelpSearch::findResults(), we are doing:

    $this->addCacheableDependency($this->account);
    if (!$this->account->hasPermission('access administration pages')) {
      return NULL;
    }

It seems like this should work... but maybe the AccountInterface object is not enough for addCacheableDependency?

Anyway. I'm out of time for today.

I'm going to work on the cache stuff, got some hints from @dpl.

Also I think we want to restrict the search by language -- node search does this. So if you are currently viewing the admin page you searched from in German, you should only see German results. I noticed when looking at some of the HTML debug output in the tests that there were duplicated links, and I am pretty sure that is why. Anyway, assigning back to me... should have a new patch shortly.

Here is my thought on caching (I'll add this to the issue summary too):
- We're really only concerned about when a search results page needs to be invalidated.
- The Search module already puts cache tags in based on the search index (which I have a test assert for already). So that means whenever the search index changes (i.e. for whatever reason we have decided to add, delete, or update an entry for a help item), the search results page cache is invalidated.
- Because of that, I don't think we need to worry about some of the caching stuff from the previous patch, such as making sure that each help section that might declare itself to be searchable has its plugin manager added to cache tags.
- We do also want to let each item in the results, as it renders itself, to declare cache stuff (which we're already doing in the previous patch).
- As per discussion with @dpl in slack, rather than adding a cache tag for the user who is searching, we should instead add a cache context for user.permissions.
- We should also restrict the search by the current language, and add a context for that. (languages:language_interface context)

As a note, I looked into changing the translation test base class in this patch into a trait, but I think it's better as an actual class that can be extended. This will allow the setUp() method to call parent::setUp(), which is a normal pattern, easily recognized... would be messier code to have a trait, alias that method, etc.

Anyway. Here's a new patch. The tests pass now, and I think the caching stuff is all good.

I ran into a problem when trying to get Configurable Help to work (I had this patch installed). In one place we are trying to call search_index_clear() passing in a list of IDs. That is incorrect. Patch/interdiff...

I am very happy to report that since I got #3072353: Update Configurable Help module so it works with Core Help Topics finished, with the above patch on #68, the configurable help topics also appear in search along with the Twig-provided topics. Woot!

So... I think this is about ready for RTBC. I am still not sure what was meant on #60, but as far as I can tell, that is the only To Do left for this patch.

Oh, we need a hook_help update and a new help topic too. Here is a patch for that, and updating issue summary for that being done. Here's a new patch and interdiff.

Yeah, I don't know what the right answer is. I thought we should not have that table present unless both the help_topics and search modules are installed. But maybe it is not so terrible to just use hook_schema, and know that it will be empty unless search is installed. Do we have other examples of this situation in Core?

I looked at other implementations of hook_modules_installed and hook_modules_uninstalled in Core and there isn't anyone doing schema installs. But what I don't know is if any modules are declaring schema that only applies of a certain other module is installed.

Regarding #72, it looks like all the Core examples that have defined an ensureTableExists() function [there should be a trait! They just all defined them separately] are in core/lib/Drupal/Core. Meaning they are outside the control of any module, so they had to take care of their own schema outside of hook_schema(), and only if their class/service/etc. was being used. And they are all doing that "on-demand" schema.

This situation is a bit different, because we do have hook_schema available.

Probably really help_search should be its own module... then we wouldn't have this trouble. The module would just depend on Search and Help and voila.

OK, we discussed this on Slack and decided the best alternative was probably just to use hook_schema() and live with an empty table in the case that the Search module is not enabled. I'm making a new patch that does that.

OK, here's the patch (that was easy). I verified that the functional test HelpTopicsSearchPatch at least still passes, so it should be fine.

It was put into the HelpTopicTranslationTestBase class, not removed. :)

Interdiff looks fine to me if test bot agrees. Thanks!

This is all looking good to me. +1 for RTBC. Thanks!

I added a change record for this; putting the release notes into the issue summary here too.

Ugh. Somehow I managed to delete a bunch of files from this issue. They are all still in the file system, but unlinked from the issue. So, I'm re-uploading the files I messed up. I think I must have had a stale form... sorry about that!

Actually I will do them in 2 comments so that the latest patch is alone for testing. Anyway here are the oldest ones first. These are all andypost's patches.

And here's the latest one, also from andypost. I'll try to cancel the other two tests that just launched from the previous comment.

I do not think much is a direct duplicate. Some code was certainly adapted, but the queries and results are different, plus rendering is different. I definitely didn't copy/paste very much without changing it...

What did you see that you thought we could maybe avoid duplicating? Because we could move stuff to a base class... but I don't think there is much if anything.

I've run into limits with how many placeholders a query can have in the past. This query uses

->condition('sid', $this_list, 'IN')

so each SID that is being deleted will have a placeholder in the query, because we're deleting specific item(s) from the table.

I didn't run into the placeholder limit in this case in my testing, but knowing I've run into it before, my general practice in situations where I'm doing an insert, update, or delete query that uses placeholders that grow as the size of what is being inserted/updated/deleted, is to chunk it. 100 could be overly cautious, but it should prevent problems down the line. I think in most circumstances, there will only be a few items deleted (e.g. because someone removed a module/theme that had a few topics that had been indexed and don't exist any more), so we won't run into the chunking very often.

Hm. Something has changed recently in rendering, and we're getting a strange test error about body not being a valid render key:

Drupal\Tests\help_topics\Functional\HelpTopicSearchTest::testHelpSearch
Exception: User error: "body" is an invalid render array key
Drupal\Core\Render\Element::children()() (Line: 97)

Going to hit retest just in case. If it still fails, I will investigate today or tomorrow.

Ah. What happened is that on #3072519: Help Topics discovery cannot be decorated easily we added some YAML-based topics and a TestHelpTopicPlugin class. That class's getBody() method should have been returning a render array, but was instead returning as string. So in the test for searching, we were trying to render/index these new topics from the test class, and they wouldn't render (which we never tested in the other patch that added these fake topics -- they were added to test *discovery* but we didn't actually look at the topic pages).

So. Here's a patch that fixes this method on the test class, so that the fake YAML-based topics can now be search indexed in the test. This is a pretty trivial interdiff (4-line change), and I don't think it really affects the earlier RTBC... but I'll let someone else (and the bot) review the interdiff and new patch.

Yeah, I usually prefer to put #type => 'markup' in. For one thing, on api.drupal.org, it links to the Markup element class. For another thing, for people new to the render element stuff in Drupal, I think it's clearer to always have a #type instead of having this exception.

So... Do you want me to remove the #type -> 'markup', or is this patch OK to get back to RTBC?

I just remembered that the other classes within core/modules/help_topics are listed as @internal for now. Adding that to the docs header of the new classes in this patch. See interdiff. (I copy/pasted the @internal notation from the other classes that already have it. They all have the same typo about "Help Topic" being internal. Oh well. At least they are consistent.)

Since anything under tests/ is automatically internal, I didn't add that notation there.

That fail was totally unrelated to this patch. Hitting retest. I'll check and see if I need to file a random test fail issue or if it's a rare glitch or already known or what.

This is the failure issue.

Thanks for the reviews and suggestions! Some of the items in the last 3 comments were more "observations" than suggestions for changes. So... Here are some notes... Responding to comment #105 first:

3 / 4 / 5 / 7 / 8 / 9 / 11 / 14 / 15 / 16 / 17 / 18 / 19 / 20 / 23 / 24 / 26 : Made suggested minor improvements to the code/comments. Hopefully changes are clear and what was desired.

6 [will be addressing that later]

10 : I am not sure about this. The code in question is:

  protected function prepareResults(StatementInterface $found) {
...
    foreach ($found as $item) {
      $result = $this->database->select('help_search_items', 'hsi')
        ->condition('sid', $item->sid)
        ->fields('hsi', ['plugin_type', 'id'])
        ->execute();
...

And the suggestion is to do this database query outside of the foreach. ...

So... The problem is that I would still (I think?) need to do some kind of a foreach on $found to extract the IDs from the StatementInterface object (it's not really an array), so that I could extract the plugin types and IDs in a single select. Then the code in the foreach would also be a bit harder to understand... So I would replace having 1 foreach loop and let's say about 10 queries, and fairly clear code, with having 2 foreach loops and 1 query with an IN condition in it, and I think harder-to-understand code. Is that really a lot more efficient? I'm not convinced it's worth it. I have left this as it is for now.

12 : Hmm... well the langcode gets to this function inside $item->langcode... anyway I changed the code so we are only getting the language once and saving it in an array, like the code was already doing for plugins around there.

13 / 22 : Yeah, both of these could be follow-ups. The search API has been stable in Core since about 8.0.

21 : I discussed this with pwolanin above in comments 90 and 91. I did replace the code with array_chunk though, and updated the comment to explain why I did it that way.

25 : I tried replacing this line in the test

    $this->drupalPostForm('admin/config/development/performance', [], 'Clear all caches');

with using $this->resetAll(). However, the test failed on this line because it got a 404 error visiting the page:

    $this->drupalPostForm('de/search/help', ['keys' => 'foomm'], 'Search');

So, I put it back the way it was, with a comment as to why it is necessary. This is not a crucial thing to debug and figure out the difference between going to that page vs. calling the resetAll() method, I think? If so, it's probably a follow-up? Suffice it to say the test does not pass (at least locally) unless it is done this way.

30. Well, maybe? I am way more familiar with xpath than with this "named" Behat thing. I did not make this change, because I couldn't quickly find documentation on what "named" does, or why putting "link" in there would work.

OK, back to item 6 from comment 105, and comments 106/107:
- I added the context pop thing to the cache thing.
- I changed the name of the key/variable as suggested in #106.

And one more thing from comment #106 -- regarding having one method for both indexing and results:
I started out with two methods, but there were very few differences between the two. I felt that rather than duplicating a *lot* of code, the most sensible thing to do was just to have a single method that HelpSection plugins wanting to make their items searchable would need to implement (see SearchableHelpInterface in this patch) in order to render an item.

You're right that we don't use the cache meta-data when indexing, so that is a bit of extra overhead during cron runs. But the language could be important for some types of help items that could be displayed in search results. For instance, look at what NodeSearch does when it is displaying search results -- you need the language code in order to render the item at that time.

I can split them up, but really I think it's a lot simpler this way. Let me know if you want to insist.

Anyway... Phew! Here's a new patch and interdiff. The tests still pass locally.

3 whitespace coding standards messages from that last test. Apparently coder wants the break statements indented the same as the case statements in a switch. ?!? Anyway. Whatever.

OK... so what do you think we need to do for this patch?

OK. Well regarding #110, the way the test is set up currently, it is working... I guess we could debug it further but it's not all that relevant for the test or this patch?

Regarding breaks, yeah I guess after a return or a throw we do not need a break. I will fix that. Here's a new patch. Hopefully Coder doesn't require break statements.

I don't agree with this change in TestHelpSection:

-          'text' => 'Fake foreign foomm text',
+          'text' => 'Fake foreign foo text',

We should have different text in English and German so we can verify the search indexing works correctly in both languages, and the correct text is stored in the index and searched for in both languages.

Then I also don't agree with this change:

-    $this->drupalPostForm('de/search/help', ['keys' => 'foomm'], 'Search');

+    $this->drupalPostForm('search/help', ['keys' => 'foo'], 'Search', [
+      'language' => $german,
+    ]);

We need to verify that the correct text is getting into the search index for the correct language. So we should be putting the word "foomm" in for German, and then testing that you can search for "foomm" in German. The test as it is currently written doesn't really verify that the translations are handled correctly in search indexing, because you are searching for "foo" and only verifying that the title on the search result page shows the German title.

I also don't think this is a good practice in the test:

     $this->drupalPostForm('search/help', ['keys' => 'ABC'], 'Search');
-    $session->addressMatches('|search/help|');
-    $this->assertLinkExistsOnce('ABC Help Test module');
+    $this->assertSearchResultsCount(2);
+    $this->assertLink('Writing good help');

The problem here is, we *currently* have a topic called "Writing good help". But this is a production topic, not a topic from a test module, so if that topic's title or content changes, the test will fail. We should only search for test topic text and test topic results. Hm... Actually this whole thing looks wrong? Why is "Writing good help" coming up in a search for ABC at all???

Probably using nonsense words as the search terms and the title/body text is the best way to make sure that we do not get real topics in the results.

So it seems like we need to verify:
a) When we search in English for a word that is in the English text, we get the correct English results.
b) When we search in English for a word that is in the German but not English text, we get no results.
c) When we search in English for a word that is in both English and German texts, we get only English results. It would be best to search for a word that is in one topic in English and in a different topic in German, and verify we only get the right topic, and it is in English.
d) Same as above a-c but with English/German reversed.
e) Searching works both with Help Topics and with the test fake Help Section plugin.

So... What we currently have for topics in the test module + current patch is:
1. A topic called "ABC Help Test Module" with body (partial) saying "Test translation", with fake German translation of these two strings as "ABC-Hilfetestmodul" and "Übersetzung testen.". This topic is in tests/modules/help_topics_test/help_topics, and the translation comes from HelpTopicTranslatedTestBase [we could add more translations to that test pretty easily].

2. Two other topics "Additional topic" and "Linked topic", plus the rest of the "ABC Help Test Module" that do not have translations for their strings. These are in the same directory.

3. A deriver class TestHelpTopicDeriver that creates a topic with title "Label for help_topics_derivatives" and body "Body for help_topics_derivatives". No translations.

4. A topic defined in the help_topics_test.help_topics.yml file with label "Test direct yaml topic label" and body "Test direct yaml body". No translations.

5. Two topics from the TestHelpSection with English titles "Foo in English title" and "Bar in English title", and German text that we're discussing what it should be in the previous two comments. Both the text and translations are provided in that class.

So.... I think we have enough topics and translations to test all of the above if we are careful, and we should avoid using real words as search terms that might pull in real topics. So, we might want to rewrite some of the topic text so it is full of enough fake words that we can do all the tests.

You are probably/hopefully asleep (??) so I will make a new patch in a few minutes...

I have to run out but here's a test that does all of that... it is not passing yet, and I am not sure why... I can look at it again tomorrow or you can andypost...

As far as I can tell, this is a real bug, either in the test or in the code. When I debugged this test, I could see that during the cron runs,although we asked for the DE translation of the test topic, the text from the Twig template was not actually translated using the PO file that is in the translation test.

Something is going wrong... I'm not sure what. The HelpTopicTranslationTest, which uses the same PO file and the same topic file, works -- you can see it in the browser output if you run interactively -- the test topic is definitely translated there. But not when we try to render it for searching. So, something is definitely wrong with either the test environment or the code that is rendering topics in different languages to send to the search index.

Meanwhile, here is an updated patch with a test that we need to get to work. I think this is now a very thorough test, but it isn't passing, so...

Obviously, I didn't get what you said in #106. Thanks for posting it again. :) I'll see what I can do...