Steps to reproduce:
- create a facet (e.g. content type)
- filter on a value
- remove the value id from the url (e.g. /search?f[0]=content_type%3A instead of /search?f[0]=content_type%3Apage)
The following error will occur:
The website encountered an unexpected error. Please try again later.
Error: Unsupported operand types in Drupal\search_api_solr\Plugin\search_api\backend\SearchApiSolrBackend->reduceFilterQueries() (line 1647 of modules/contrib/search_api_solr/src/Plugin/search_api/backend/SearchApiSolrBackend.php).
Drupal\search_api_solr\Plugin\search_api\backend\SearchApiSolrBackend->reduceFilterQueries(Array, Object) (Line: 1604)
Drupal\search_api_solr\Plugin\search_api\backend\SearchApiSolrBackend->createFilterQueries(Object, Array, Array) (Line: 1564)
Drupal\search_api_solr\Plugin\search_api\backend\SearchApiSolrBackend->getFilterQueries(Object, Array, Array) (Line: 911)
Drupal\search_api_solr\Plugin\search_api\backend\SearchApiSolrBackend->search(Object) (Line: 427)
Drupal\search_api\Entity\Server->search(Object) (Line: 346)
Drupal\search_api\Query\Query->execute() (Line: 501)
Drupal\search_api\Plugin\views\query\SearchApiQuery->execute(Object) (Line: 1412)
Drupal\views\ViewExecutable->execute(NULL) (Line: 1440)
Drupal\views\ViewExecutable->render() (Line: 171)
Drupal\views\Plugin\views\display\Page->execute() (Line: 1615)
Drupal\views\ViewExecutable->executeDisplay('page_1', Array) (Line: 78)
Drupal\views\Element\View::preRenderViewElement(Array)
call_user_func(Array, Array) (Line: 381)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array, ) (Line: 226)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 227)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object) (Line: 117)
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: 98)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 77)
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: 628)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
Also fixed a typo :-)
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | add_backend_tests_for-2767609-16.patch | 3.64 KB | borisson_ |
Comments
Comment #2
mpp commentedComment #3
berdirI've seen this one too, but it might be better to catch this, in my case it happened because an empty or condition group was added, which doesn't make sense.
Comment #4
mkalkbrennerOK, to get that error reported is bad thing. But it happens if the Query contains illegal conditions.
From my point of view the Facets module, which introduces and handles that 'f' parameter, should verify it's parameters itself and not create an illegal query condition.
Next, Search API should avoid sending incomplete conditions to the backends instead of having each backend implementing such checks.
Moving to Facets first for further discussions ...
Comment #5
mpp commentedHey what about the typo :-)
Comment #6
borisson_I agree that we should make sure that an empty value can't be added onto a query.
This should be something that facets handles.
Comment #7
borisson_This should fix that.
Comment #8
mpp commentedRTBC, thanks!
Comment #9
drunken monkeyI have only looked at the proposed solution, but it seems the Solr backend isn't able to handle an empty string as a condition value? That is definitely a bug there, then – why wouldn't that be a valid value?
Maybe we should add that to our backend base test, too …
Regarding the patch for Facets, I'm not sure: there might be cases where an empty string as a value does make sense? Not in this case, sure, but if there's no general validation for "this is an actual facet value that was clicked" I'm not sure adding it for this special case makes sense. And, as said, it could be an actual value in some (probably rare) cases, so hard-coding that doesn't seem like a good idea.
In the end, users will (probably) always be able to arrive at empty results pages, or even error messages, through URL hacking. And in this specific case, the error isn't even your fault, but that of the Solr backend.
Comment #10
mkalkbrennerI think in this particular use case, an empty value is an invalid value.
But it's a good idea to have a test case in the backend test base that covers a real use-case of an empty value. That will allow us to adjust the Solr backend if required.
I guess that at least an empty value isn't allowed for all operators.
Comment #11
borisson_So I guess this can be moved to the search api queue and I don't need to commit #7?
Comment #12
drunken monkeyYes, exactly. We should add a test to the Search API, and then create a new issue for Solr search for the (probable) fail.
The attached patch implements this test. Running it, though, I discovered a bug in the DB backend, too – for conditions with an inequality operator on single-valued fields, we don't take
NULLvalues into account properly. So fixed that, too. (Not really what I set out to test, but still nice find.)Comment #13
drunken monkeyWell, you probably shouldn't pass an empty array with
INorNOT IN, and it's definitely forbidden for(NOT) BETWEEN, but I'd say empty strings are fine for all scalar values (at least on string or fulltext fields).Comment #14
borisson_80 cols.
Only a small thing I'd say that needs fixing, otherwise RTBC, so setting to rtbc, provided you fix that small thing on commit :)
Comment #15
mkalkbrennerThe tests pass on my current version of the Solr backend.
From my point of view and for the Solr backend it's a difference between an empty value and no value. Therefor I extended the tests to cover "no value" as well.
Comment #16
borisson_Fixed #14
Comment #18
drunken monkeyThat's not necessary, we already have tests for that (see
regressionTest2136409()).NULLvalues are indeed something completely different, so no cause to also add them to this issue's regression tests.Applied Joris' interdiff to my patch from #12 and committed. Thanks again, everyone!