Do not set an error message when "Fulltext search" filter fails its "Required" validation

I wonder if this could be from src/ParamConverter/SearchApiConverter.php

    $current_user_id = $this->currentUser->id() ?: session_id();

The fatal error you mentioned in #2 is already fixed in search_api_solr 8.x-1.x-dev.

The cookie is obviously a standard session cookie. I don't think that the solr backend forces opening a session. Therefore I move this issue to the Search API queue.

I also looked into this, together with Acquia colleague @wouter.adem.

The cookie is set because the exposed filter form (a GET form) is automatically submitted (because it is a GET form). Then, during form validation, this runs:

      // Ensure that a #required form error is thrown, regardless of whether
      // #element_validate handlers changed any properties. If $is_empty_value
      // is defined, then above #required validation code ran, so the other
      // variables are also known to be defined and we can test them again.
      if (isset($is_empty_value) && ($is_empty_multiple || $is_empty_string || $is_empty_value)) {
        if (isset($elements['#required_error'])) {
          $form_state->setError($elements, $elements['#required_error']);
        }
        // A #title is not mandatory for form elements, but without it we cannot
        // set a form error message. So when a visible title is undesirable,
        // form constructors are encouraged to set #title anyway, and then set
        // #title_display to 'invisible'. This improves accessibility.
        elseif (isset($elements['#title'])) {
          $form_state->setError($elements, $this->t('@name field is required.', ['@name' => $elements['#title']]));
        }
        else {
          $form_state->setError($elements);
        }
      }

Why does this validation logic run? Because the exposed filter is required by default. And if you make it un-required, the error cited in #2 happens.

So… the problem is somewhere in the fact that a Views-powered Search API block is making a search string required, which makes no sense — because searching is an optional activity on any site.

Then if you make the search optional, the validation doesn't happen, hence the error doesn't occur, hence the cookie doesn't get set … but because there is no validation error, the view is executed, which in this case means the search is executed (and note that the search was only not executed previously because of the form validation error) … and that is the real bug here, which was being masked.

Tricky stuff!

Updated title a little bit.

This is definitely worth the critical because it 100% breaks varnish.

Capturing the exact root cause described in #8 in the title.

@kylebrowning - If I'm reading this correctly, it would also break all of Drupal's page-level caches (Dynamic page cache, Page cache) in addition to any upstream caches (Varnish, Fastly, etc.).

If it were just upstream caches, I don't think that'd be enough to warrant a critical.

I think this is what's going on? Is what I'm doing in the test correct? Because if it is, there might be something going on, there's no errors now.

It is however 1000% possible that I'm doing something wrong in the test.

Improved comments in test + use BTB methods. Still no clue why the test is not failing.

Sorry, but I'm gonna need much more information than that to do anything here.

First off, @ dakku, I can't reproduce your problem, sorry. (Which is supported by Joris' test not failing, I'd say.) When I set the "Fulltext search" filter to be "Required" (see attached view export), it does give me a validation error on first going to the page (as expected). However, it doesn't set any kind of cookie – neither with the DB backend nor with Solr. So that needs further explanation/investigation, in my opinion. E.g., what other modules are you using that might be involved here? Are you using normal Solr, or with the Acquia Search module?
If just setting a form error breaks caching, and there needn't be a cookie involved, the issue summary (or at least any of the comments) should reflect this.

Then, @ Wim: nothing in #8 describes a bug, as far as I can see.
When you set the filter to "Required", we validate that it's actually there. Yes – what else? If you don't want that, don't set the filter to "Required", or set a default value.
When you don't set it to "Required" then the view will get executed even if it's not there. Once again: what else should we do?
All of this is not a bug, it's completely normal Views behavior. If you do this with a standard view, it will be exactly the same. The problem is not the code, it's your configuration.

For avoiding both these problems, Views already has a solution: the "Input required" exposed form style. Just use that, it should do exactly what you want (though, of course, it also has some problems). But that's got nothing to do with this module. (One reasonable feature request here would be to make the default search view included in the DB Defaults module use that exposed form style – might make sense. But hardly a critical bug report.)

So, please, explain again how this is any kind of bug, or I'll have to mark this as "works as expected".
If, of course, we find out that Search API (or Solr) code is indeed to blame for setting a cookie in some such cases, that would indeed be a bug to be investigated – that definitely shouldn't happen. But, as said, I can't reproduce that part.

I wonder if this could be from src/ParamConverter/SearchApiConverter.php

