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.

CommentFileSizeAuthor
#17 views_string_filter_2823963-17.patch1.74 KBAnonymous (not verified)
#12 interdiff-4-12.txt1.3 KBAnonymous (not verified)
#12 views_string_filter_2823963-12.patch1.78 KBAnonymous (not verified)
#5 views_string_filter_2823963-4.patch1.76 KBAnonymous (not verified)
#4 views_string_filter_2823963-4-test-only.patch1.31 KBAnonymous (not verified)
views-string-filter.patch467 bytesAndrej Galuf
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Andrej Galuf created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three for Drupal Association commented
Issue tags: -views +Needs tests

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

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three for Drupal Association commented

The patch for itself looks fine. Maybe we could add a comment explaining why we need this condition.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
views_string_filter_2823963-4-test-only.patch1.31 KB

Test only

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
views_string_filter_2823963-4.patch1.76 KB

Pacth have not comment explaining due to poor my English.

The last submitted patch, 4: views_string_filter_2823963-4-test-only.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Priority: Minor » Normal

And Exception via Filter does not look like Minor. It is at least a Normal like this (yeah, it is artful up).

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three for Drupal Association commented
Issue tags: -Needs tests

Thank 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?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

Exactly, 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

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented
Status: Needs review » Needs work

Bit of nitpicking:

  1. +++ b/core/modules/views/src/Plugin/views/filter/StringFilter.php
@@ -287,7 +287,7 @@ protected function opContainsWord($field) {
+    if (!$where || $where->count() == 0) {

    couldn't this just be if (!$where->count())) {
    $where is always set. 

  2. +++ b/core/modules/views/tests/src/Kernel/Handler/FilterStringTest.php
@@ -293,6 +293,43 @@ function testFilterStringWord() {
+    // Change the filtering

    missing . at the end
    EDIT: And maybe make it something like : Change the filtering to a sting containing only illegal characters. 

  3. +++ b/core/modules/views/tests/src/Kernel/Handler/FilterStringTest.php
@@ -293,6 +293,43 @@ function testFilterStringWord() {
+    $view->destroy();

    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.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review
FileSize
views_string_filter_2823963-12.patch1.78 KB
interdiff-4-12.txt1.3 KB

@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?

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented
Title: Views StringFilter breaks query on invalid strings » Views StringFilter using 'Contains any word' or 'Contains all words' breaks query on strings containing only characters that will be trimmed
Priority: Normal » Major
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: views_string_filter_2823963-12.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Reviewed & tested by the community

It looks like Mr. Bot just wants to up this topic :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: views_string_filter_2823963-12.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Reviewed & tested by the community
FileSize
views_string_filter_2823963-17.patch1.74 KB

reroll by instruction + manual replace this part:

+++ b/core/modules/views/tests/src/Kernel/Handler/FilterStringTest.php
@@ -293,6 +293,42 @@ function testFilterStringWord() {
       ),
     );

  • catch committed 2135a25 on 8.4.x 
    Issue #2823963 by vaplas, Andrej Galuf, dawehner: Views StringFilter...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Patch (to be ported) » Fixed

@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!

  • alexpott committed 0a44ce0 on 8.3.x authored by catch 
    Issue #2823963 by vaplas, Andrej Galuf, dawehner: Views StringFilter...

Status: Fixed » Closed (fixed)

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