Add capability for search blocks on non-default search

Interesting, checked how custom projects implement search block (most using search api)...
80% just configure route to submit search to
Makes sense contrib modules too

From BC POV block could add default configuration pointing to default search and allow to configure it

OTOH it could use derivatives and just deprecate old block plugin

I think option B makes sense in part becuase it's generically useful. I don't think adding a block for each is necessarily more helpful.

Actually it seems as though B and A are roughly 2 flavors of the same thing

Initial "sleepy" review, main points are BC (does it need change record) and update hook

NW for default value

1. +++ b/core/modules/search/config/schema/search.schema.yml
   @@ -88,3 +88,11 @@ search.page.*:
   +    search_page:
   +      type: string
   
   +++ b/core/modules/search/src/Plugin/Block/SearchBlock.php
   @@ -28,7 +59,49 @@ protected function blockAccess(AccountInterface $account) {
   +    $id = isset($this->configuration['search_page']) ? $this->configuration['search_page'] : NULL;
   +    return \Drupal::formBuilder()->getForm('Drupal\search\Form\SearchBlockForm', $id);
   ...
   +      'search_page' => 0,
   

   setting supposed to be a string (so default should be '')
   isset() not needed because there's defaults

2. +++ b/core/modules/search/src/Form/SearchBlockForm.php
   @@ -74,16 +74,20 @@ public function getFormId() {
   +  public function buildForm(array $form, FormStateInterface $form_state, $id = NULL) {
   ...
   +    if (!$id) {
   +      $entity_id = $this->searchPageRepository->getDefaultSearchPage();
   ...
   +    else {
   +      $entity_id = $id;
   

   I think $id argument could be replaced with $entity_id and fallback to default if null passed

3. +++ b/core/modules/search/src/Plugin/Block/SearchBlock.php
   @@ -15,7 +19,34 @@
   +class SearchBlock extends BlockBase implements ContainerFactoryPluginInterface {
   ...
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, SearchPageRepositoryInterface $search_page_repository) {
   +    parent::__construct($configuration, $plugin_id, $plugin_definition);
   +    $this->searchPageRepository = $search_page_repository;
   

   Not sure how to deal with BC - probably it's safe to just add container injection
   but what if contrib or custom already do that in child classes?

4. +++ b/core/profiles/standard/config/install/block.block.bartik_search.yml
   @@ -16,4 +16,5 @@ settings:
   +  search_page: ''
   

   means it needs post update hook and upgrade test

Constructor is not API according to https://www.drupal.org/core/d8-bc-policy#constructors

But not sure about for this case because nothing in contrib extends it http://grep.xnddx.ru/search?text=SearchBlock

Example upgrade is like https://git.drupalcode.org/project/drupal/commit/281c6f0

Working on upgrade path

Added upgrade path & a bit clean-up, will work on tests soon

Upgrade test added, now needs CR

And form builder should be injected one constructor changed

Attempt to fix tests
1) block module could be disabled
2) expected - no more cache tag on front

+++ b/core/profiles/demo_umami/config/install/block.block.umami_search.yml
@@ -16,4 +16,5 @@ settings:
+  search_page: node_search

+++ b/core/profiles/standard/config/install/block.block.bartik_search.yml
@@ -16,4 +16,5 @@ settings:
+  search_page: node_search

explicit default to content removes cache tag from front page!

I think we should default to node search in profiles because this explicitly tells what our profiles expect.
That's about default but upgrade defaults to empty because existing sites may have other search configured as default and it was only way to change where block points.
Also one less cache tag means no reason to load config for default searcher because blocks pre configured - and I think CR should suggest to configure searcher in block for performance reasons (-1 config cache read on every page)

In building the options in blockForm() the new description text says "Leave blank to use your default search page.", but seems as though the user would actually need to select the "Default" option? Should that be more distinctly not a search name like "- Default -" ?

Minor, but I think the code would be clearer if the variable and config keys indicate we are using a search page ID, rather than the etity. e.g.:
+ $page = $this->configuration['search_page'] ?? NULL;

Would be much clearer as:
+ $entity_id = $this->configuration['search_page_id'] ?? NULL;

Since the param to the form function is $entity_id

Makes sense to remove "search" prefix as config always bound to search block, should address #28.2

About #28.1 - I see no usages of "- Default -" in core, maybe makes sense to extend description pointing that current default is
So when block is placed end-user will know what is default?

Fixed last place and clean-up

Looks like all comments have been either integrated or addressed in #31. The only thing I found was a typo ("cacheablity" -> "cacheability") in a comment block. New patch and interdiff attached.

--- a/core/modules/search/src/Form/SearchBlockForm.php
+++ b/core/modules/search/src/Form/SearchBlockForm.php
@@ -82,7 +82,7 @@ public function buildForm(array $form, FormStateInterface $form_state, $entity_i
       // SearchPageRepository::getDefaultSearchPage() depends on
       // search.settings.  The dependency needs to be added before the
       // conditional return, otherwise the block would get cached without the
-      // necessary cacheablity metadata in case there is no default search page
+      // necessary cacheability metadata in case there is no default search page
       // and would not be invalidated if that changes.
       $this->renderer->addCacheableDependency($form, $this->configFactory->get('search.settings'));
     }

Let's get commiter's attention

+1 looks good

Committed ce189a3 and pushed to 8.8.x. Thanks!

Published the change record