$current_user_id = $this->currentUser->id() ?: session_id();

No, that's only active on a few admin pages – if you get to that code, you definitely have some session cookies anyways.

The cookie is obviously a standard session cookie. I don't think that the solr backend forces opening a session. Therefore I move this issue to the Search API queue.

Since the issue reporter clearly states it only occurs in combination with the Solr backend, and that they've tested that, it does seem somehow dependent on it, though maybe indirectly. I'd also not be aware how the Search API would set any session cookie, so that's not really an argument.

That being said, though, I can reproduce this with neither DB nor Solr backend, so we might as well discuss this here. For me, it currently just seems like a misconfigured view.

#13: I could not disagree more.

#16:

it does give me a validation error on first going to the page (as expected). However, it doesn't set any kind of cookie – neither with the DB backend nor with Solr. So that needs further explanation/investigation, in my opinion.

It's not Search API that sets a cookie. A form validation error calls drupal_set_message() which sets $_SESSION['messages'] which triggers a session cookie.

Suggested solution: Search API should make it impossible to make such an exposed filter required.

If you don't want that, don't set the filter to "Required", or set a default value.
When you don't set it to "Required" then the view will get executed even if it's not there.

Setting to required is a no-go as per the above, because that triggers a validation error. How can you set a default value to not trigger a search? Why is this always executed? If there's nothing to search, then Search API should not search (or perhaps return the empty result set?).

I don't know anything about Search API's architecture/execution model, nor do I know enough about Views. But what I do know, is that this configuration is extremely misleading. So I can find myself in:

All of this is not a bug, it's completely normal Views behavior. If you do this with a standard view, it will be exactly the same. The problem is not the code, it's your configuration.

(One reasonable feature request here would be to make the default search view included in the DB Defaults module use that exposed form style – might make sense. But hardly a critical bug report.)

Especially that second quote. It is completely beyond me why you would ever want to release a 1.0 stable with such a massive usability problem. Because it'll cause many support requests. And once a site uses a Search API-powered view, it's in configuration, and updating configuration retroactively may be impossible.

I urge those who are working on sites with exactly this problem to report more details ASAP. It surprises me that that hasn't already happened. Without further information from you, the module maintainers can't help.

Finally, to be clear: I wholly defer to the maintainers of this module to determine whether should block the long-awaited 1.0 stable release or not.

Again, I don't know the code well enough. I just know the reported behavior will be a showstopper for many sites: I marked this critical for that reported behavior, which as far as I can tell has its origin in Search API's Views integration. No matter if Views is to blame or not, people will come and report this here, especially if it's a default, which it indeed seems to be: modules/search_api_db/search_api_db_defaults/config/optional/views.view.search_content.yml.

It's not Search API that sets a cookie. A form validation error calls drupal_set_message() which sets $_SESSION['messages'] which triggers a session cookie.

Not when I tested it. (Nor for the test bot, as you can see above.) As far as I know, $_SESSION['messages'] is removed again if the message is displayed in the same page request (see drupal_get_messages()). It's only used to "remember" messages if they can't be displayed right away.
Of course, this might still break caching, I guess, maybe in conjunction with BigPipe, or some other setups. You obviously know more about caching than I do. I'm not gonna argue against this being a problem for caching, and it's certainly clear that having a page where the user sees an error by default is a bad idea.

Whatever the consequence, though, I maintain that this is just a configuration problem. It might be confusing to you, but it's exactly like other views work, too. So I daresay that changing this in any major way (maybe even breaking existing views) would confuse a lot more users than this currently does.
The fact that we already have 12k (reported) installations in D8, with no-one complaining about this until now, supports this claim, I think.
I'm certainly not happy with all aspects of how Views work, and this specifically also doesn't look very sensible, but it's been like this for a decade or so, I guess, and I'm certainly not gonna change it just in this one module.

Making the "Required" checkbox function the same basically, but maybe not setting a status message when it triggers, might be an improvement to look into. Or maybe having a separate option that does that, or modifies "Required" that way – we can certainly discuss that, and would have to take a closer look.
But that's just a feature request, possibly major, but hardly a "critical bug". As said, Views has been working like this for around a decade, and the same goes for the Search API Views integration since it started working almost seven years ago. This shouldn't block a stable release a lot of people are waiting for.

@ Joris: What's your opinion on the switch of the "Exposed form style" to "Input required"? I guess that does make sense, but would you agree with Wim that this is important enough to go in before 1.0? As you know, I'd rather release ASAP. (Same for my suggestion in the above paragraph, though that would take a bit longer to implement, so should be considered even more carefully.)

