Help searches fail if site is not fully indexed, and users do not know why

That seems like a good solution!

Typo in the patch:

Reset last cron run ti allow new modules to execute scheduled tasks.

ti -> to

1. Install Drupal 8.8.-dev (HEAD at fd8dbd5675)
2. Enable Help Topics module
3. Navigate to admin/help
4. Search for 'basic' —> No results.
5. Checked cron. Cron recently ran (a few minutes ago)
6. Searched again. Still no results.
7. Checked admin/config/search/pages and 0% of the site has been indexed. (Content: 0 of 0 indexed. Help: 0 of 13 indexed. Users: Does not use index.)

Is there something else I should be checking? This looks like a failed test to me.

@webchick asked if we could just trigger a cron run from help_topics_install?

Is that possible?

Ok, applying the patch (in #4) does get us the result we want...help topics results immediately after enabling the module. So yay! We still need to fix the failing tests though. But this is great!

I will defer to @andypost on the question in #8.

The patch here looks more future-proof than my suggestion, since my understanding is this would work for any module. (I guess the disadvantage is it is doing it for any module, whether it implements hook_search() or not. But I guess we can't know what other "time sensitive" hooks exist in contrib/custom code, so this seems sensible.)

However, if this is at all tricky to get tests passing, the suggestion in #8 might be a workaround that fixes it enough for now, while we wait for a better fix to be perfected.

I think @webchick's suggestion is better, the fix here relies on automated_cron module, which is one of those modules (like dblog) that you shouldn't use in production. (You should setup a real cron solution instead - the module used to be called poorman's cron in contrib).

For those people, help topics won't work if they give it a spin, so a solution that is specific to help topics or generic (eg. part of system module) feels like a better solution.

Oh, fair enough then! :)

The problem with the suggestion in #8 is that you need to have both Search and Help Topics installed to get Help Search. So if Help Topics is installed first, the cron run will do nothing. Then Search is installed, you suddenly have Help Topics Search, and you need to run cron to get any results.

The problem we're trying to solve is showing help search results after someone installs help_topics. This will happen when cron is run after help_topics is installed. Running and scheduling cron and re-indexing search are fairly well-known administrative tasks but we should assume that not everyone will realize this. It's also been pointed out that while help_topics is likely to be enabled on production, automated_cron might be uninstalled on prod.

The urgent issue (for beta stability) is that we want a smooth user experience for the person installing help_topics for the first time. We want them to either see search results for help topics or understand what to do next if there are no results. So, what if we solve this 2 ways:

1. Continue with getting this automated_cron scheduling task to work when help_topics is installed (this issue). This will solve the problem for folks with automated_cron installed.
2. Add help text to the "no results". Currently it says: "Your search yielded no results." Change to: "Your search yielded no results. Expecting results? Run cron to re-build the search index." Or something like that. Would that solve this UX problem?

I am not sure putting a "Run cron" message in would be good -- that would be for all searches, as it does not come from the help search plugin but the core Search module. Sounds like not a great idea.

Oh, but!! If you look at HelpSearch::findResults(), it does this down at the bottom:

    if ($status & SearchQuery::EXPRESSIONS_IGNORED) {
      $this->messenger->addWarning($this->t('Your search used too many AND/OR expressions. Only the first @count terms were included in this search.', ['@count' => $this->searchSettings->get('and_or_limit')]));
    }

... more messages for other conditions.

It seems like we could put a check like that at the top of that method -- something like this:

    $total = $this->database->select('help_search_items', 'hsi')
      ->countQuery()
      ->execute()
      ->fetchField();
   if ($total < 1) {
... put out a message saying that help topics have not yet been indexed
  }

I don't think we should mention cron, because probably the person searching help will not have permission to run/configure cron. But we could link to the help_topics.help_topic_search topic (which tells you how to configure it properly, including running cron).

I'm not sure what you mean by "ask index about state"... but if you mean what I think you mean, then that will not help, because asking about the search index status runs a very similar count query (or multiple queries). This one would be lighter weight than calling the status method.

Circling back on this... A summary of the problem(s), as I see it anyway:

1. With core search functionality, if someone enables the Search module + Node [or in our case, Help Topics + Search], they might expect to see search results immediately. However, until they run cron, they won't see any search results. This problem predates Help Topics [by a decade or more].
2. It may take several cron runs to index all the Help Topics (or Nodes for that matter). While that is happening, you may get some results but not all expected results in a search.
3. The Standard profile enables Search, Node, and Automated Cron [and will include Help Topics eventually when it is stable]. This means that upon initial site install, cron will run immediately (on/before first page load), and it will continue to run every few hours by default. So the above is not a long-lasting problem -- if search results don't appear immediately, they will eventually without further action by an admin.
4. Some people may disable Automatic Cron (especially on production sites), but if they do so hopefully they will set up an outside cron process and again, any problems with search results not appearing will eventually vanish.
5. Some people may install Drupal without the Automated Cron module (if they don't use the Standard profile) and may not know they must set up outside cron. They would never get search results from Node or Help Topics and might not understand why.
6. Currently Help Topics are experimental, so if someone starts with Standard and installs Help Topics, they will immediately see the help search box, but it won't show results until cron runs, and possibly won't show all search results until after a few cron runs. This might give them a bad impression of the Experimental module. It will (as the other problems are) go away as soon as cron runs a few times.

So, what are the possible solutions to some/all of these problems:

• Run cron whenever any module is installed, whether or not Automated Cron is installed. This may not fully solve the problem, though, because it may take several cron runs to fully index for search.
• Reset the cron timestamp whenever any module is installed, for purposes of Automated Cron module, so it will run on the next page load (the previous patch on this issue). This only works at all if Automated Cron is installed, and it may not fully solve the problem, because it may take several cron runs to fully index for search.
• Check search index status after a search, or perhaps only if the search results are empty, and point the user towards the search status page that tells them the index status and how to fix it by running cron. (see comment 18). This could be expensive. On the other hand, the status could possibly be cached after the cron run so it doesn't have to be recalculated (at the risk of it not being totally accurate)?
• If our main concern is Help Topics, we could batch index those when the Help Topics and Search modules are first both installed, using hook_install() for the case that Search was enabled first, and hook_modules_installed() for the case that Search is enabled later.

I'm not sure which direction we should go on this, but I hope this list of problems and possible solutions is helpful. Thoughts?

Looking over what I wrote in the last comment again... I think I'm in favor of the last solution, where we would batch index help topics as soon as both Help Topics and Search are installed. That would be the least disruptive. If other modules providing search want to do something similar, they could.

I'm going to update the issue summary and put all of the above in it.

Better title to reflect problem rather than one proposed solution

OK, to get this going again... here's a new patch. It implements the idea that if modules/themes are installed, we should index any new help topics that have never been indexed.

This isn't quite the same as during cron, because that also indexes items marked as needing reindex. That's an important difference, because we decided whenever the caches were cleared, we would mark all topics as needing reindex -- that was done in case a module provided an updated topic (which we have no real way to detect) or languages were changed or translations were imported or any number of other possibilities.

Anyway, this way the new topics will at least be available, and over time their search indexes will be updated too (on cron runs as usual).

I expect the HelpSearch test to fail because of this patch -- it's testing at the beginning that nothing is indexed until a cron run. So it will need to be adjusted. I just didn't have time and I wanted to get this patch out there to look at... See what you think? It seems to work in my manual testing.

I already see that I need to look at some of the API docs in this patch, like this bit of HelpSearch, oops:

+  /**
+   * Counts the number of topics that have never been indexed.
+   *   The number of topics that exist that have not been indexed for search.
+   *   Does not include the topics that need updating.
+   */
+  public function unindexedCount() {

I'll look into it another day.

Fixing the nitpicks.

That's weird...

Fix another test break (this one I understand).

OK, this looks like it's ready for review, except that it needs to have a test for the intended behavior, which is not in this patch. Looks like the Needs tests tag is already on the issue.

Here's a new patch with tests added. I removed a couple of lines from the search test where it was testing that topics were not available until cron was run -- those lines would pass now, but it's only because the modules were installed without running the batch process that indexes on module install. Then I added some lines to test if you install a new module, its topics are immediately searchable. I also moved a few lines from one test method to another in the test. Hopefully the interdiff is clear enough. :) Test passes locally... let's see what the bot says.

Couldn't apply the patch in #33, so re-rolled it. It's attached.

And another reroll is needed.

Re-rolled the patch in #35. Kindly review.

Thanks for the rerolls! I just confirmed that after the 2 rerolls, the patch is the same except context. So, still needs review. Thanks!

I asked for a reroll, because I wanted to do a review. And I like to install a patch on my local machine when I review a patch to see how the added code fits in with all the other code. Just my thing.

1. +++ b/core/modules/help_topics/src/Plugin/Search/HelpSearch.php
   @@ -323,7 +347,9 @@ public function updateIndex() {
   +    if (count($items) < $limit &&
   +      (!isset($this->updateParameters['new_only']) ||
   +        !$this->updateParameters['new_only'])) {
   

   Nitpick: Can we do this on one line.

2. +++ b/core/modules/help_topics/src/Plugin/Search/HelpSearch.php
   @@ -300,6 +307,19 @@ protected function prepareResults(StatementInterface $found) {
   +  public function setUpdateParameters($limit, $new_only) {
   
   @@ -465,6 +491,23 @@ public function indexStatus() {
   +  public function unindexedCount() {
   

   These 2 methods need to be in a CR.

3. +++ b/core/modules/help_topics/help_topics.bulk.inc
   @@ -0,0 +1,66 @@
   +  $help_search->setUpdateParameters(5, TRUE);
   

   Could you explain why you chose to set the default limit to 5? I am not saying that it is wrong in any way. Just curious.

Thanks for the review!

First off, sorry for the misunderstanding. I totally agree the patch needed to be rerolled. I was trying to say in #38 that I have verified the rerolls were done correctly and resulted in the same patch (with updated context).

Regarding the review in #39:

1. Maybe? I actually prefer it for readability to be on two lines and we do not have a coding standard saying we shouldn't split things up into two lines. We do have a coding standard preferring lines < 80 characters in code:
https://www.drupal.org/docs/develop/standards/coding-standards#linelength
So... I think this is better as it is in this patch?

2. The guidelines for when you need a CR are here:
https://www.drupal.org/about/core/policies/core-change-policies/drupal-c...
Note that it says "...that module or theme developers need to know about, or that will need to be documented outside of the patch.". I don't think this fits that standard. This protected method and public functions were added so that they could be used in the function help_topics_prepare_indexing_batch(), which is why they needed to be public. I cannot see anyone else needing to use them, but if they do, they are documented in the function heading doc blocks.

3. That's a good question! I chose that somewhat arbitrarily, so that each batch wasn't very large and someone who installed a module could see the progress bar changing. 5 seemed "about right" to me and it got left in the patch when I got it working. Hm....

Probably we should either:

a) Change it to 1 item indexed per batch (in which case maybe the public function should be a Boolean to say "we're doing this interactively in a batch" rather than the "set the number of items to a specified value" as we have now).

or

b) Leave it at 5 by default but make it read from Settings so that someone could change it in settings.php if they wanted it to be something else or it was failing on their system.

or

c) Leave it at 5 and put in a code comment explaining why, because it should be at least explained in a comment.

Thoughts?

Here's a new patch that changes the number of items to index per batch run to 1 and adds a comment explaining why.

Regarding item 1 from the review in #39, I removed the need to have 3 lines in that if statement. I also simplified one other line by using the (relatively) new ?? operator.

Hopefully I didn't make any errors -- I'm not on my dev machine so I didn't test this.

Note that it says "...that module or theme developers need to know about, or that will need to be documented outside of the patch.". I don't think this fits that standard. This protected method and public functions were added so that they could be used in the function help_topics_prepare_indexing_batch(), which is why they needed to be public. I cannot see anyone else needing to use them, but if they do, they are documented in the function heading doc blocks.

Is it an idea to marks those methods as @internal, then there is no CR needed. See: #2873395: [meta] Add @internal to core classes, methods, properties.

The rest of the patch is for me RTBC.

I still think it is unnecessary, but in the interest of getting this done, I wrote up a CR. https://www.drupal.org/node/3193822

If the committers agree with me that it is unnecessary they can delete it. :)

The patch is for me RTBC.

We have a difference of opinion if the 2 new methods in this patch need a CR. The module is experimental and according to @jhodgdon the method are only for internal usage and therefore do not need a CR. I have a different opinion. For me a CR is needed or adding the tag @internal is needed. I leave the decision to the committer what the best solution is.

Running a batch after each module install seems overly intrusive to me. We search index on cron specifically so that it happens in the background. Somewhere there is an old issue about immediately indexing nodes when they're saved, and it's still stuck from years ago iirc.

#2 (resetting the cron timestamp) seems like a potentially good approach that could help other modules in similar situations.

#3 (checking for users with search admin permissions whether the index is 100% and inviting them to manually index) seems viable too.

Both of these could be implemented (in two different issues) I think and would complement each other.

OK, back to the drawing board. Thanks for the quick response! I'll delete the change record, since this approach is not happening.

I found that old issue catch was talking about. Also updating the issue summary... Here's my suggestion -- we should make a patch that:

a) Resets the cron timestamp whenever any module is installed. This will probably benefit other modules as well.
b) After each indexing run, save a State value that remembers the indexing status at the end of the run.
c) When searching, if the saved State value is not 100%, put a message on the screen saying that indexing is not at 100% and pointing to the Search status page (that page already tells you to run cron to finish indexing).

We can do this for all types of search that use indexing, not just help topics. In fact, I think we should do it for all types of search that use indexing -- they all need it. So... moving this into the Search module and re-titling. Also hiding the last patch since we will need to start over, and setting status back to Active because effectively, there is not a patch.

There's also #1189464: Add an 'instant' queue runner which if it ever happens could turn into non-blocking batches, but that's also very far off at the moment.

#47 sounds good but I think we should split A and B into two separate issues.

OK. Let's leave this for the part about "notify users if indexing isn't complete", since it fits with the issue title as it is now. And I've created child issue for the other part: #3194120: Need a way to say it's time to run cron and updating issue summary.

FYI -- I took the patch from @andypost on this issue and added it on #3194120: Need a way to say it's time to run cron. Andy if you see this, please comment on the other issue to hopefully get credited.

Here is a patch that:
- Updates a state variable after the search_cron run, storing the status of indexing for each plugin.
- Also updates the state variable when you visit the search admin page, because it was checking status anyway for each plugin.
- If you do a search that uses an index, and the state variable indicates it is not fully indexed, puts out a warning that the user can see that not all results might be shown.
- Tests that the warning is shown/not shown in appropriate situations (I added assertions into two existing tests).

This is a completely new approach and has no relationship to the previous patches (and no lines in common), so I did not make an interdiff file.

See what you think...

But it is also a problem if you search for something and get some results, but not the page you are really looking for. For instance, you might search for "views" and get several pages that mention views, but not the views overview topic because it (by chance) hasn't been indexed yet. The patch I put up there gives you a warning for any search against an index that was not complete as of the last cron run or the last time someone went to the admin page.

Maybe we should have a setting for what % done should trigger the warning, and pick a default value of something like if you are under 75% indexed, you get the warning. The setting could be per-page, so maybe Help would pick a higher % and node would go lower, and on some sites you could set it to 0% for node so you would never get the warning when searching for content.

Actually I think this is the way to go... I don't think we want to see that warning on node search for the most part, unless there is practically nothing indexed, but I do think on help search it makes more sense to see the warning.

Setting to needs work to add that to the patch.

I started working on adding the ability to configure the threshold for this warning being shown. But:

a) We can't put it on the SearchPage config entity, because not all types of search have indexes, so the config doesn't make sense for all SearchPage config entities. For example, user search has no search index, so having the config there would be confusing and irrelevant.

b) Search plugins are allowed to define their own configuration options that get saved on the config entities. But we cannot depend on a search plugin to do this, so there would not be a way for the search controller to know whether the index is complete or not.

