SearchApiViewsHandlerFilterTaxonomyTerm::ids_to_strings() applies array_filter() over the passed $ids array while assigning them as condition values.

The only reason this hasn't caused problems so far is that every call to ids_to_strings() checks if the value is empty before calling.

CommentFileSizeAuthor
#3 2905445-3--prevent_empty_IN_condition.patch784 bytesdrunken monkey
#3 2905445-3--prevent_empty_IN_condition--interdiff.txt649 bytesdrunken monkey
#2 search_api-ids_to_strings-invalid-query-2905445-2.patch816 bytesciss

Comments

ciss created an issue. See original summary.

ciss’s picture

ciss commented
Status: Active » Needs review
StatusFileSize
new search_api-ids_to_strings-invalid-query-2905445-2.patch816 bytes
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
StatusFileSize
new 2905445-3--prevent_empty_IN_condition--interdiff.txt649 bytes
new 2905445-3--prevent_empty_IN_condition.patch784 bytes

Thanks a lot for reporting this! Good catch!
However, I'm pretty sure we want to return an empty string, not an empty array, in this case?
Otherwise, looks very good! If you could please confirm this still works for you, I can commit it.

ciss’s picture

ciss commented
Status: Needs review » Reviewed & tested by the community

However, I'm pretty sure we want to return an empty string, not an empty array, in this case?

You're absolutely right. I missed both the implode() call and the @return doc in the parent docblock.

If you could please confirm this still works for you, I can commit it.

I only came across this while altering/overriding exposed form widgets. I currently can't reproduce the edge case that led to problems in that method (and ultimately didn't include the patch).

IMO the patch looks sound and is ready to go.

ciss’s picture

ciss commented
Status: Reviewed & tested by the community » Needs review

Spoke too soon. Had the patch applied locally but not yet added to our make. Will test.

ciss’s picture

ciss commented
Status: Needs review » Reviewed & tested by the community

... Definitely not able to reproduce. As mentioned in the summary, calls to ids_to_strings() are usually guarded against passing of empty values.
I believe I encounted this while testing the latest patch in #1783746: Add support for "between" operator (which we ultimately removed again). Back to RTBC.

  • drunken monkey committed f838690 on 7.x-1.x authored by ciss 
    Issue #2905445 by ciss, drunken monkey: Fixed error handling in Views...
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Fixed

Good to hear, thanks for testing and reviewing!
Committed. Thanks again!

Status: Fixed » Closed (fixed)

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