Apart from anything else, though, the test fails in #15 regarding 8.2 are very good to know: #2870782: Fix tests for Drupal 8.2.

These are the steps so the error can be reproduced locally, plus some files and screenshots.

Some general info about Drupal core and contributed modules that were used.

• Drupal Core: 8.2.7
• Lightning: 2.0.5
• search_api: 1.0-rc2
• search_api_solr: 1.0-beta2
• better_exposed_filters: 3.0-alpha2 (probably not relevant since the issue seemed to be there when we disabled this module)

The config file views.view.search contains with the following relevant extracts:

cache:
  type: search_api_time
  options:
    results_lifespan: '1800'
    results_lifespan_custom: '0'
    output_lifespan: '1800'
    output_lifespan_custom: ‘0'
cache_metadata:
  max-age: 0
  contexts:
    - 'languages:language_content'
    - 'languages:language_interface'
    - url.query_args
    - user.permissions
  tags:
    - 'config:field.storage.node.field_event_start_date_time'
    - 'config:field.storage.node.field_search_image'
    - ‘config:field.storage.node.field_search_promote'
search_api_fulltext:
  id: search_api_fulltext
  table: search_api_index_acquia_search_index
  field: search_api_fulltext
  relationship: none
  group_type: group
  admin_label: ''
  operator: or
  value: ' '
  group: 1
  exposed: true
  expose:
    operator_id: search_api_fulltext_op
    label: Title/Keyword
    description: ''
    use_operator: false
    operator: search_api_fulltext_op
    identifier: s
    required: true
    remember: false
    multiple: false
    remember_roles:
      authenticated: authenticated
      anonymous: '0'
      administrator: '0'
      content_admin: '0'
      content_publisher: '0'
      content_editor: '0'
  is_grouped: false
  group_info:
    label: ''
    description: ''
    identifier: ''
    optional: true
    widget: select
    multiple: false
    remember: false
    default_group: All
    default_group_multiple: {  }
    group_items: {  }
  parse_mode: direct
  min_length: null
  fields: {  }
  plugin_id: search_api_fulltext

As you can see the fulltext search is set to required.

Steps:

1. Remove all COOKIES (expect my XDebug session) using Developer Tools.
2. Clear the cache_render and cache_dynamic_page_cache tables
3. Run curl -s -D

Output:

HTTP/1.1 200 OK
Date: Tue, 18 Apr 2017 18:32:18 GMT
Server: Apache/2.4.17 (Unix) OpenSSL/1.0.1h mod_fcgid/2.3.9
X-Powered-By: PHP/5.6.14
Cache-Control: must-revalidate, no-cache, private
X-Drupal-Dynamic-Cache: UNCACHEABLE
Link: </node/40496>; rel="shortlink", </node/40496>; rel="revision"
X-UA-Compatible: IE=edge
Content-language: en
X-Content-Type-Options: nosniff
X-Frame-Options: SameOrigin
Expires: Sun, 19 Nov 1978 05:00:00 GMT
X-Generator: Drupal 8 (https://www.drupal.org)
Content-Security-Policy: report-uri /report-csp-violation
X-Content-Security-Policy: report-uri /report-csp-violation
X-WebKit-CSP: report-uri /report-csp-violation
Strict-Transport-Security: max-age=1000
Set-Cookie: SESS46641659cb1b97dcd0a426bba010e339=HZ-xCFfEcUZVDND8KniJK5SXBMF4hwKeBD6UFABuD3E; expires=Thu, 11-May-2017 22:05:40 GMT; Max-Age=2000000; path=/; domain=.dev.dd; HttpOnly
Transfer-Encoding: chunked
Content-Type: text/html; charset=UTF-8

Some debugging info:
Checking the $_SESSION and form_state in XDebug:

• Breakpoint in FormState.php and looking into the form_state variable
• Breakpoint in FinishResponseSubscriber.php and looking into the $S_SESSION

Gives the following info:

Then given the comment from bootstrap.inc, this explains why the session cookie is set.

Messages are stored in a session variable and displayed in the page template
* via the $messages theme variable

I hope given the info from above this could help.

Wouter

I don't know anything about Search API's architecture/execution model, nor do I know enough about Views. But what I do know, is that this configuration is extremely misleading.

For me, this is a showstopper. If Wim, with all his drupal-knowledge is confused with the current behavior, what are us normal humans supposed to do?

@ Joris: What's your opinion on the switch of the "Exposed form style" to "Input required"? I guess that does make sense, but would you agree with Wim that this is important enough to go in before 1.0? As you know, I'd rather release ASAP. (Same for my suggestion in the above paragraph, though that would take a bit longer to implement, so should be considered even more carefully.)

I know we'd like to release sooner rather than later, but I'd very much love to see this bug squashed before release. Not sure if changing the default will help, I'd try to to take a look at: \Drupal\search_api\Plugin\views\filter\SearchApiFulltext::validateExposed to see if we can resolve the error there.

@Wim: I don't see the error in the test I uploaded in #15, is there anything we're missing there to help reproduce the bug? Maybe we can change the behavior there from setting the validation error to just returning?

In any case. I'm convinced that we need to write a test that fails before and passes after the fix is applied.

You're giving too much credit to me ;)

I'm heavily time-constrained this week, so I'm concerned about holding up this issue and that possibly being unnecessary.

@wouter.adem: Why is the field set to required? The default search view doesn't have the exposed filter set to required. But if you make it optional, there's an error deep in Search API. Can you make it optional, and then post a comment describing the consequences? I think that will e very helpful.

So when:

• Uncheck the 'required' field
• Save the view
• Clear render/page cache
• curl -s -D

Then I get the following output:

On first curl request:

HTTP/1.1 200 OK
Date: Wed, 19 Apr 2017 08:58:00 GMT
Server: Apache/2.4.17 (Unix) OpenSSL/1.0.1h mod_fcgid/2.3.9
X-Powered-By: PHP/5.6.14
Cache-Control: must-revalidate, no-cache, private
Link: </node/40496>; rel="shortlink", </node/40496>; rel="revision"
X-UA-Compatible: IE=edge
Content-language: en
X-Content-Type-Options: nosniff
X-Frame-Options: SameOrigin
Expires: Sun, 19 Nov 1978 05:00:00 GMT
X-Generator: Drupal 8 (https://www.drupal.org)
Content-Security-Policy: report-uri /report-csp-violation
X-Content-Security-Policy: report-uri /report-csp-violation
X-WebKit-CSP: report-uri /report-csp-violation
Strict-Transport-Security: max-age=1000
Set-Cookie: SESS46641659cb1b97dcd0a426bba010e339=fr4Q18EQo5Xa8E9wTW_Z9rDpOluZJ5d1vryodsSPgpE; expires=Fri, 12-May-2017 12:31:36 GMT; Max-Age=2000000; path=/; domain=.dev.dd; HttpOnly
Transfer-Encoding: chunked
Content-Type: text/html; charset=UTF-8

Next curl request:

HTTP/1.1 500 500 Service unavailable (with message)
Date: Wed, 19 Apr 2017 08:58:19 GMT
Server: Apache/2.4.17 (Unix) OpenSSL/1.0.1h mod_fcgid/2.3.9
X-Powered-By: PHP/5.6.14
Cache-Control: no-cache
X-Content-Type-Options: nosniff
Connection: close
Transfer-Encoding: chunked
Content-Type: text/html; charset=UTF-8

with error trace:

The website encountered an unexpected error. Please try again later.
Recoverable fatal error: Argument 1 passed to Drupal\search_api\Utility\QueryHelper::addResults() must implement interface Drupal\search_api\Query\ResultSetInterface, null given, called in /docroot/modules/contrib/search_api/src/Plugin/views/cache/SearchApiCachePluginTrait.php on line 154 and defined in Drupal\search_api\Utility\QueryHelper->addResults() (line 86 of modules/contrib/search_api/src/Utility/QueryHelper.php).
Drupal\search_api\Utility\QueryHelper->addResults(NULL) (Line: 154)
Drupal\search_api\Plugin\views\cache\SearchApiTimeCache->cacheGet('results') (Line: 1406)
Drupal\views\ViewExecutable->execute(NULL) (Line: 1441)
Drupal\views\ViewExecutable->render() (Line: 2391)
Drupal\views\Plugin\views\display\DisplayPluginBase->preview() (Line: 1649)
Drupal\views\ViewExecutable->preview('page_1', Array) (Line: 63)
Drupal\views\Element\View::preRenderViewElement(Array)
call_user_func(Array, Array) (Line: 376)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array) (Line: 474)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html', NULL, 1) (Line: 134)
__TwigTemplate_45d6b3a0933aa9ead5190b8cf2ce822260c77babaddf785c5c968e70e53e9cb0->doDisplay(Array, Array) (Line: 432)
Twig_Template->displayWithErrorHandling(Array, Array) (Line: 403)
Twig_Template->display(Array) (Line: 411)
Twig_Template->render(Array) (Line: 64)
twig_render_template('themes/custom/templates/layout/html.html.twig', Array) (Line: 384)
Drupal\Core\Theme\ThemeManager->render('html', Array) (Line: 435)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array) (Line: 147)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 148)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object) (Line: 149)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 64)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 652)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Let me know if an XDebug output could help here.