c) Therefore, we would need to implement this on the search plugin.

d) I'm not actually sure we want warnings for Node search like this, although we almost certainly do for help search.

e) This is currently part of the Help Topics roadmap for stable.

So I think we should go back to only making this work for Help Topics, and leave Node search working as it was before with no warnings. Help search warnings would only be shown to administrator types anyway, so I think that is reasonable and I kind of don't think we want these warnings to be shown to regular site visitors (for node searches). (See also #52 and #53) ... Also updated issue summary to reflect this.

Here's a completely new patch that does it only for Help Topics. Interdiff is not useful since this patch has nothing in common with previous patches (again). However, this patch is smaller, simpler, and only affects Help Topics now. So, probably good.

I don't think we want it for other search plugins, or at least not quite the same and not in this issue.

We cannot do this for User search. User search does not use the search index -- it actually searches using a query directly on the user table, because you can only search for user names or email addresses (and email only if you have the permission of administer users I think, because others cannot see email addresses anyway). So it is always 100% "indexed".

For node search, I think you would need to set some kind of a threshold, like x% indexed, to see the error. The value to choose would (in my opinion) be very dependent on the site. Some sites would want to never show the error, because they have content updates/additions all the time and they don't want users to be confused or contact them about this weird error message... Really I think few sites would want a message like this shown to end users, or if so, only in dire situations where the site was not indexed hardly at all.

