Move HelpSearch to Search sub-module

Going to give it a shot this weekend

Addressed the feedback.

I have concerns about one of the moved test functions. But I also left several other comments. Setting to Needs Work because of the test.

Thoughts?

Thank you for considering my feedback.

This looks good. Feedback was addressed. I believe the only open comments are self-review notes. Thank you for adding those. They helped during the review.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Updates were necessary due to #3569127: Add new 11.3.x database dump fixtures, without modules deprecated for removal in 12.x. They look good to me. Back to RTBC.

NW for merge conflict.

Makes sense because the update fixtures were updated in the last week or so. I checked the most recent changes. They seem appropriate to me. Tests are green. Back to RTBC.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

A few comments on the MR. It also has a merge conflict.

The applied suggestions looks good to me. The change to the fixture makes sense too since the module would not be installed in any core fixture.

Committed 317a647 and pushed to main. Thanks!

Reverted, because this was causing very slow runs in CI: https://git.drupalcode.org/project/drupal/-/pipelines/802911.
Credited @amateescu and @nicxvan for finding/investigating.

Never mind! It was #3496257: Race conditions in CacheCollector/State (again) causing the slowness in HelpTopicsSyntaxTest.

Committed bc0673e and pushed to main

Re-opening for a different problem:

core/modules/search/modules/search_help/tests/src/Kernal/HelpSearchPluginTest.php 

We need a quick follow-up to rename the directory. The namespace in the class itself is OK.

Hi everyone
Seems like this fix causes an error
Exception: Drupal\Component\Plugin\Exception\PluginNotFoundException: The "help_search" plugin does not exist. Valid plugin IDs for Drupal\search\SearchPluginManager are: node_search, user_search

I can reproduce it when installing dev-main locally and running `drush cr` or `drush updb` with my old database/config

Also, it reproduces in pipeline tests of my MR
https://git.drupalcode.org/issue/drupal-3309016/-/jobs/9569029
https://git.drupalcode.org/issue/drupal-3309016/-/jobs/9569030

Looks like you probably need to update your MR to include the install example https://git.drupalcode.org/project/drupal/-/commit/bc0673ecec60eba357d6b...

@smustgrave
1. you missed my first message - I upgraded my test env from 11.3 to the main branch and got an error, which is not good

2. This is not ok to add
__DIR__ . '/../../../../../system/tests/fixtures/update/install-search-help.php' where we don't need it.
3. According to the `node.info.yml` search_help module is not required.
modules/node/config/optional/search.page.node_search.yml also doesn't have a dependency on the search_help module.

Seems like an upgrade path is needed here.

I think this is an instance you’ll have to fix your local environment. This is only committed to main so eventually search and this module are going to be removed

This does needs an upgrade path as @hitchshock points out an existing site will run into this going from D11 to d12... we just hit by this in HEAD and I had to do https://git.drupalcode.org/project/drupal/-/commit/1f55640159dce9fb7c494...

I guess this is part of the plan to remove the core search module in favour of search_api. But I think atm we need a update in the help module to install this module so existing sites can continue working then when you deprecate search you deprecate this module too and tell everyone to uninstall it.

I think given this we should revert this change and introduce it without the update fixture.

Arghhh,,, we;ve had commits on top... forwards it is.

Fixing this is a critical bug having no upgrade path here completely breaks sites - you can't even install the search_help module because the table already exists!

Nice work. lgtm!

Now it works well! Thanks!
+1 RTBC

Committed/pushed to main, thanks!

Will need an 11.x backport MR.

Will need an 11.x backport MR.

Is the move to search_help not 12.x only?

I would have thought we'd backport it to 11.x so that we can then deprecate search + search_ help in 11.x and remove them in 12.x

Oh, OK. Then we need to back port the original commit bc0673ec as well as 8381d32 from #3586821: Fix typos in search_help module.

And does system_update_12002 need to be renumbered?

@godotislate we need to use https://www.drupal.org/node/3459876

Discussed with @catch changing the update to _11400 is fine because main is not production anywhere.

Opened 2 MRs - one against main to change the update number ASAP as per @catch's suggestion and antoher to backport everything to 11.x

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Bot is allergic to 11.x.

Actually there is an 11.x conflict.

The two MRs for main and 11.x are ready now.

I've compared the two branches and the search_help module is 100% the same and the differences in the help module are changes that have happened on main but not on 11.x and so are expected.

This approach seems fine to me.

Committed/pushed the follow-up to main and also the 11.x backport MR, thanks!