Actually, this seems to be a problem of not being able to show any results, so this is probably not related.

Also, had to update a file in Lightning "drupal/lightning" so it uses search_api_time.
Will test this on a stable environment and let the outcome know.

If Wim, with all his drupal-knowledge is confused with the current behavior, what are us normal humans supposed to do?

He himself said, "I don't know […] enough about Views." While I wouldn't doubt his prowess regarding, e.g., development in the caching system, it's perfectly possible that he doesn't have much experience creating views. (It's similar for me: while I think it's safe to say I'm an expert for programming search functionality, give me a lot of pretty basic site builder tasks and I'd be stumped.)

And as, again, no-one else has really complained about this before, at least definitely not in as strong a way, I'd say it's likely that most people have not such a problem with this. Especially: We had this running like this in Drupal 7 for ages, with currently 80k reported installations, and no major complaints about this. So, improving usability: definitely, why not? But holding up the stable release for it and having to implement and test this under a lot of time pressure: I'd vote against that.

Anyways, attached is a patch that should implement all this:

• Makes the "Required" setting for the fulltext filter (no other filter was changed yet) fail without an error message.
• Sets the filter to "Required" in our default view, and also sets its exposed form to "Input required".
• Adds tests verifying both the filter's behavior and the settings in the default view. (Since the setting of the session cookie obviously has some further dependencies, this is currently not tested. If setting a message leads to the cookie (sometimes), verifying that there is no message should be good enough.)

If a few people here could test this and confirm that this has a better UX, and also solves the cookie problem for them, that would be awesome! If we get a few reviews quick enough, then maybe we can commit this before the stable release after all. But usually, UI changes and feature requests are a big no-go during RC phase, for pretty good reasons. So maybe we should then at least do another RC release before stable.

In any case, this currently still has a problem, in my opinion, namely that it now doesn't communicate the required search keywords at all. (Though the empty message we got previously was also not optimal, of course.) This is mitigated if you use the "Input required" exposed form style, since that will enable you to display a message, but if you use "Basic", or the user just submits the form without entering any keywords (as we know, "Input required" isn't smart enough to detect that case), you get a page with just the form and no results, without any indication what's going on. (Or, probably even worse, the "No results behavior" message, telling you there were no results for your search.)
So maybe we should separate the cases of "no input yet" and "input is there, but empty" and still set a form error in the latter case. Would that be OK, in your opinion?

@ Wim:

Why is the field set to required?

I thought having this "Required" is the whole point of this issue? Now I'm really confused. What "error deep in Search API" is there if you don't have it "Required"?

@ #23: Yes, that looks like an entirely different issue, at least as far as I understand. But, as said above, I thought this issue was about "Required" being enabled and making problems in the first place.
For the issue you describe in #23, see #2871030: Cached Search API results in Views are NULL.

Oh, forgot the tests-only patch in #24.

Maybe there is also some mis-understanding which is totally my fault because I dragged Wim into this and he doesn't have the full background.

Wim mentioned:

Because the exposed filter is required by default.

This is not true (I think), but we had to set it to required since that broke our site because of an error related to search_api_solr.
Updating to search_api_solr: 1.0-beta2 did solve that locally. So that's something that Wim was not aware off and what he saw, was the required field being checked in our view assuming it was set to required per default.

After updating that module we could avoid setting it to required, as expected.
But the error in #23 occored and (see #2871030: Cached Search API results in Views are NULL) does not solve this.

Wim mentioned:

Because the exposed filter is required by default.

This is not true (I think), but we had to set it to required since that broke our site because of an error related to search_api_solr.

Yeah, you're right, that part is definitely wrong. (Same for the sentence immediately following it.) This issue is getting pretty chaotic, I think, with multiple different problems discussed, some bugs, some UX issues, and most of them unrelated to the original issue reported.
I've edited the issue summary to reflect the problem that I think is being discussed here, and which my patch attempts to solve. If there are other problems (except #2871030: Cached Search API results in Views are NULL – please comment there if you tested that patch), we should probably open new issues for those, with clear descriptions.

