CONCAT_WS requires at least 3 arguments.

Patch which uses the CONCAT_WS expression, only when there are multiple fields to concatenate together.

I should probably change it to:

if ($count < 2)

because an empty field array makes just as little sense, and would probably yield an error with the existing code.

Testing a combine filter with no fields in the array.

1. Adding more comments is certainly a good idea. In fact, if you have any idea why the foreach loop exists I would love to hear it. To me it reads like the foreach loop adds an additional single space character to the array, but only between existing fields. The implode will turn the array into a comma separated string. It seems like it could be simplified by getting rid of the foreach loop and changing the implosion to:

expression = implode(', \’ \’,  ', $separated_fields);

Then this all is used as the argument list for CONCAT_WS, which adds a single space between each argument. Again, we can instead just use a triple-space as the separator...but then again, why are we using three spaces anyways? The whole reason for adding the single-space as an argument seems pointless.

2. The new test was to check if MySQL was fine executing a CONCAT_WS with NO fields, just a seperator. I was thinking it would fail. I have to figure out how everything should work given this fact. Should the whole expression be dropped when the filter contains no fields? The core test FilterCombineTest::testNonFieldsRow() uses only one field, so it will follow the new execution path while all the other tests in the class will use the old one. The new execution path will be covered.

I’ve reviewed the patch in #8 and successfully tested with sqlsrv.

I found an old issue for this that can probably be closed when this is.

@lendude, why not just use three spaces as a seperator between fields instead of adding in a single-space string between fields and then concatinating another single space. Is it just to ensure there are six (or four?) spaces between fields when one is NULL? And if so, why is that important?

@lendude
3 spaces makes sense in that regard, but the method of creating it is odd. Why is the function adding a single-space As a field, and then adding another in the CONCAT_WS. Why not just add three spaces between the actual fields?

Like this:

    if ($fields) {
       $count = count($fields);
      if ($count == 1) {
       $expression = $fields[0];
      }
      else {
        $expression = implode(', ', $fields);
        $expression = "CONCAT_WS('   ', $expression)";
       }
    }