CONCAT_WS is not compatible with all database drivers

Edit: Never mind, I don't know what I'm talking about.

FWIW, the same applies to SQLite, but that's a bug in Drupal core: #2318191: [meta] Database tests fail on SQLite

A more generic approach would be to define a custom user function for MSSQL (in case MSSQL supports that). That's how we're solving similar compatibility issues with e.g. PostgreSQL in the core Database driver.

Applied patch to views, and verified that it works on MS SQL Server R2.

just to confirm that patch working fine for us
thanks :)

SQLite also does not support CONCAT_WS. In the SQLite driver there is a file called core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php and in that file there is a helper function called sqlFunctionConcatWs that fixes the problem for SQLite.

see this sandbox to get a module which fixes /admin/people filter for Drupal 8. I created this sandbox module because Drupal don't give enough love to Windows platform and patch may not even get commited for a long while. Will come back with a patch for those who want to fix it at the root level.

@audriusb: I will review your patch, although I do not have a Windows platform to test it on.

The current patch has specifics for the Microsoft SQL server and that database is not supported by core.

What I missed is that this is a D7 issue. The problem is that I know very little of D7, so I cannot do a really good review and I cannot help with a database agnostic implementation.

I do not know if the same problem exists for D8. If it does than it needs to be fixed with a service. The service takes over the SQL string generating part and the service needs to be overridable. The MS SQL server problem can then be fixed by overriding the original service. For an example see the patch made by @chx in https://www.drupal.org/node/2443679#comment-10157734.

Rerolled patch #12. It was failing to apply by composer.

        "patches": {
            "drupal/core": {
                "2009238-CONCAT_WS-not-compatible" : "https://www.drupal.org/files/issues/2009238-CONCAT_WS-not-compatible_0.patch"
            },
        },

The patch was not needed on UAT which had later version of MS SQL Server which supports CONCAT_WS but on PROD which has V13.x.x needed only CONCAT function , updating patch

A small nitpick for this patch:

+++ b/core/modules/views/src/Plugin/views/filter/Combine.php
@@ -84,8 +122,15 @@ public function query() {
-      $expression = implode(', ', $separated_fields);
...
+        $expression = implode(', ', $separated_fields);
...
+        $expression = implode(', ', $separated_fields);

This part of the patch can be removed. You are copying it unnecessary into the if statement.

Because this patch does something special for a specific database, I would like for the subsystem maintainer (@lendude) to have a look at proposed solution.

The subsystem maintainer for the views module (@Lendude) does not want to have database specific code in the views module.
Personally I agree completely with him. And I have an other solution for the problem. I got this solution from @chx. Big thanks to @chx.

If we create a module for SQLServer we can implement the hook to change the views filter plugin and change the class that it uses to implement the plugin.
Everything below here is pseudo code:

sqlsrv.module:

/**
 * Implements hook_views_plugins_filter_alter().
 */
function sqlsrv_views_plugins_filter_alter(array &$plugins) {
  $plugins['class'] = 'Drupal\\sqlsrv\\Combine';
}

Combine.php:

namespace Drupal\sqlsrv;

use Drupal\views\Plugin\views\filter\Combine as CoreCombine;

class Combine extends CoreCombine {


  public function query() {
    $this->view->_build('field');
    $fields = [];
    // Only add the fields if they have a proper field and table alias.
    foreach ($this->options['fields'] as $id) {
      // Overridden fields can lead to fields missing from a display that are
      // still set in the non-overridden combined filter.
      if (!isset($this->view->field[$id])) {
        // If fields are no longer available that are needed to filter by, make
        // sure no results are shown to prevent displaying more then intended.
        $this->view->build_info['fail'] = TRUE;
        continue;
      }
      $field = $this->view->field[$id];
      // Always add the table of the selected fields to be sure a table alias exists.
      $field->ensureMyTable();
      if (!empty($field->field_alias) && !empty($field->field_alias)) {
        $fields[] = "$field->tableAlias.$field->realField";
      }
    }
    if ($fields) {
      $count = count($fields);
      $separated_fields = [];
      foreach ($fields as $key => $field) {
        $separated_fields[] = $field;
        if ($key < $count - 1) {
          $separated_fields[] = "' '";
        }
      }
      $expression = implode(', ', $separated_fields);
      // Updated the next line for SQLServer
      $expression = "CONCAT(' ', $expression)";

      $info = $this->operators();
      if (!empty($info[$this->operator]['method'])) {
        $this->{$info[$this->operator]['method']}($expression);
      }
    }
  }

}

The problem that now remains is that database drivers now need module capabilities to make this solution work. See: #2846366: Improve Drupal's Database Abstraction Layer Extensibility and Capabilities.

@daffie #44 sounds like a great solution.

+++ b/core/modules/views/src/Plugin/views/filter/Combine.php
@@ -84,8 +122,15 @@ public function query() {
+      if ($this->connection->driver() == 'sqlsrv') {

We ideally don't want switches like this in core anyway, let alone for a database backend we don't officially support. So either do something DB agnostic (like #23), or go for #44.

The contrib datbase driver needs module like capabilities for the solution (in comment #44) that @lendude wants.

Not really, the sql server driver is a combination of a module + a driver, so we already have "module capabilities".

So you have two pieces of software that are together one logical unit. How does that work out with different versions of your software. The driver is of version X and the module is of Y. You can get in a world of troubles with that.
With themes we do the same. In essence it is a templates directory with module capabilities. Why can't we do the same for database drivers.

For my full support database driver for MongoDB, I need the my module to be installed as the first module, because I need to override the entity storage classes.

@david_garcia: Does my solution from comment #44 work for you?

I have tested #44 in combination with SQL Server, sqldrv module and drupalonwindows.com driver. I made a custom 'sqldrv_combine' module with the provided code.

When i open the user list from the admin interface the following error is shown:

Uncaught PHP Exception Drupal\Core\Database\DatabaseExceptionWrapper: "Exception in People[user_admin_people]: SQLSTATE[42000]: [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]'CONCAT_WS' is not a recognized built-in function name." at E:\IIS\www\***\core\modules\views\src\Plugin\views\query\Sql.php line 1528

So the solution from #44 makes no difference.

@Marceldeb: The solution from comment #44 is a main plan how to fix the problem. As I stated it is "pseudo code". The reason why the code fails is the code in the hook_views_plugins_filter_alter() is not correct. Again it is "pseudo code".
We are working on having DrupalCI testbot for custom database driver such as SQL-Server. After we have that then we can start working on issues like this one. You can follow the testbot progress for custom database driver at #3106299: Create testbots for the contrib database drivers for OracleDB, Microsoft SQL Server and MongoDB. Reviews on its sub-issues are very welcome. Untill that time it is best to the patch from comment #41.

@daffie: Thank you for the quick reply. I really liked the way to temporary solve this using a module.. leaving the core intact. I will patch and watch the progress on the other issue. Thanks!

Only three versions of SQL Server are officially supported by Microsoft, 2016, 2017, and 2019. Of these, only 2016 does not have native CONCAT_WS support. My plan for the sqlsrv driver was to intercept the sql statement and replace any calls to CONCAT_WS with an appropriate combination of STUFF() and COALESCE() when needed. This should be done in Condition::where(), Select::addExpression(), and Connection::query(), however, the hook approach in #44 looks like it might be a cleaner fix for the filter. Unfortunately, the BasixSyntaxTest.php file contains a static query with a CONCAT_WS, so a find/replace with a compatible function in Connection::query() is still needed.

If the core dev teams wants Drupal to be more successful, ensuring that the code is universal and can run on the widest range of databases is essential. For example, in this case, the filter seems to be applying a LIKE operation on a CONCAT_WS’d group of fields as a substitution for a series of OR conditions. This seems more “clever” than “useful”.

@Beakerboy: I get your frustration. I want those to have those same problems fixed and I want to have them fixed yesterday instead of today. The problem is however that we need to first fix some underlying problems, before we can fix the problem you have mentioned. The big underlying problem is that how the by core supported database drivers work is different then the contrib ones. In #3129043: Move core database drivers to modules of their own will a level playing field be created. After that issue we can start moving all those special exceptions for the by core supported database drivers to their modules. When we do that, the contrib database drivers can then use the same solution.

This should be done in Condition::where(), Select::addExpression(), and Connection::query()

To fix this we can replace those methods with methods that do the same only do not allow sql strings as input. Like condition() vs where(). My suggestion is to create a new issue to start replacing the method addExpression(). Create a new method addSumExpression($alias, array $fields = [], $value = NULL), create tests for it and then replace all instances in core with addExpression() and using the SUM aggregate. Do that for all aggregate operators and after the last one you can deprecate addExpression(). It is some/a lot of work, but it the only way we shall get rid of addExpression().

The #3129043: Move core database drivers to modules of their own is blocked by #3129534: Automatically enable the module that is providing the current database driver, which is blocked by #2664322: key_value table is only used by a core service but it depends on system install. I know those are not easy issues, but they are necessary and your help on them will be much appreciated.

When we keep at it we will get there!

@daffie, I’m just asking about this one case so we can get a better comment in the code explaining the rationelle.

I’ve been thinking about how to replace addExpression() as well. My thought is to make it like condition() where the method signature is expression($alias, $args, $type). The arguments could be fields or other expressions. That way if someone wanted to CONCAT() the result of a FOO(), but a database does not support FOO(), the driver could replace the inner expression and place that into the array, passing it along to compile().

But this is all outside the scope of this issue...checking things off, one at a time.

I’m tempted to close this as outdated. Like I stated above, only one database version does not support CONCAT_WS, and I as the only active dev on the sqlsrv driver module, have no intention of continuing to support legacy database versions.I have a plan in place to ensure that sql server 2016 will be able to execute queries that contain this function by providing an override in the driver. If no one has any objections, I’ll close this in 2 weeks.

Altough i don't really have a 'vote' here I agree we can close this issue. I have migrated to the latest SQL Express and i used the latest driver provided by the module maintained by @beakerboy. It runs like a charm now.

Status can be set to closed.