Views Combined fields filter with "Contains any word" or "Contains all words" operator results in an incorrect SQL query and a fatal error

Moving to the right queue.

Do you have an export of this View? This is not a standard View since it uses an expression and a subquery. Do you have some steps to reproduce this?

I'm experiencing the same behaviour. All the other filters I put on the view are working correctly but the combined field filter causes the same error.

Using Drupal 8.0.5.

To recreate it I just needed to create a view, add a few fields and then add an exposed combined field filter that searches on several of the fields. Also add a regular filter on a single field. The single field filter works fine by itself. If I put a search term into the the combined field filter it dies with the SQL error as vakorol reported.

Through a little testing I think I was able to narrow it down a little. When the operator on the combined filter is "Contains any word" then the filter causes the crash. If I set it to "is equal to" then the filter doesn't crash.

EDIT: "Contains" operator works as well. It seems to be the "Contains any word" operator that causes a problem for me. I have not tested the other operators.

Hope that helps.

"Contains" operator works as well. It seems to be the "Contains any word" operator that causes a problem for me. I have not tested the other operators.

@andrew.trebble Yup that was the bit that did it, thanks!

Here is a failing test for this.

The reason it fails is that the Combine filter doesn't override the opContainsWord() method, but haven't managed to get that to work yet.

Not sure if this is the most elegant way to fix this, but it's a fix.

Bumping to major since this results in a fatal error.

Patch in #8 worked for me. Thanks Lendude.

Nice!

