Steps to reproduce:

  1. create a facet (e.g. content type)
  2. filter on a value
  3. 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 :-)

Comments

mpp created an issue. See original summary.

mpp’s picture

Status: Active » Needs review
berdir’s picture

I'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.

mkalkbrenner’s picture

Project: Search API Solr » Facets
Assigned: mpp » Unassigned
Priority: Minor » Normal
Status: Needs review » Active

OK, 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 ...

mpp’s picture

StatusFileSize
new751 bytes

Hey what about the typo :-)

borisson_’s picture

Issue tags: +Needs tests

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.

borisson_’s picture

Status: Active » Needs review
StatusFileSize
new809 bytes

This should fix that.

mpp’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, thanks!

drunken monkey’s picture

I 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.

mkalkbrenner’s picture

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?

I 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.

borisson_’s picture

So I guess this can be moved to the search api queue and I don't need to commit #7?

drunken monkey’s picture

Title: Unsupported operand types in SearchApiSolrBackend->reduceFilterQueries() » Add backend tests for empty value conditions
Project: Facets » Search API
Component: Code » Tests
Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs tests
StatusFileSize
new2.82 KB

Yes, 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 NULL values into account properly. So fixed that, too. (Not really what I set out to test, but still nice find.)

drunken monkey’s picture

I guess that at least an empty value isn't allowed for all operators.

Well, you probably shouldn't pass an empty array with IN or NOT 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).

borisson_’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/modules/search_api_db/src/Plugin/search_api/backend/Database.php
@@ -2112,6 +2112,14 @@ protected function createDbCondition(ConditionGroupInterface $conditions, array
+            // Since SQL never returns TRUE for comparison with NULL values, we need
+            // to include "OR field IS NULL" explicitly for some operators.

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 :)

mkalkbrenner’s picture

Status: Reviewed & tested by the community » Needs review
StatusFileSize
new1.63 KB
new3.64 KB

The 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.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new1018 bytes
new3.64 KB

Fixed #14

  • drunken monkey committed ee0c21f on 8.x-1.x
    Issue #2767609 by drunken monkey, borisson_: Added backend tests for...
drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

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.

That's not necessary, we already have tests for that (see regressionTest2136409()). NULL values 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!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.