Views combined filters add redundant separators in CONCAT_WS()

Looks really nice, especially given that we have dedicated test coverage for this filter.

Added an issue summary and a beta evaluation. I think though that this is more of a task than a bug fix, don't you think so?

Nice, though with no longer adding the quotes, are we maybe introducing/having existing security problems?
Given that fields are just the fields which appears in the query:

        $fields[] = "$field->tableAlias.$field->realField";

I would say no.

Oh right.

+++ b/core/modules/views/src/Tests/Handler/FilterCombineTest.php
@@ -81,6 +81,46 @@ public function testFilterCombineContains() {
 
+  public function testFilterCombineContainsPhrase() {

We're missing a method doc block saying what this is testing.

@MrHaroldA - well that's not obeying our standards. Not everything in core does :( - But we shouldn't add new violations if we spot them in new code.

Sounds good - should be "Tests..." though... maybe Tests filtering result using a phrase that matches combined fields. is better

Adressed from alex got adressed

Committed 8c30ce9 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

I've rolled this back due to it causing #2411703: Random fail in Drupal\user\Tests\UserAdminTest

We need to be able to configure whether to search across concatenated fields or search using an OR.

I always figured that the 3 spaces were a delimiter between fields and there for a purpose.

+++ b/core/modules/views/src/Tests/Handler/FilterCombineTest.php
@@ -82,6 +82,49 @@ public function testFilterCombineContains() {
+        'value' => 'ohn Si',

I would actually expect this to return no results (as the current HEAD does), because none of my fields contain this value, it is only valid as an byproduct of the way we search across multiple fields.

I always thought the spaces were a bit of a weird choice as delimiters, but I would be against removing them.

Actually, thinking about this more, spaces make perfect sense as delimiters because they are the only characters that won't give false positive results when using the 'word' operators that explode on spaces.

The spaces need to stay in.

This will generate a query containing double spaces

It will create a query with triple spaces actually, because the inserted space will also be concatenated with the space separators.

which prevents you from searching across multiple fields

Which I feel is exactly the point when using the 'contains' operator. You don't want to find any results that only exist because we happen to search across multiple fields using a trick that concatenates the values (even if you did want to enable this kind of multi-field search, it would only work for fields that just happen to be concatenated in sequence, a sequence unknown to any end-user).

When you want to search for multiple terms across multiple fields, you use the 'contains any word'/'contains all words' operators, not the 'contains' operator. Of course that doesn't currently work #2664530: Views Combined fields filter with "Contains any word" or "Contains all words" operator results in an incorrect SQL query and a fatal error, but that's not the point :)

I'm not saying the end result will always be bad, but like the example in the test 'value' => 'ohn Si' this will mostly lead to 'what the ??' results for end users. Finding rows where none of the fields actually match the searched for data.

If you happen to have a View that puts your first name and current place of residence in sequence and you live in Amsterdam, then your search will also return true, but it's not a result you were actually looking for or expecting.

The current implementation gives clear results, 'the search string needs to be in one field', going with one space would lead to unpredictable results. Your use-case can also be accomplished using the other operators (once we get them to work....., which reminds me, I should look at that other issue again :)

Closing this now, this works as designed.

I'm extending this Combine class to work with integers and other fields (like datetime). Having spaces in between each field on a CONCAT_WS fails horribly as the query only checks the first field before an errant space separator.

CONCAT_WS(' ', field_a, field_b) -> works as expected and is a properly formed MYSQL CONCAT_WS function call
CONCAT_WS(' ', field_a, ' ', field_b) -> does not work (even if matches are found in field_b) due to bad function call (presence of blank fields)

I assume however that there must be some magic the empty fields (space fields), performs when searching against string-based field values and that is why this patch was rejected?

If the user used three spaces in his search string, the partial match across sequential fields is back.

Nope, because multiple sequential spaces get stripped from the search string. It is impossible to search across fields now, the triple space separator is the only way to do this, because it is impossible to get into your search string.

@Lendude Actually for me the filter gives results with three spaces. I mistakenly used the combined filter like other people to combine first_name last_name field then I found this issue. So tested with three spaces and got result with it. I used it as simple exposed filter.