+++ b/core/modules/views/src/Plugin/views/filter/Combine.php
@@ -137,6 +137,36 @@ protected function opContains($expression) {
+  protected function opContainsWord($expression) {

Needs docs, even if it's just @inheritdoc

Docs added. @inheritdoc wouldn't work since the parent method doesn't have any docs either (nor do any of the other ops methods, we should fix that...).

8.1.x patch should work for 8.2.x too.

Thank you @Lendude!!

1. +++ b/core/modules/views/src/Plugin/views/filter/Combine.php
   @@ -137,6 +137,42 @@ protected function opContains($expression) {
   +   * Filters by one or more words.
   +   *
   +   * By default opContainsWord uses add_where, that doesn't support complex
   +   * expressions.
   +   */
   +  protected function opContainsWord($expression) {
   

   Missing @param documentation

2. +++ b/core/modules/views/src/Plugin/views/filter/Combine.php
   @@ -137,6 +137,42 @@ protected function opContains($expression) {
   +        $this->query->addWhereExpression($group, "$expression LIKE $temp_placeholder", array($temp_placeholder => '%' . db_like($word) . '%'));
   

   db_link() is deprecated but there's no inject database to easily use so this looks okay to me.

3. +++ b/core/modules/views/src/Plugin/views/filter/Combine.php
   @@ -137,6 +137,42 @@ protected function opContains($expression) {
   +    preg_match_all('/ (-?)("[^"]+"|[^" ]+)/i', ' ' . $this->value, $matches, PREG_SET_ORDER);
   ...
   +      if ($match[2]{0} == '"') {
   +        $match[2] = substr($match[2], 1, -1);
   +        $phrase = TRUE;
   +      }
   

   Is all of this properly tested?

I'm also experiencing the same problem. The patch in #13 works for me. Thanks @Lendude.

15.1 Added.
15.2 Cool.
15.3 I updated the test to contain a 'phrase', so the preg_match_all() double quote logic is tested. Taking a closer look at it, it's unclear to me what the preg_split() is supposed to be doing anyway. Since the first preg_match_all() already splits on ' ', I don't see what the preg_split() is supposed to accomplish. Taking it out leaves the tests green, so unless somebody can think of a search sting to turn it red, lets leave it out.

8.1.x patch should apply to 8.2.x too.

Discussed with @xjm and @catch. We agree that this is major because this results in an exception through normal usage of views.

Forgot age... no change in the 8.0 patch, but reposting to keep everything together.

On an 8.1.x-dev system I applied the patch from #20. My view is only using the "Contains All Word" variety of the filter, but it seems to work as expected.

Had the same issue on 8.1.2. #20 fixed it.

1. +++ b/core/modules/views/src/Plugin/views/filter/Combine.php
   @@ -137,6 +137,39 @@ protected function opContains($expression) {
   +    preg_match_all('/ (-?)("[^"]+"|[^" ]+)/i', ' ' . $this->value, $matches, PREG_SET_ORDER);
   

   This needs a comment.

2. +++ b/core/modules/views/src/Plugin/views/filter/Combine.php
   @@ -137,6 +137,39 @@ protected function opContains($expression) {
   +      if ($match[2]{0} == '"') {
   +        $match[2] = substr($match[2], 1, -1);
   +      }
   

   Why not just trim?

@catch thanks for the feedback! So something like this?

+++ b/core/modules/views/src/Plugin/views/filter/Combine.php
@@ -157,11 +159,8 @@ protected function opContainsWord($expression) {
-      // Strip off phrase quotes
-      if ($match[2]{0} == '"') {
-        $match[2] = substr($match[2], 1, -1);
-      }
-      $word = trim($match[2], ',?!();:-');
+      // Clean up the user input and remove the sentence delimiters.
+      $word = trim($match[2], ',?!();:-"');

Nice improvement!

Committed/pushed to 8.2.x and cherry-picked to 8.1.x, thanks!

Gave review credit to andrew.trebble (post commit) for the clear steps to reproduce.

I think this issue may have broken postgres.

Testbot doesn't like patches on fixed issues now apparently. Uploaded at #2756595: test issue.

The relevant CI fail: https://www.drupal.org/pift-ci-job/346519

Confirmed; it fails on Postgres. I reverted the commit in both 8.2.x and 8.1.x.

Well that will teach me to add tests for other databases when messing with views query building.....thanks for the revert @xjm!

I know next to nothing about Postgres, so no idea how to fix this.

For people familiar with postgres but not views, this is the final query that is build by the test:

"SELECT views_test_data.age AS views_test_data_age, views_test_data.id AS id, views_test_data.name AS views_test_data_name, views_test_data.job AS views_test_data_job, views_test_data.id AS views_test_data_id 
FROM {views_test_data} views_test_data 
WHERE (( (CONCAT_WS(' ', views_test_data.name, ' ', views_test_data.job) LIKE :views_combine_0) OR (CONCAT_WS(' ', views_test_data.name, ' ', views_test_data.job) LIKE :views_combine_1) )) 
ORDER BY views_test_data_id ASC"

I assume the problem stems from using a CONCAT_WS inside a WHERE clause, but *shrug*

Bleh, Postgres is case sensitive, well at least I now know how to run tests on postgres locally.....

This passes locally on postgres now.

Its nice that the combined filter is able to filter something like this now.

1. +++ b/core/modules/views/src/Plugin/views/filter/Combine.php
   @@ -133,6 +133,38 @@ protected function opContains($expression) {
   +    // Match all words separated by spaces or sentences when encapsulated by
   +    // double quotes.
   +    preg_match_all('/ (-?)("[^"]+"|[^" ]+)/i', ' ' . $this->value, $matches, PREG_SET_ORDER);
   

   Nitpick: maybe add a comma before 'when', so its clear "when encapsulated by double words" is just for sequences.

2. +++ b/core/modules/views/src/Plugin/views/filter/Combine.php
   @@ -133,6 +133,38 @@ protected function opContains($expression) {
   +      $this->query->addWhereExpression($group, "$expression LIKE $temp_placeholder", array($temp_placeholder => '%' . db_like($word) . '%'));
   

   Potential follow-up: Replace db_like with Database::getConnection()->escapeLike($string) whereever possible, in views.

Fixed comment by removing 'when' altogether, I think this is even clearer then adding the comma.

opened follow-up #2784739: Fix PostgreSQL operator in views

now with an actual patch....

Nice!

Unrelated fails

The postgres driver maps LIKE to ILIKE for normal condition queries: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%...

We shouldn't introduce driver-specific logic here, but the behaviour should be the same IMO.

Having said that I see this a lot in StringFilter and Combine, so might need to be a follow-up bug report to look at all of those queries?

I updated the follow-up #2784739: Fix PostgreSQL operator in views to also fix the hardcoded LIKE and NOT LIKE instances. Since both mapConditionOperator and escapeLike would require the connection it's probably easiest to tackle both at the same time.

@catch would that address your concerns?

Did you mean that?
addresses #40.2 and revert changes from #39

I think this addresses the concerns catch had in #47, in the follow-up #2784739: Fix PostgreSQL operator in views we should update everything to use this pattern.

Tests are passing on all databases, so this looks good to me. Nice!

• random error with PHP 5.5 & SQLite 3.8 (added to re-test)
• updated follow-up

Why do we use a complex expression

Because I just copy/pasted it from \Drupal\views\Plugin\views\filter\StringFilter::opContainsWord and figured "if it ain't broke....."

I think from a sanity point of view they should use the same regex and changing StringFilter is out of scope here, maybe open a follow-up if this lands and update both?

I think it is in scope to ensure that \Drupal\views\Plugin\views\filter\Combine::opContainsWord() uses the same regex as \Drupal\views\Plugin\views\filter\StringFilter::opContainsWord() because we're implementing an override of that method. The simplest thing to do is just add the regex as a class constant to StringFilter and then use it in both methods.

Everything else looks great and it is fantastic to be building up our views test coverage.

@vaplas Lets not add methods, that would make this an API change and push this to 8.3.x (or 8.4.x). I like the idea, I'm just not thrilled about the impact it has on fixing this. Lets just get this working.

I'd say just go with what @alexpott suggested, add the regex as a class contant to \Drupal\views\Plugin\views\filter\StringFilter and use that. Clean, concise, no BC breakage.

addresses #59

Feedback from #59 has been addressed, still passes on all supported databases, nice!

Committed/pushed to 8.3.x and cherry-picked to 8.2.x, thanks!