Deprecate usage of Connection::getDriverClass for some classes, and use standard autoloading instead

Maybe we can still get on after ✨ Introduce database driver extensions and autoload database drivers' dependencies Fixed commit leaving aside, alas, 📌 Convert select query extenders to backend-overrideable services RTBC .

Let's make this issue a bit less boastful. The MR here helps a lot reducing boilerplate classes in database drivers extending from core ones.

@mondrake: Thank you for working on this.

Adding the same code to every database drivers Connnection class is a bit of an ugly solution. We are breaking the DRY principle. Would it be possible to put the code in trait? Or maybe another solution?

It’s not the SAME code actually. Each method instantiates a class either in the same namespace of the Connection class, or in the core lib, depending in the use imports at the beginning of each driver’s Connection class. In D11 we might change the core abstract Connection methods to instantiate the classes on Database\Query, and use default inheritance in drivers’ extending Connection classes. We can’t do that now without breaking BC. In any case, most of the classes even in core drivers DO extend from the core classes, so the driver specific methods will stay.

Going to put this in the committers queue. But if I'm understanding correctly this gives more control in the individual drivers?

Change record was clear too for how to update my own driver. Think getting in early gives more time for contrib modules to transition.

diff --git a/core/lib/Drupal/Core/Database/Connection.php b/core/lib/Drupal/Core/Database/Connection.php
index 7e405e1b1b..a2e19d1b1b 100644
--- a/core/lib/Drupal/Core/Database/Connection.php
+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -27,6 +27,13 @@
  */
 abstract class Connection {
 
+  /**
+   * The class for the query builder for SELECT statements.
+   *
+   * @var string
+   */
+  protected $selectClass = Select::class;
+
   /**
    * The database target this connection is for.
    *
@@ -990,8 +997,7 @@ public function exceptionHandler() {
    */
   public function select($table, $alias = NULL, array $options = []) {
     assert(is_string($alias) || $alias === NULL, 'The \'$alias\' argument to ' . __METHOD__ . '() must be a string or NULL');
-    $class = $this->getDriverClass('Select');
-    return new $class($this, $table, $alias, $options);
+    return new $this->selectClass($this, $table, $alias, $options);
   }
 
   /**
diff --git a/core/modules/mysql/src/Driver/Database/mysql/Connection.php b/core/modules/mysql/src/Driver/Database/mysql/Connection.php
index 73239a4536..b80ad9ec07 100644
--- a/core/modules/mysql/src/Driver/Database/mysql/Connection.php
+++ b/core/modules/mysql/src/Driver/Database/mysql/Connection.php
@@ -23,6 +23,11 @@
  */
 class Connection extends DatabaseConnection implements SupportsTemporaryTablesInterface {
 
+  /**
+   * {@inheritdoc}
+   */
+  protected $selectClass = Select::class;
+
   /**
    * Error code for "Unknown database" error.
    */

@mondrake I am not sure my other solution also works with static code analysis. A database driver only needs to set the class variable when it is overriding the class. This solution also works for database drivers that are extending another database driver. The problem with your solution is that every database driver Connnection class needs to add the "same" boilerplate code. What do you think?

@daffie IMHO we should avoid using properties to store the class whereabouts, and replace native autoloading with our magic. That's what protected $statementWrapperClass is doing, and I think we should also change that at some point.

I know this looks like adding boilerplate, but it's actually not. These are methods that are there just to create a proper instance of a class depending on the driver. In our current predicament, we can not leverage inheritance because of BC concerns. But once we can, there will be quite some cleanup possible - the same cleanup that it's possible with this MR in contrib drivers that extend from core ones and do not have to override a specific class (see the test drivers in the MR already).

Better than many words, I'll post here 'how' D11 could look like in this respect.

+1 for RTBC.

@mondrake: Thank you for explaining.

@daffie here's the idea per #28; Upsert and Schema are abstract classes in core, so every driver must implement its ones. But the rest of the classes we can inherit from the core Connection in D11, unless there are overrides.

@mondrake: Lets go with your solution. I think that in some point in the future, we should create a boilerplate module for a custom database driver for maintainer/creators of database driver modules.

Should we not deprecate getDriverClass() entirely here? Are there any other calls to it with other arguments?

Should we not deprecate getDriverClass() entirely here? Are there any other calls to it with other arguments?

Unfortunately not, 📌 Convert select query extenders to backend-overrideable services RTBC blocks that deprecation since Query\Select::extend() uses it.

Also #28/#30 imply that we might not make all those @todo methods abstract?

Yes, NW to adjust the @todo(s) - so we need to make schema and upsert abstract since the classes are abstract, all others should be implementations instantiating the corresponding Query\* base class.

Putting back in front of committer eyes.

Rebased.

Some tiny nitpicks/questions, and the Postgres tests had a random fail, but I have reviewed this twice now and think it is ready to go after this last round is responded to.

Here it comes, @longwave. Thanks

The changes look good to me.
Back to RTBC for me.
As long as the testbot is happy too.

Committed and pushed 97db62a138 to 11.x. Thanks!

mondrake → closed merge request !757

Thank you. Edited the CR to reflect the approach that was eventually committed.

Filed 📌 Deprecate usage of Connection::getDriverClass for 'Install\Tasks' Fixed for follow up.

Automatically closed - issue fixed for 2 weeks with no activity.