Remove unnecessary classes from the database drivers

@daffie I support this change but I know that there are old issues that have argued that this is the way it is meant to be. I think we need to find and reference the arguments they made here

If the core classes must be implemented by each driver, why wouldn't they be abstract? That would be the correct way to dictate that behavior IMO, not some buried discussion thread from years ago.

So this issue is essentially a duplicate of #2461239: Database driver class fallback is broken and stubbed instead. In comment #8 in that issue the maintainer of the DB API at the time said

I think I follow what is being done here from the patch. However, this is a regression. Early in the D8 cycle we switched the child classes to always be required in order to simplify the code. I think I made that change as part of the namespace-ification.

I don't see a reason to go back here. The empty classes hurt no one, and the patch here introduces a while loop into the driver handling that seems completely unnecessary.

I agree with both @daffie and @Beakerboy - I not sure I agree with the decision taken early in the D8 cycle. And what @Beakerboy says is entirely correct:

why wouldn't they be abstract? That would be the correct way to dictate that behavior IMO

@daffie I think not. The two things are completed entangled and deserve to go together.

My conclusion is that @catch also does not like to have the empty driver classes.

This conclusion is correct :)

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -867,10 +874,37 @@ public function getDriverClass($class) {
          $driver_class = $this->connectionOptions['namespace'] . '\\' . $class;
          $this->driverClasses[$class] = class_exists($driver_class) ? $driver_class : $class;
   -      if ($this->driverClasses[$class] === 'Condition') {
   -        // @todo Deprecate the fallback for contrib and custom drivers in 9.1.x
   -        //   in https://www.drupal.org/project/drupal/issues/3120036.
   -        $this->driverClasses[$class] = Condition::class;
   +      switch ($this->driverClasses[$class]) {
   

   This logic can be more performant. We only need to do the switch if class_exists($driver_class) is FALSE.

2. +++ b/core/modules/system/tests/modules/database_statement_monitoring_test/src/LoggedStatementsTrait.php
   @@ -3,6 +3,11 @@
   @@ -53,8 +58,19 @@ public function getDriverClass($class) {
   
   @@ -53,8 +58,19 @@ public function getDriverClass($class) {
        if (class_exists($driver_class)) {
          return $driver_class;
        }
   -    elseif ($class == 'Condition') {
   -      return Condition::class;
   +    switch ($class) {
   +      case 'Condition':
   +        return Condition::class;
   +      case 'Delete':
   +        return Delete::class;
   +      case 'Merge':
   +        return Merge::class;
   +      case 'Select':
   +        return Select::class;
   +      case 'Transaction':
   +        return Transaction::class;
   +      case 'Update':
   +        return Update::class;
        }
        return $class;
      }
   

   We can change this whole function to be

    /**
      * {@inheritdoc}
      */
     public function getDriverClass($class) {
       static $fixed_namespace;
       if (!$fixed_namespace) {
         // Override because we've altered the namespace in
         // \Drupal\KernelTests\Core\Cache\EndOfTransactionQueriesTest::getDatabaseConnectionInfo()
         // to use the logging Connection classes. Set to a proper database driver
         // namespace.
         $this->connectionOptions['namespace'] = (new \ReflectionClass(get_parent_class($this)))->getNamespaceName();
         $fixed_namespace = TRUE;
       }
       return parent::getDriverClass($class);
     }
   

   That makes the implementation less special and easier to maintain.

Updating the patch as per #11.

Here is an updated patch. Kindly review.

Can't add an interdiff as the old patch was not applying.

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -871,11 +878,42 @@ protected function expandArguments(&$query, &$args) {
   +          case 'Upsert':
   +            $this->driverClasses[$class] = Upsert::class;
   +            break;
   +        }
   

   To maintain the old behaviour we need a default here too :)

   Something like

   default:
     $this->driverClasses[$class] = $class;
   

2. +++ b/core/modules/system/tests/modules/driver_test/src/Driver/Database/DrivertestNoCustomClassesMysql/Connection.php
   @@ -0,0 +1,19 @@
   +<?php
   +
   +namespace Drupal\driver_test\Driver\Database\DrivertestNoCustomClassesMysql;
   +
   +use Drupal\Core\Database\Driver\mysql\Connection as CoreConnection;
   +
   +/**
   + * MySQL test implementation of \Drupal\Core\Database\Connection.
   + */
   +class Connection extends CoreConnection {
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function driver() {
   +    return 'DrivertestNoCustomClassesMysql';
   +  }
   +
   +}
   

   I'm not convinced that adding this driver is really worthwhile - we can test one of the other test drivers just as well. If the class fallback code was not working everything would be broken. I think it might more important to test that a driver can override the core defaults.

3. +++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerNonDefaultDatabaseDriverTest.php
   @@ -33,10 +33,14 @@ protected function setUpSettings() {
   -    $elements = $this->xpath('//label[@for="edit-driver-drivertestmysql"]');
   -    $this->assertEqual(current($elements)->getText(), 'MySQL by the driver_test module');
   -    $elements = $this->xpath('//label[@for="edit-driver-drivertestpgsql"]');
   -    $this->assertEqual(current($elements)->getText(), 'PostgreSQL by the driver_test module');
   +    if ($driver == 'mysql') {
   +      $elements = $this->xpath('//label[@for="edit-driver-drivertestmysql"]');
   +      $this->assertEqual(current($elements)->getText(), 'MySQL by the driver_test module');
   +    }
   +    if ($driver == 'pgsql') {
   +      $elements = $this->xpath('//label[@for="edit-driver-drivertestpgsql"]');
   +      $this->assertEqual(current($elements)->getText(), 'PostgreSQL by the driver_test module');
   +    }
   

   Is this change actually necessary - I can't see why.

4. +++ b/core/tests/Drupal/KernelTests/Core/Database/ConnectionTest.php
   @@ -186,4 +186,49 @@ public function testCondition() {
   +    if (Database::getConnection()->driver() != 'mysql') {
   +      $this->markTestSkipped("This test only runs for MySQL");
   +    }
   

   How come we do this? As far as I can see this test is db driver independent.

Re #17.4 but say we removed mysql testing... unlikely yes... but this test should be independent of the DB driver. I agree we only need to test this once but that's the same for 99% of unit tests already. And we don't do that. That's not how our orthogonal tests of each DB driver work atm.

Also for #17.2 well my point is that

If the class fallback code was not working everything would be broken. I think it might more important to test that a driver can override the core defaults.

Plus we're missing test coverage of #17.1 - I thought this was important for SelectExtenders but looking at \Drupal\Core\Database\Query\Select::extend() the way in which we allow drivers to override select extenders is completely wrong and pre namespaces. Ho hum. We need an issue for that too :(

Oh compare \Drupal\Core\Database\Query\Select::extend() and \Drupal\Core\Database\Query\SelectExtender::extend() - that's an interesting difference. Anyhow looking for a class of $this->connectionOptions['namespace'] . '\\' . \Drupal\search\SearchQuery' . ; is amusing...

I wonder what actually is happening in the following code:

    $query = $this->database
      ->select('search_index', 'i')
      // Restrict the search to the current interface language.
      ->condition('i.langcode', $this->languageManager->getCurrentLanguage()->getId())
      ->extend(SearchQuery::class)
      ->extend(PagerSelectExtender::class);

from \Drupal\help_topics\Plugin\Search\HelpSearch::findResults().

Just minor random thoughts:

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -871,11 +880,50 @@ protected function expandArguments(&$query, &$args) {
   +        switch ($class) {
   +          case 'Condition':
   ...
   +          case 'Delete':
   ...
   +          case 'Insert':
   

   We could use an array lookup table here instead. It pairs nice with the Null coalescing operator. Although, this is just plainly for the "easier on the eye" reason.

2. +++ b/core/modules/system/tests/modules/database_statement_monitoring_test/src/LoggedStatementsTrait.php
   @@ -46,18 +44,16 @@ public function resetLoggedStatements() {
   +      $this->connectionOptions['namespace'] = (new \ReflectionClass(get_parent_class($this)))->getNamespaceName();
   +      $fixed_namespace = TRUE;
   

   This feels wrong. A getter shouldn't set a value. It is a test class though, so maybe it's fine.

3. +++ b/core/tests/fixtures/database_drivers/module/corefake/src/Driver/Database/corefakeWithAllCustomClasses/Condition.php
   @@ -0,0 +1,9 @@
   +class Condition extends QueryCondition {
   
   +++ b/core/tests/fixtures/database_drivers/module/corefake/src/Driver/Database/corefakeWithAllCustomClasses/Connection.php
   @@ -0,0 +1,14 @@
   +class Connection extends BaseConnection {
   
   +++ b/core/tests/fixtures/database_drivers/module/corefake/src/Driver/Database/corefakeWithAllCustomClasses/Select.php
   @@ -0,0 +1,9 @@
   +class Select extends QuerySelect {
   
   +++ b/core/tests/fixtures/database_drivers/module/corefake/src/Driver/Database/corefakeWithAllCustomClasses/TableSortExtender.php
   @@ -0,0 +1,9 @@
   +class TableSortExtender extends QueryTableSortExtender {
   

   Here we need docblocks. There are more occurences.

#25.1

Something like:

$lookup = [
	'Condition' => Condition:class,
	'Delete' => Delete::class,
	'Upsert' => Upsert::class,
];

$this->driverClasses[$class] = $lookup[$class] ?? $class;

return $this->driverClass;

This is just a preference (and why I only suggested it) I don't think we use this pattern that much in core either.

Let's leave it as is :)

#25.2 I'm not that bothered with the reflections parts per se, I'm just pointing out that our getter method is producing side-effects. This is generally a bad practise, in this case it's in a test, but I'll leave it to other reviewers to have their say if that's something that should be changed.

I missed that #24.2 was actually pointed out as a suggested change in earlier reviews, and as I was unsure myself, I feel ok to not pursue the discussion.

Interdiff in #25 looks good.

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -871,11 +880,50 @@ protected function expandArguments(&$query, &$args) {
   +          case 'PagerSelectExtender':
   +            $this->driverClasses[$class] = PagerSelectExtender::class;
   +            break;
   ...
   +          case 'TableSortExtender':
   +            $this->driverClasses[$class] = TableSortExtender::class;
   +            break;
   

   It never gets called with these. We shouldn't add these here. We need to open up a follow-up issue about select extenders and database overrides. There differences between \Drupal\Core\Database\Query\Select::extend() and \Drupal\Core\Database\Query\SelectExtender::extend() are problematic and the implementations are BOTH problematic!

   In the case of these classes the calls are always made with ->extend('Drupal\Core\Database\Query\PagerSelectExtender') or ->extend('Drupal\Core\Database\Query\TableSortExtender') so this represents new functionality that's not really supported and makes it look like things will work that don't.

2. +++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
   @@ -134,23 +134,176 @@ public function providerGetDriverClass() {
   +      // The following test cases are for BC reasons.
   

   This is not true. It's not BC - it's expected functionality.

Whilst checking want we call getDriverClass with discovered we need yet another follow-up for...
<code>
  /**
   * {@inheritdoc}
   */
  public function upsert($table, array $options = []) {
    // Use the (faster) native Upsert implementation for PostgreSQL >= 9.5.
    if (version_compare($this->version(), '9.5', '>=')) {
      $class = $this->getDriverClass('NativeUpsert');
    }
    else {
      $class = $this->getDriverClass('Upsert');
    }

    return new $class($this, $table, $options);
  }

There's no need for the NativeUpsert class anymore. It should become postgres's upsert class.

Interdiff looks good and follow ups has been created. Pre-Re-RTBC.

Fixing the issue summary and adding a change record.

I’d like to commit this - but it’s removing all the core db driver specific useless classes. Note we can’t deprecate them because they’ll be used by core. I see a couple of options here:

• Commit to 9.0.x with the removal and update the CR to reflect this - custom / contrib db drivers are rare and painful because of things like this.
• Add back the core classes add an @deprecated but not an @trigger_error() and commit to 9.1.x and file an issue to remove them in 10.0.x
• Add class aliases for all the deprecated classes and commit to 9.1.x and file an issue to remove this class aliases in 10.0.x (as an aside I think we should think about having a deprecated alias class loader than can alias classes and trigger a deprecation)

Any thoughts?

Given only two projects are affected let's remove the redundant classes in 9.0.x. The inability to properly deprecate them is not great, and we also can't do that for class_alias.

We should try to do this before RC though.

Added the removed classes to the issue summary.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -4,6 +4,13 @@
 
 use Drupal\Component\Assertion\Inspector;
 use Drupal\Core\Database\Query\Condition;
+use Drupal\Core\Database\Query\Delete;
+use Drupal\Core\Database\Query\Insert;
+use Drupal\Core\Database\Query\Merge;
+use Drupal\Core\Database\Query\Select;
+use Drupal\Core\Database\Query\Truncate;
+use Drupal\Core\Database\Query\Update;
+use Drupal\Core\Database\Query\Upsert;
 
 /**
  * Base Database API class.
@@ -871,11 +878,44 @@ protected function expandArguments(&$query, &$args) {

@@ -871,11 +878,44 @@ protected function expandArguments(&$query, &$args) {
   public function getDriverClass($class) {
     if (empty($this->driverClasses[$class])) {
       $driver_class = $this->connectionOptions['namespace'] . '\\' . $class;
-      $this->driverClasses[$class] = class_exists($driver_class) ? $driver_class : $class;
-      if ($this->driverClasses[$class] === 'Condition') {
-        // @todo Deprecate the fallback for contrib and custom drivers in 9.1.x
-        //   in https://www.drupal.org/project/drupal/issues/3120036.
-        $this->driverClasses[$class] = Condition::class;
+      if (class_exists($driver_class)) {
+        $this->driverClasses[$class] = $driver_class;
+      }
+      else {
+        switch ($class) {
+          case 'Condition':
+            $this->driverClasses[$class] = Condition::class;
+            break;
+          case 'Delete':
+            $this->driverClasses[$class] = Delete::class;
+            break;
+          case 'Insert':
+            $this->driverClasses[$class] = Insert::class;
+            break;
+          case 'Merge':
+            $this->driverClasses[$class] = Merge::class;
+            break;
+          case 'Schema':
+            $this->driverClasses[$class] = Schema::class;
+            break;
+          case 'Select':
+            $this->driverClasses[$class] = Select::class;
+            break;
+          case 'Transaction':
+            $this->driverClasses[$class] = Transaction::class;
+            break;
+          case 'Truncate':
+            $this->driverClasses[$class] = Truncate::class;
+            break;
+          case 'Update':
+            $this->driverClasses[$class] = Update::class;
+            break;
+          case 'Upsert':
+            $this->driverClasses[$class] = Upsert::class;
+            break;
+          default:
+            $this->driverClasses[$class] = $class;
+        }
       }
     }

wouldn't sth like

+      if (class_exists($driver_class)) {
+        $this->driverClasses[$class] = $driver_class;
+      }
+      else {
+        $this->driverClasses[$class] = __NAMESPACE__ . '\Query\' . $class;
+      }

be simpler (maybe with additional check that the class exists)?

Committed a11ed7a and pushed to 9.1.x. Thanks!

Also cherry-picked back to 9.0.x.

diff --git a/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
index 40c45f40a1..528e248b95 100644
--- a/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
+++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
@@ -241,32 +241,32 @@ public function providerGetDriverClass() {
       [
         'Drupal\Core\Database\Query\PagerSelectExtender',
         'Drupal\corefake\Driver\Database\corefakeWithAllCustomClasses',
-        'Drupal\Core\Database\Query\PagerSelectExtender'
+        'Drupal\Core\Database\Query\PagerSelectExtender',
       ],
       [
         '\Drupal\Core\Database\Query\PagerSelectExtender',
         'Drupal\corefake\Driver\Database\corefakeWithAllCustomClasses',
-        '\Drupal\Core\Database\Query\PagerSelectExtender'
+        '\Drupal\Core\Database\Query\PagerSelectExtender',
       ],
       [
         'Drupal\Core\Database\Query\TableSortExtender',
         'Drupal\corefake\Driver\Database\corefakeWithAllCustomClasses',
-        'Drupal\Core\Database\Query\TableSortExtender'
+        'Drupal\Core\Database\Query\TableSortExtender',
       ],
       [
         '\Drupal\Core\Database\Query\TableSortExtender',
         'Drupal\corefake\Driver\Database\corefakeWithAllCustomClasses',
-        '\Drupal\Core\Database\Query\TableSortExtender'
+        '\Drupal\Core\Database\Query\TableSortExtender',
       ],
       [
         'Drupal\search\SearchQuery',
         'Drupal\corefake\Driver\Database\corefakeWithAllCustomClasses',
-        'Drupal\search\SearchQuery'
+        'Drupal\search\SearchQuery',
       ],
       [
         '\Drupal\search\SearchQuery',
         'Drupal\corefake\Driver\Database\corefakeWithAllCustomClasses',
-        '\Drupal\search\SearchQuery'
+        '\Drupal\search\SearchQuery',
       ],
     ];
   }

Fixed the coding standards on commit.