Problem:

Invalid argument supplied for foreach() service.inc:1638

SearchApiQueryFilter::getTags() can sometimes return NULL. That happens with Search API Sqaved Searches where SearchApiQueryFilter objects are serialized into the database. Old serialized objects have NULL on their tags property.

The method_exists() call also does not make sense in createFilterQueries() because the type hint to SearchApiQueryFilterInterface already enforces that the getTags() method is always there.

CommentFileSizeAuthor
#5 2837745-5.patch425 bytesdrunken monkey
#5 2837745-5--interdiff.txt461 bytesdrunken monkey
#3 2837745-3--do_not_return_null_tags.patch411 bytesdrunken monkey
#2 sapi-solr-2837745-2.patch625 bytesklausi

Comments

klausi created an issue. See original summary.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Active » Needs review
StatusFileSize
new sapi-solr-2837745-2.patch625 bytes

Patch.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Project: Search API Solr » Search API
Component: Code » Framework
StatusFileSize
new 2837745-3--do_not_return_null_tags.patch411 bytes

Thanks for reporting this issue! Seems we forgot to think about old serialized queries/filters when implementing this. Easy to miss, I guess.
However, instead of checking for an illegal return value in the Solr backend, I think we should just eliminate the root cause: the getTags() says it'll always return an array, so it should. Patch attached.

Regading removal of method_exists(): that's still needed in case someone updates the Solr module but has an outdated Search API module. "Don't do that!" is of course the right answer, but I think we both know that it'll still happen.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work

Makes sense!

+++ b/includes/query.inc
@@ -1071,7 +1071,8 @@ public function hasTag($tag) {
   public function &getTags() {
-    return $this->tags;
+    // Tags can sometimes be NULL for old serialized query filter objects.
+    return $this->tags ? $this->tags : array();

So the method returns by reference, but you return an array which is not a reference to $this->tags.

You should set $this->tags to the empty array first and then return that.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs work » Needs review
StatusFileSize
new 2837745-5--interdiff.txt461 bytes
new 2837745-5.patch425 bytes

Correct, of course – thanks for catching that!
Otherwise, would you say this is RTBC?

  • drunken monkey committed 7891a93 on 7.x-1.x 
    Issue #2837745 by drunken monkey, klausi: Fixed NULL tags on old...
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Fixed

Should be fine, so: committed.
Thanks again for reporting!

Status: Fixed » Closed (fixed)

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