Use case:
We create a view with a text filter using operator Contains any word or Contains all words. As a value we set a string composed exclusively of one or more following characters: ,?!();:-, in my case a test string of --.
If we search by this filter, we will receive a database exception.
SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')) AND (
with the query containing:
WHERE ((node_field_data.status = :db_condition_placeholder_0) AND ())
The bug originates in rows 266-285 of \Drupal\views\Plugin\views\filter\StringFilter - The value is checked for being empty in row 266, but gets preg_matched and trimmed afterwards, resulting in the above test string being empty.
The variable $where remains valid, so it passes row 285, but being empty, only adds an empty AND() to the query in row 291, resulting in the exception.
A tiny fix is attached.
Comment | File | Size | Author |
---|---|---|---|
#17 | views_string_filter_2823963-17.patch | 1.74 KB | Anonymous (not verified) |
#12 | interdiff-4-12.txt | 1.3 KB | Anonymous (not verified) |
#12 | views_string_filter_2823963-12.patch | 1.78 KB | Anonymous (not verified) |
#5 | views_string_filter_2823963-4.patch | 1.76 KB | Anonymous (not verified) |
#4 | views_string_filter_2823963-4-test-only.patch | 1.31 KB | Anonymous (not verified) |
Comments
Comment #2
dawehnerLet's ensure we have a test so that bug never appears again. I guess extending
\Drupal\Tests\views\Kernel\Handler\FilterStringTest
would be a good place to do so.Comment #3
dawehnerThe patch for itself looks fine. Maybe we could add a comment explaining why we need this condition.
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedTest only
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedPacth have not comment explaining due to poor my English.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedAnd Exception via Filter does not look like Minor. It is at least a Normal like this (yeah, it is artful up).
Comment #8
dawehnerThank you for your patch and test!
To be honest I would have expected, that the result would be an empty result, rather than all items. Is this just me?
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedExactly, I also felt the discomfort of this in the beginning. But on the other hand it makes sense:
condition without word == not condition
filter without conditions == not filter
not filter = all items
Comment #11
LendudeBit of nitpicking:
couldn't this just be if (!$where->count())) {
$where is always set.
missing . at the end
EDIT: And maybe make it something like : Change the filtering to a sting containing only illegal characters.
not needed at the end
I agree with the logic in #9 (but I also agree that it feels a bit odd). But I think changing the logic to make the View empty if an invalid sting is entered would also lead to unexpected situations. Like: I start typing with a space -> View returns empty -> I type a legal character after that -> View is filled -> ???. So I think there is no 'right' way to do this, so lets not touch the logic.
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commented@Lendude, thanks!
I totally agree that this behavior is questionable. But Fatal Error with exposed filter.. maybe we can fix it, and create follow-up issue about clear behavior of filter?
Comment #13
LendudeWe have a test we have a fix.
I don't feel we need a follow-up, the current logic is that a string containing only trimmable characters evaluates to an empty string and and empty string shows all results. I don't think changing that to finding no results would be less confusing, because that would still not be the result set users would expect (if it was, why would they search for it?).
But if anybody feels it needs more discussion, yeah please open a follow up. I agree with @vaplas that fixing the fatal should be done first in any case.
Bumping to major since this is a fatal that can be generated though normal use of the UI.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedIt looks like Mr. Bot just wants to up this topic :)
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedreroll by instruction + manual replace this part:
Comment #19
catchCommitted/pushed to 8.4.x. This should definitely go into 8.3.x and for me during the RC is OK since it's a one-line change + test coverage, but leaving to be ported for now.
Comment #20
alexpott@catch, @cielfen, @xjm and I decided that patch rules can apply to 8.3.x up unitl RC2 so this is eligible for backport.
Committed 0a44ce0 and pushed to 8.3.x. Thanks!