#24:

While I wouldn't doubt his prowess regarding, e.g., development in the caching system, it's perfectly possible that he doesn't have much experience creating views.

Exactly. Nobody knows all the things in Drupal.

But holding up the stable release for it and having to implement and test this under a lot of time pressure: I'd vote against that.

+1 — that's what I said (or tried to) in my last comment.

Anyways, attached is a patch that should implement all this:

Yay!

#28:

After updating that module we could avoid setting it to required, as expected.

Great!

#30:

This issue is getting pretty chaotic

Agreed.

Patch review:

1. +++ b/modules/search_api_db/search_api_db_defaults/config/optional/views.view.search_content.yml
   @@ -33,15 +33,17 @@ display:
   -          submit_button: Apply
   +          submit_button: Search
   

   This makes sense!

2. +++ b/modules/search_api_db/search_api_db_defaults/config/optional/views.view.search_content.yml
   @@ -33,15 +33,17 @@ display:
   +          text_input_required: 'Please enter some keywords to search.'
   

   Also makes sense!

3. +++ b/modules/search_api_db/search_api_db_defaults/config/optional/views.view.search_content.yml
   @@ -33,15 +33,17 @@ display:
   +          text_input_required_format: basic_html
   

   Does not make sense. Why is this not simply plain text?

   I looked at \Drupal\views\Plugin\views\exposed_form\InputRequired to understand why — apparently this is tightly coupled to formatted text for some reason… I wonder why.

   Oh… apparently this text format is not applied to the required input, but:

   Text to display instead of results until the user selects and applies an exposed filter.
   

   Wow this is confusingly named, but of course the fault of Views, not Search API!

4. +++ b/modules/search_api_db/search_api_db_defaults/config/optional/views.view.search_content.yml
   @@ -120,9 +133,13 @@ display:
        cache_metadata:
          contexts:
   +        - 'languages:language_content'
            - 'languages:language_interface'
            - url
   +        - url.query_args
          cacheable: false
   +      max-age: 0
   +      tags: {  }
   

   This suggests that this default view was created years ago, long before the D8 release. So it's been wrong all this time, which might have caused edge case-y errors itself. Glad to see this resolved :)