This is different than the situation for help search, where it's admins only. Admins need to know why their search failed, and even that it may not have returned all the results. Admins might have a chance of fixing the problem, or might know who to contact to see about fixing it. End users (node search) I think would just be confused.

Also, we don't want to mix patches for help topics with other modules. So... if we want to do it for Node we need to make a new issue.

+++ b/core/modules/help_topics/tests/src/Functional/HelpTopicSearchTest.php
@@ -73,9 +75,11 @@ protected function setUp(): void {
+    $this->assertSession()->pageTextNotContains('is not fully indexed');

Would this message ever be shown on /admin/config/search/pages? i.e. should we be visiting /search/help first to check whether this message is shown?

Doh! You are totally right, that is an error in the patch. Here's a new patch that searches and verifies the warning is not on the search page, which was the intention but not what the patch did.

I'll have to fire up my dev machine and see what's going on.

Doh! That test search does have a result, as can be seen by later in the test.

+++ b/core/modules/help_topics/src/Plugin/Search/HelpSearch.php
@@ -432,6 +438,24 @@ public function updateTopicList() {
+    if ($updateTopicList) {
+      $this->updateTopicList();
+    }

Do we need the extra parameter that triggers this? It's only used in one place - can't the caller just call this method first if they need to?

If we are to keep this, $updateTopicList should be $update_topic_list.

That's a good point. Updating patch...

Looks good to me now.

Random fail, see #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest

#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest was committed. Ordered retest.

+++ b/core/modules/help_topics/help_topics.module
@@ -82,3 +84,21 @@ function help_topics_themes_uninstalled(array $themes) {
+
+/**
+ * Implements hook_modules_installed().
+ */
+function help_topics_modules_installed(array $modules, $is_syncing) {
+  // Use the same code as module uninstall to ensure that index state is
+  // updated when a module is installed.
+  help_topics_modules_uninstalled([]);
+}
+
+/**
+ * Implements hook_themes_installed().
+ */
+function help_topics_themes_installed(array $themes) {
+  // Use the same code as module uninstall to ensure that index state is
+  // updated when a theme is installed.
+  help_topics_modules_uninstalled([]);
+}

The code in help_topics_modules_uninstalled() could be moved to a _helper_function(), and that would save having to explain what we're doing here. However I think that can be done in a follow-up.

Committed/pushed to 9.2.x, thanks!

Follow-up issue created: #3209139: Create helper function for help_topics_modules_installed()