5. +++ b/modules/search_api_db/search_api_db_defaults/tests/src/Functional/IntegrationTest.php
   @@ -101,7 +101,15 @@ public function testInstallAndDefaultSetupWorking() {
   +    $this->assertSession()->pageTextContains('Please enter some keywords to search.');
   +    $this->assertSession()->pageTextNotContains($title);
   +    $this->assertSession()->responseNotContains('Error message');
   +    $this->submitForm([], 'Search');
   +    $this->assertSession()->pageTextNotContains($title);
   +    $this->assertSession()->responseNotContains('Error message');
   +    $this->submitForm(['keys' => 'test'], 'Search');
        $this->assertSession()->pageTextContains($title);
   +    $this->assertSession()->responseNotContains('Error message');
   

   Excellent!

6. +++ b/src/Plugin/views/filter/SearchApiFulltext.php
   @@ -205,6 +205,17 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
   +    // We use custom validation for "required", so don't want it set on the form
   +    // element.
   +    $form['#required'] = FALSE;
   

   Where is this custom validation? Can we @see it?

7. +++ b/src/Plugin/views/filter/SearchApiFulltext.php
   @@ -224,8 +235,11 @@ public function validateExposed(&$form, FormStateInterface $form_state) {
   +    // If there is no input, we just have to make sure that none is required.
        if (!trim($input)) {
   +      if (!empty($this->options['expose']['required'])) {
   +        $this->getQuery()->abort();
   +      }
          return;
   

   Hm, does this comment really match the logic here?

   If there is no input, and the "required" config option is set to "true", then we abort the query, followed by an early return.

   We abort the query, because the query is what would cause an error, because there are no keywords to search? And without this explicit abortion, the return statement below would cause the validation of the input to succeed, and then during the query execution we'd get an error?

   I think that's what's happening?

   Perhaps we can get a slightly longer comment explaining this? This is not obvious to me — but if it's obvious to the Search API maintainers, then please ignore this request for a more detailed comment :)

8. +++ b/tests/src/Functional/ViewsTest.php
   @@ -286,6 +287,28 @@ public function testView() {
   +    $this->checkResults([], [], 'Search without required fulltext keywords');
   +    $this->assertSession()->responseNotContains('Error message');
   

   Excellent!

Thanks for your detailed comment! Good to see the issue getting back on track. Now I'd just like Joris' OK for releasing 1.0 – if he still thinks this is too much of a usability problem than I guess we should still try to fix it.

The attached addresses your comments 6 and 7, and also fixes a problem I just discovered that the "Required" validation was dependent on a "Minimum keyword length" being said – hardly ideal. (Such things are exactly why I'd need several reviews and tests before feeling confident in committing this between the last RC and the first stable release.)

We abort the query, because the query is what would cause an error, because there are no keywords to search? And without this explicit abortion, the return statement below would cause the validation of the input to succeed, and then during the query execution we'd get an error?

No, not at all. The Search API has no problem executing a search without keywords (or even without any other filters/conditions). A lot of people, e.g., use it to replace admin listings (like admin/content), which of course shouldn't require keywords. Maybe this is where part of your confusion originated?
We abort the query just so it doesn't return any results. Without this abort(), it would just return all items (except for those excluded by other filters, of course) – which is not what people want when they set a filter to "Required".
I think the comment should have been clear enough for most people familiar with Search API development, but I guess there's no harm in being more verbose. Is the new phrasing better?

1. +++ b/src/Plugin/views/filter/SearchApiFulltext.php
   @@ -205,17 +205,23 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
   +    // We use custom validation for "required" (see validateExposed()), so don't
   +    // want the Form API to interfere.
   

   I expected to see:

   // We use …
   // @see ::validateExposed()
   

2. +++ b/src/Plugin/views/filter/SearchApiFulltext.php
   @@ -224,8 +230,19 @@ public function validateExposed(&$form, FormStateInterface $form_state) {
   +    // If it was, then we "abort" the query to just return no results.
   

   Thanks for the explanation in #32, much clearer now! Comment is a big improvement, but I think this would be even clearer:

   If it was required yet not provided, then abort the query so no results are returned.
   

3. +++ b/src/Plugin/views/filter/SearchApiFulltext.php
   @@ -224,8 +230,19 @@ public function validateExposed(&$form, FormStateInterface $form_state) {
   +      // In any case, if there is no input checking word lengths doesn't make
   +      // any sense.
   

   I propose:

   // If the input is empty, there is nothing to validate: return early.
   

4. +++ b/src/Plugin/views/filter/SearchApiFulltext.php
   @@ -224,8 +230,19 @@ public function validateExposed(&$form, FormStateInterface $form_state) {
   +    // Only continue if there is a minimum word length set.
   +    if ($this->options['min_length'] < 2) {
   

   This < 2 condition seems like an unnecessarily confusing. Surely, min_length can never be 0 or 1, because those minimum lengths simply don't make sense? So surely, we validate those numbers when the user is configuring this through the UI?

   I had to read all the subsequent code to understand this!

   Improving this is probably out of scope though.

I agree with everything #33 said except .4. That is definitly out of scope if we'd want to fix that.

I think we should commit this and release 1.0 after that.

#34: agreed with #33.4 being out of scope. I already said that, actually :)

I have no strong opinion whether this should be committed before 1.0. Although the fact that this is changing modules/search_api_db/search_api_db_defaults/config/optional/views.view.search_content.yml is a strong indication that this is preferable: otherwise all sites that start using Search API because it reached 1.0 will start with incorrect default configuration!

Furthermore, the changes here seem very low-risk to me, and come with test coverage.

As far as I'm concerned, this is RTBC. #33 only has comment nitpicks. So, being bold here in order to signal to @drunken monkey that this is ready.

@drunken monkey Related to the patch in #32 and to confirm that it solves the following tasks:

• Test the patch, both regarding the session cookie problem and the error message UX problem: Confirmed, works as expected
• No (empty) error message when first visiting such a search view: Confirmed, no error message is shown anymore.

Thanks, this was really useful to follow.

Wouter

#34: agreed with #33.4 being out of scope. I already said that, actually :)

Ah, I disagreed with the initial point and agreed with it being out of scope.
Communication over text is hard. :-)

Thanks for coming back here and confirming that the patch works @wouter.adem.

I expected to see:

// We use …
// @see ::validateExposed()

Since @see doesn't work in inline comments, I usually don't use it there. (Also, the correct syntax is debated – according to the current standards, I think we'd have to include the FQN of the class, too.) Also, validateExposed() is just the next method.

Anyways, since Joris agreed, I've now applied all your suggestions. Thanks again for the detailed review!

And if you also, more or less strongly, agree that this should go in before stable, then let's do that!
However, I think I'm still gonna create a last RC for a few days before finally making the module stable. Would be really good to have more than one person verifying that this works well for them.

So: committed. Thanks again, everyone!

@drunken monkey Sorry, I was a little to fast when confirming that all code from the patch worked.

cfr. #31.7 the comments from Wim

+    // If there is no input, we check whether the filter was set to "Required".
+    // If it was, then we "abort" the query to just return no results.
     if (!trim($input)) {
+      if (!empty($this->options['expose']['required'])) {
+        $this->getQuery()->abort();

This raises a fatal error:
Fatal error: Call to a member function abort() on null in modules/contrib/search_api/src/Plugin/views/filter/SearchApiFulltext.php on line 237

I also got fatal error on all pages after updating, because there is a site search box in the header. Looking at what the error comes from though, I tweaked the config yaml to change the "required" settting to false, and after that no more error. However it would be useful to be able to set the search text field as required...

Good thing @drunken monkey did another RC!

For what it's worth I can tell that on line 272 in search_api/src/Plugin/views/filter/SearchApiFulltext.php the early return causes the $query not being set cfr.

public function query() {
    while (is_array($this->value)) {
      $this->value = $this->value ? reset($this->value) : '';
    }
    // Catch empty strings entered by the user, but not "0".
    if ($this->value === '') {
      return;
    }

This causes the fatal error on the $this-getQuery() method, which returns NULL cfr.

// If there is no input, we check whether the filter was set to "Required".
    // If it was, then we "abort" the query to just return no results.
    if (!trim($input)) {
      if (!empty($this->options['expose']['required'])) {
        $this->getQuery()->abort();
      }
      // In any case, if there is no input checking word lengths doesn't make
      // any sense.
      return;
    }

We probably need to set 'some' query object so we can abort properly.

Instead of setting some query object, we should probably do if ($this->getQuery === NULL) { return; } at the top of the validation method? No query object means no query should be aborted because no query will be executed.

I think, not sure. Letting @drunken monkey confirm.

Good thing @drunken monkey did another RC!

Woah, yeah, thank god for that … ! I know why I didn't want to commit this this late in the process …

Anyways, Joris is right, we just shouldn't abort the query when there is none. Just didn't realize this was possible – but apparently, this is the case when the exposed form for a page display is displayed in a block (potentially on other pages on the site). Man, Views is fucked up …
Please see/test/review the attached, simple patch!

After this, I really need a vacation.

@ wouter.adem/#44: Please stop making suggestions!

Man, Views is fucked up …

:P

After this, I really need a vacation.

:) I'll let @borisson_ review this, that's probably safer.

Please stop making suggestions!

@wouter.adem is just trying to help. He was himself very much doubting whether to post this. I encouraged him to post it anyway, in case it might help. In my experience, those who don't know a particular piece of code inside-out often come up with suggestions/ideas that end up not being the solution, but do make the solution better.

I looked at the patch and that does exactly what I figured it should do, great work @drunken monkey.

Thanks for the suggestion @wouter.adem: you pinpointed the problem perfectly! The suggestion was also good and I really appreciate it, it allowed us to come up with the solution that ended up in the patch, great work!

You're right, of course, I shouldn't have written that. I apologize, Wouter!
This issue just has me pretty annoyed by now (as evidenced by the recent influx of expletives into my comments), but of coruse I understand that people with incomplete knowledge of the code base can still come up with good suggestions and remarks, and should be encouraged to try, not deterred.
So, again, I'm sorry! It won't happen again.

Anyways, back to the patch, it's good to hear you think it's fine like this, but I'm shocked none of you tagged this "Needs tests"! :P I went ahead anyways, the attached should do it. (Interdiff == tests-only.)

Test looks sufficient.

Good to hear, thanks for reviewing again!
Committed. Onwards, to RC 4!
Thanks again, everyone!

#49:

Thank you for taking a step back and apologizing :) It again shows how constructive and well-intentioned this community is! <3

Also: you already added tests! :D That's why I didn't add the Needs tests issue tag! You're right that we should've asked for a failing test-only patch though!

Glad to see this resolved!

Added #2872280: Set error message when required fulltext filter is submitted empty as a follow-up.

Hm, also, I guess a back-port to D7 wouldn't hurt?