Testing framework does not work with contributed database drivers

Discussed with @catch and @xjm - we agreed that the that part of this issue about running tests on contributed database drivers is a major bug.

1. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -489,6 +489,15 @@ public static function convertDbUrlToConnectionInfo($url, $root) {
   +      if (isset($parts['namespace'])) {
   +        $database['namespace'] = $parts['namespace'];
   
   @@ -525,6 +534,9 @@ public static function getConnectionInfoAsUrl($key = 'default') {
   +      $db_url .= '?namespace=' . $db_info['default']['namespace'];
   

   This is problematic... since namespace is SqlServer thing - other databases could have other requirements. Actually I think the bug here is that we should call the method on the selected database driver and not on the abstract base class. Then each db can do it's own thing.

2. +++ b/core/modules/simpletest/simpletest.module
   @@ -184,7 +184,7 @@ function simpletest_run_phpunit_tests($test_id, array $unescaped_test_classnames
   -      'test_id' => '1',
   +      'test_id' => $test_id,
   

   I guess this is an unrelated change to help debugging? If so can this be filed as a separate issue.

3. +++ b/core/modules/simpletest/simpletest.module
   @@ -316,7 +316,7 @@ function simpletest_phpunit_command() {
   -    $phpunit_bin = escapeshellarg($php) . ' -f ' . escapeshellarg($vendor_dir . '/phpunit/phpunit/composer/bin/phpunit') . ' --';
   +    $phpunit_bin = escapeshellarg($php) . ' -f ' . escapeshellarg($vendor_dir . '/phpunit/phpunit/phpunit') . ' --';
   

   This should be in a separate issue. Given that phpunit can be run from the command line I think this is a just a normal bug.

So part of #11.1 is wrong - namespace is not an SqlServer concept - it is the namespace of the driver class - which is a Drupal concept. But the point still stands. We need to be able to defer to the real database driver class in these functions.

Here's a patch that allows each database driver to do custom stuff to the URI <-> connection options conversions. And because SQLite is different in core it uses this.

Fixed up the test fails.

A quick review:

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -6,6 +6,7 @@
    namespace Drupal\Core\Database;
   +use GuzzleHttp\Psr7\Uri;
   

   Nitpick: No black line between the two.

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1433,4 +1434,73 @@ public function __sleep() {
   +   * This method can be overridden in order to customise how a URI is converted.
   ...
   +   * This method can be overridden in order to customise how connection options
   +   * are converted.
   

   Is this necessary? Basic object oriented coding stuff.

3. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1433,4 +1434,73 @@ public function __sleep() {
   +   * @internal
   +   *   This method should not be called. Use
   +   *   \Drupal\Core\Database\Database::convertDbUrlToConnectionInfo() instead.
   ...
   +  public static function convertDbUrlToConnectionInfoHelper(Uri $uri, $root, array $connection_options) {
   ...
   +   * @internal
   +   *   This method should not be called. Use
   +   *   \Drupal\Core\Database\Database::getConnectionInfoAsUrl() instead.
   ...
   +  public static function getConnectionInfoAsUrlHelper(array $connection_options, Uri $uri) {
   

   I see the problem. But I do think that internal function should be protected.

4. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1433,4 +1434,73 @@ public function __sleep() {
   +  public static function convertDbUrlToConnectionInfoHelper(Uri $uri, $root, array $connection_options) {
   ...
   +  public static function getConnectionInfoAsUrlHelper(array $connection_options, Uri $uri) {
   
   +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -415,4 +416,30 @@ public function getFullQualifiedTableName($table) {
   +  public static function convertDbUrlToConnectionInfoHelper(Uri $uri, $root, array $database) {
   ...
   +  public static function getConnectionInfoAsUrlHelper(array $connection_options, Uri $uri) {
   

   The class GuzzleHttp\Psr7\Uri implements the interface Psr\Http\Message\UriInterface. Should we not use the interface for parameter typehinting.

5. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1433,4 +1434,73 @@ public function __sleep() {
   +    $user = '';
   +    if ($connection_options['username']) {
   +      $user = $connection_options['username'];
   +    }
   +    $password = '';
   +    if ($connection_options['password']) {
   +      $password = $connection_options['password'];
   +    }
   

   Can we rewrite this to:

   $user = $connection_options['username'] ?: ‘’;
   $password = $connection_options[‘password’] ?: ‘’;
   

6. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -460,36 +457,33 @@ public static function ignoreTarget($key, $target) {
   +    if (empty($uri->getHost()) || empty($uri->getHost()) || empty($uri->getPath())) {
   

   There are two getHost()'s. One should be getSchema().

7. +++ b/core/tests/Drupal/Tests/Core/Database/DatabaseTest.php
   @@ -0,0 +1,176 @@
   +  public function testNoConnectionInfo() {
   

   There is no docblock and no @covers data.

8. +++ b/core/tests/Drupal/Tests/Core/Database/DatabaseTest.php
   @@ -0,0 +1,176 @@
   +
   +/**
   + * @file
   + * Contains \Drupal\Tests\Core\Database\DatabaseTest.
   + */
   

   Nitpick: We do not do this any more.

9. +++ b/core/tests/Drupal/Tests/Core/Database/DatabaseTest.php
   @@ -0,0 +1,176 @@
   \ No newline at end of file
   

   Nitpick: No new line.

I am working on rerolling patch

I fixed the issues according to comment #19 except point no 2 and 3 , need more explanation about that.

Thanks

Thank you for contributing the patch, @Pradnya Pingat.

The patch in #22 makes many coding standard changes. You may want to confirm your IDE or text editor is configured for the Drupal coding standards. The child pages linked at the bottom of the Development tools page may help as well.

Rewrite #22 fixing coding standard issues. I only fix those added by #22.

I think a big clean should be done on all this files but it's not the purpose of this issue.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1433,4 +1434,73 @@ public function __sleep() {
+   * This method can be overridden in order to customise how a URI is converted.
...
+   * This method can be overridden in order to customise how connection options
+   * are converted.
Is this necessary? Basic object oriented coding stuff.

I agree with #19 2., so i remove this part of comment.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1433,4 +1434,73 @@ public function __sleep() {
+   * @internal
+   *   This method should not be called. Use
+   *   \Drupal\Core\Database\Database::convertDbUrlToConnectionInfo() instead.
...
+  public static function convertDbUrlToConnectionInfoHelper(Uri $uri, $root, array $connection_options) {
...
+   * @internal
+   *   This method should not be called. Use
+   *   \Drupal\Core\Database\Database::getConnectionInfoAsUrl() instead.
...
+  public static function getConnectionInfoAsUrlHelper(array $connection_options, Uri $uri) {
I see the problem. But I do think that internal function should be protected.

For #19 3. I understand why alexpott put this @internal comment and i think we should keep it, so contributor know this method should not be called directly.

I cancel test for interdiff. Sorry for that

It looks good. Got 2 points left:

1. In the patch from comment #15 is the file core/tests/Drupal/Tests/Core/Database/Stub/StubConnection.php renamed to core/tests/Drupal/Tests/Core/Database/Stub/Connection.php. Why is that not do?
2. All my remarks from comment #19 are addressed except for #19.4.

I only can do a code review because I have no windows PC.

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -455,36 +452,33 @@ public static function ignoreTarget($key, $target) {
+    $connection_opptions = array(
...
+    $driver_class = static::getDatabaseConnectionClass($connection_opptions['driver'], $connection_opptions['namespace']);
...
+    return $driver_class::convertDbUrlToConnectionInfoHelper($uri, $root, $connection_opptions);

Error typing, $connection_opptions should be $connection_options.

In the patch from comment #15 is the file core/tests/Drupal/Tests/Core/Database/Stub/StubConnection.php renamed to core/tests/Drupal/Tests/Core/Database/Stub/Connection.php. Why is that not do?
All my remarks from comment #19 are addressed except for #19.4.

Good question, i rename it, but it seems git diff make them separated as delete and create.
Interdiff make some strange things to. It the first time i use interdiff, i usually use git diff between to patched branchs, but i cannot apply alexpott patch. May be i miss something.

4.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1433,4 +1434,73 @@ public function __sleep() {
+  public static function convertDbUrlToConnectionInfoHelper(Uri $uri, $root, array $connection_options) {
...
+  public static function getConnectionInfoAsUrlHelper(array $connection_options, Uri $uri) {

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
@@ -415,4 +416,30 @@ public function getFullQualifiedTableName($table) {
+  public static function convertDbUrlToConnectionInfoHelper(Uri $uri, $root, array $database) {
...
+  public static function getConnectionInfoAsUrlHelper(array $connection_options, Uri $uri) {

The class GuzzleHttp\Psr7\Uri implements the interface Psr\Http\Message\UriInterface. Should we not use the interface for parameter typehinting.

I don't understand what you want to do with Interface.

@GoZ: I think you get the git diff rename with the commend git diff -C -M or git diff -C -M HEAD. And a good find with connection_opption.

In Drupal we like to use an interface for typehinting, so that we can change the implementing class with an other one without having to do an API change. We now have only to make sure that the replacing class implements the interface (Psr\Http\Message\UriInterface).
We would like to use the class GuzzleHttp\Psr7\Uri. And if we look at that class we see that that class is implementing the interface Psr\Http\Message\UriInterface. So the code change that I am suggesting is:

At the top of the file we add: use Psr\Http\Message\UriInterface;

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1425,4 +1427,62 @@ public function __sleep() {
-  public static function convertDbUrlToConnectionInfoHelper(Uri $uri, $root, array $connection_options) {
+  public static function convertDbUrlToConnectionInfoHelper(UriInterface $uri, $root, array $connection_options) {
...
-  public static function getConnectionInfoAsUrlHelper(array $connection_options, Uri $uri) {
+  public static function getConnectionInfoAsUrlHelper(array $connection_options, UriInterface $uri) {

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
@@ -415,4 +416,30 @@ public function getFullQualifiedTableName($table) {
-  public static function convertDbUrlToConnectionInfoHelper(Uri $uri, $root, array $database) {
+  public static function convertDbUrlToConnectionInfoHelper(UriInterface $uri, $root, array $database) {
...
-  public static function getConnectionInfoAsUrlHelper(array $connection_options, Uri $uri) {
+  public static function getConnectionInfoAsUrlHelper(array $connection_options, UriInterface $uri) {

Trying to figure out why tests fail, here is what i found :

Test fail in KernelTestBaseTest::testGetDatabaseConnectionInfoWithOutManualSetDbUrl() comparing prefix database connection.

  /**
   * @covers ::getDatabaseConnectionInfo
   */
  public function testGetDatabaseConnectionInfoWithOutManualSetDbUrl() {
    $options = $this->container->get('database')->getConnectionOptions();
    $this->assertSame($this->databasePrefix, $options['prefix']['default']);
  }

In this test, default prefix from connectionOptions is composed of two concatenated prefixes. Example : simpletest123456simpletest987654. This concatenation is made by KernelTestBase::getDatabaseConnectionInfo().

  /**
   * Returns the Database connection info to be used for this test.
   *
   * This method only exists for tests of the Database component itself, because
   * they require multiple database connections. Each SQLite :memory: connection
   * creates a new/separate database in memory. A shared-memory SQLite file URI
   * triggers PHP open_basedir/allow_url_fopen/allow_url_include restrictions.
   * Due to that, Database tests are running against a SQLite database that is
   * located in an actual file in the system's temporary directory.
   *
   * Other tests should not override this method.
   *
   * @return array
   *   A Database connection info array.
   *
   * @internal
   */
  protected function getDatabaseConnectionInfo() {
    // If the test is run with argument dburl then use it.
    $db_url = getenv('SIMPLETEST_DB');
    if (empty($db_url)) {
      throw new \Exception('There is no database connection so no tests can be run. You must provide a SIMPLETEST_DB environment variable to run PHPUnit based functional tests outside of run-tests.sh. See https://www.drupal.org/node/2116263#skipped-tests for more information.');
    }
    else {
      $database = Database::convertDbUrlToConnectionInfo($db_url, $this->root);
      Database::addConnectionInfo('default', 'default', $database);
    }

    // Clone the current connection and replace the current prefix.
    $connection_info = Database::getConnectionInfo('default');
    if (!empty($connection_info)) {
      Database::renameConnection('default', 'simpletest_original_default');
      foreach ($connection_info as $target => $value) {
        // Replace the full table prefix definition to ensure that no table
        // prefixes of the test runner leak into the test.
        $connection_info[$target]['prefix'] = array(
          'default' => $value['prefix']['default'] . $this->databasePrefix,
        );
      }
    }
    return $connection_info;
  }

Before this patch, in KernelTestBase::getDatabaseConnectionInfo(), default prefix values was empty. It was empty because Database::convertDbUrlToConnectionInfo() never set prefix (get it in $info variable but never put it in $database).

Before patch:

  public static function convertDbUrlToConnectionInfo($url, $root) {
    $info = parse_url($url);
    if (!isset($info['scheme'], $info['host'], $info['path'])) {
      throw new \InvalidArgumentException('Minimum requirement: driver://host/database');
    }
    $info += array(
      'user' => '',
      'pass' => '',
      'fragment' => '',
    );

    // A SQLite database path with two leading slashes indicates a system path.
    // Otherwise the path is relative to the Drupal root.
    if ($info['path'][0] === '/') {
      $info['path'] = substr($info['path'], 1);
    }
    if ($info['scheme'] === 'sqlite' && $info['path'][0] !== '/') {
      $info['path'] = $root . '/' . $info['path'];
    }

    $database = array(
      'driver' => $info['scheme'],
      'username' => $info['user'],
      'password' => $info['pass'],
      'host' => $info['host'],
      'database' => $info['path'],
    );
    if (isset($info['port'])) {
      $database['port'] = $info['port'];
    }
    return $database;
  }

With the patch, prefix is setted.
After patch:

  public static function convertDbUrlToConnectionInfo($url, $root) {
    $uri = new Uri($url);
    if (empty($uri->getHost()) || empty($uri->getScheme()) || empty($uri->getPath())) {
      throw new \InvalidArgumentException('Minimum requirement: driver://host/database');
    }

    // Discover if the URL has a driver namespace.
    $parts = array();
    parse_str($uri->getQuery(), $parts);
    $namespace = isset($parts['namespace']) ? $parts['namespace'] : '';

    $fragment = $uri->getFragment();
    $connection_options = array(
      'driver' => $uri->getScheme(),
      'host' => $uri->getHost(),
      // Strip the first leading slash of the path to get the database name.
      // Note that additional leading slashes have meaning for some database
      // drivers.
      'database' => substr($uri->getPath(), 1),
      'prefix' => $fragment ?: NULL,
      'namespace' => $namespace,
    );

    $driver_class = static::getDatabaseConnectionClass($connection_options['driver'], $connection_options['namespace']);
    if (!class_exists($driver_class)) {
      throw new \RuntimeException("Can not convert $url to a database connection, class $driver_class does not exist");
    }
    return $driver_class::convertDbUrlToConnectionInfoHelper($uri, $root, $connection_options);
  }

We should see this issue with someone who implemented KernelTestBase and Database class (see #2232861: Create BrowserTestBase for web-testing on top of Mink and #2304461: KernelTestBaseTNG™) or with someone who can tell if:

1. Database::convertDbUrlToConnectionInfo() should return prefix, so tests are wrong.
2. Database::convertDbUrlToConnectionInfo() should not return prefix, so tests are right and this patch should remove prefix from this method.

+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
@@ -437,4 +438,30 @@ public function getFullQualifiedTableName($table) {
+      // If the database is a absolute path add an additional leading slash to

Just a little thing in comments, replace 'a absolute path' by 'an absolute path'.
Sorry for this insignificant review :/

Everything else looks OK. Good job david_garcia

@david_garcia, can you cancel test on interdiff file please ?

I miss the document comment. You are right

Re-launch tests for #49 fail. Test fails due to random javascript fail see #2837676: Provide a better way to validate all javascript activity is completed

Tests fail with SQLite

No longer appears to require reroll - tag removed.

@david_garcia Can you add interdiff please so we can see difference between both patchs ? thanks

The patch looks good. I do have some remarks:

1. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseTest.php
   @@ -48,6 +48,9 @@ public function testBootEnvironment() {
   +    if (Database::getConnection()->driver() == 'sqlite') {
   +      $this->markTestSkipped('SQLITE modifies the prefixes so we cannot effectively test it');
   +    }
   

   Is this really necessary! With your comment in #59 can we try the patch without this added.

2. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -498,29 +492,64 @@ public static function convertDbUrlToConnectionInfo($url, $root) {
   +    $uri = new Uri($db_info['default']['driver'] . '://' . $host . '/' . $db_info['default']['database']);
   

   Can we use the methods withScheme(), withHost() and withPath().

@david_garcia: You are the maintainer for the Microsoft SQLServer. Can you confirm that this patch works for the Microsoft SQLServer driver? And what namespace do you use?

For me the patch is almost RTBC. I do have one issue left:

1. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -2,6 +2,8 @@
   +
   
   @@ -498,29 +492,67 @@ public static function convertDbUrlToConnectionInfo($url, $root) {
   +    $uri = $uri->withScheme($db_info['default']['driver'])
   +      ->withHost($host)
   +      ->withPath($db_info['default']['database']);
   +
   +    if (!empty($db_info['default']['prefix']['default'])) {
   +      $uri = $uri->withFragment($db_info['default']['prefix']['default']);
   +    }
   

   Why do we not remove $uri = part? So we will get:

      $uri->withScheme($db_info['default']['driver'])
        ->withHost($host)
        ->withPath($db_info['default']['database']);
   
      if (!empty($db_info['default']['prefix']['default'])) {
        $uri->withFragment($db_info['default']['prefix']['default']);
      }
   

2. The same for:

   +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1471,4 +1473,62 @@ public function __sleep() {
   +    $uri = $uri->withUserInfo($user, $password);
   +    if (!empty($connection_options['port'])) {
   +      $uri = $uri->withPort($connection_options['port']);
   +    }
   

3. The same for:

   +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -437,4 +438,30 @@ public function getFullQualifiedTableName($table) {
   +      $uri = $uri->withPath('/' . $connection_options['database']);
   

@david_garcia: Can you elaborate on what "Other things are broken". I have written a comment #2209307-47: Convert database drivers into a regular extension type and I was wondering what you think about it. And thank you for your interdiff file!

With the information from the previous comment, the patch is for me RTBC.
I do not have a contributed driver to test with and @david_garcia claims/proves that the patch works for MS SQL server driver.
The code looks good to me.

This looks good to me (just not sure on the Helper suffix to the new methods on Drupal\Core\Database\Connection), but I think it'd be good to get review from a database subsystem maintainer, so tagging for that. Moving to database since the more serious changes are there. Getting driver-specific logic into drivers is always good.

NR #67. Thanks @catch!

Now that @Crell is gone and @David_Strauss has not been seen in the database issue queue for months, am I removing the tag "Needs subsystem maintainer review" and putting it back to RTBC.

Reroll patch

Miss to rename stubConnection.php to Connection.php

1. +++ b/core/modules/simpletest/simpletest.module
   @@ -337,7 +339,7 @@ function simpletest_phpunit_run_command(array $unescaped_test_classnames, $phpun
   -    $escaped_test_classnames = array_map(function($class) {
   +    $escaped_test_classnames = array_map(function ($class) {
   
   +++ b/core/modules/system/tests/src/Kernel/Scripts/DbCommandBaseTest.php
   @@ -1,10 +1,5 @@
   -/**
   - * @file
   - * Contains \Drupal\Tests\system\Kernel\Scripts\DbCommandBaseTest.
   - */
   -
   

   These changes, whilst correct, are out-of-scope. The @file change is especially problematic as this is supposed to be checked by phpcs but apparently the sniff is broken in some way. We need to find out why (it's because there is one than one class in this file). So we need to file an issue against the coder module and also against core to fix it. Fixing it in core might involve just move the test class to its own file like it should be in strict psr-0/4.

2. +++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
   @@ -46,10 +46,10 @@ public function providerPrefixRoundTrip() {
   -    $reflection = new \ReflectionClass('Drupal\Tests\Core\Database\Stub\StubConnection');
   +    $reflection = new \ReflectionClass('Drupal\Tests\Core\Database\Stub\Connection');
   
   @@ -214,9 +214,9 @@ public function testSchema($expected, $driver, $namespace) {
   -      'Drupal\Tests\Core\Database\Stub\StubConnection',
   +      'Drupal\Tests\Core\Database\Stub\Connection',
   
   @@ -288,10 +288,10 @@ public function providerFilterComments() {
   -    $reflection = new \ReflectionClass('Drupal\Tests\Core\Database\Stub\StubConnection');
   +    $reflection = new \ReflectionClass('Drupal\Tests\Core\Database\Stub\Connection');
   

   If we're chagning this (and as there are other things to do) Let's do Connection::class instead of the hard-coded fully-qualified-classname.

Replaced 'Drupal\Tests\Core\Database\Stub\Connection' with Connection::class as per 77.2.

Setting back to Needs Work for 77.1, I tried to open a new ticket against core, but couldn't quite put the problems into words, sorry.

Can somebody remove the changes from the patch as @alexpott wants in comment #77.1.

Re-instated @file. Reverted out-of-scope code-style correction.

@Jo Fitzgerald: Thanks.

All points of @alexpott are addressed.
Back to RTBC.

+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -457,7 +457,7 @@ protected function getDatabaseConnectionInfo() {
-          'default' => $value['prefix']['default'] . $this->databasePrefix,
+          'default' => $this->databasePrefix,

This change is odd - how come? Reading back... the answer is in #34. Looks good and makes sense.

Sorry I've realised that there is more work we need - the issue summary doesn't really contain the arrived at solution and we need a CR to tell alternate DB drivers how to be updated for this change.

1. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -2,6 +2,8 @@
   +use GuzzleHttp\Psr7\Uri;
   
   @@ -455,36 +452,33 @@ public static function ignoreTarget($key, $target) {
   +    $uri = new Uri($url);
   

   Am I the only one who is a bit confused about adding a HTTP library as dependency for a database subsystem?

2. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -455,36 +452,33 @@ public static function ignoreTarget($key, $target) {
      public static function convertDbUrlToConnectionInfo($url, $root) {
   

   I'm wondering where we could document this general capability.

3. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -455,36 +452,33 @@ public static function ignoreTarget($key, $target) {
   +    $driver_class = static::getDatabaseConnectionClass($connection_options['driver'], $connection_options['namespace']);
   +    if (!class_exists($driver_class)) {
   +      throw new \RuntimeException("Can not convert $url to a database connection, class $driver_class does not exist");
        }
   

   I like checking for misconfigured database connections here. This allows people to debug failures much easier than when the system breaks somewhere later.

4. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -498,29 +492,67 @@ public static function convertDbUrlToConnectionInfo($url, $root) {
   +  public static function convertConnectionInfoToUrl($db_info) {
   

   We could typehint with array.

5. +++ b/core/modules/simpletest/simpletest.module
   @@ -308,9 +308,11 @@ function simpletest_phpunit_configuration_filepath() {
      global $base_url;
   -  // Setup an environment variable containing the database connection so that
   -  // functional tests can connect to the database.
   -  putenv('SIMPLETEST_DB=' . Database::getConnectionInfoAsUrl());
   +  if (Database::getConnectionInfo()) {
   

   ... It'd be nice to document why getConnectionInfo could fail here.

the issue summary doesn't really contain the arrived at solution

This is not always easy...

Well, right, it requires you to read the patch and the issue :) I did that now, so here is my approach to it.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1471,4 +1473,62 @@ public function __sleep() {
+  public static function convertDbUrlToConnectionInfoHelper(UriInterface $uri, $root, array $connection_options) {
...
+  public static function getConnectionInfoAsUrlHelper(array $connection_options, UriInterface $uri) {

Is it just me that "helper" as a method name is kind of not descriptive?

1. Tested the patch in #81 in conjuction with an experimental driver for Doctrine DBAL https://github.com/mondrake/drudbal. Works fine, thank you.

2. In the driver's code, added overrides for Connection::getConnectionInfoAsUrlHelper and for Connection::convertDbUrlToConnectionInfoHelper, in order to add an additional part to the URL querystring. It also work fine. Added this below as an example for a CR if you believe relevant.

  /**
   * {@inheritdoc}
   */
  public static function getConnectionInfoAsUrlHelper(array $connection_options, UriInterface $uri) {
    $uri = parent::getConnectionInfoAsUrlHelper($connection_options, $uri);
    // Add the 'dbal_driver' key to the URI.
    if (!empty($connection_options['dbal_driver'])) {
      $uri = Uri::withQueryValue($uri, 'dbal_driver', $connection_options['dbal_driver']);
    }
    return $uri;
  }
  /**
   * {@inheritdoc}
   */
  public static function convertDbUrlToConnectionInfoHelper(UriInterface $uri, $root, array $connection_options) {
    $connection_options = parent::convertDbUrlToConnectionInfoHelper($uri, $root, $connection_options);
    // Add the 'dbal_driver' key to the connection options.
    $parts = [];
    parse_str($uri->getQuery(), $parts);
    $dbal_driver = isset($parts['dbal_driver']) ? $parts['dbal_driver'] : '';
    $connection_options['dbal_driver'] = $dbal_driver;
    return $connection_options;
  }

Note: if you use the URL in the phpunit.xml file to set the db environment for testing, and the driver adds parts to the querystring, you have to manually convert the querystring separators & to HTML entities counterparts &. So, if you get

dbal://username:password@host:port/database?namespace=Drupal%5CDriver%5CDatabase%5Cdbal&dbal_driver=pdo_mysql#prefix

from call to \Drupal\Core\Database\Database::getConnectionInfoAsUrl, your phpunit.xml entry should be something like

<env name="SIMPLETEST_DB" value="dbal://username:password@host:port/database?namespace=Drupal%5CDriver%5CDatabase%5Cdbal&amp;dbal_driver=pdo_mysql#prefix"/>

3. While trying to get a clean run of phpunit --group Database with the driver, I incurred in a couple of issues that IMO need change in core:
a) Contrib/custom database drivers are not in the Drupal\Core\Database namespace, and in this case Log::findCaller will not report the query caller correctly. Filed #2867788: Log::findCaller fails to report the correct caller function with non-core drivers. for that.
b) If a database driver (contrib) is not implementing a PDO connection as Connection::$connection, the ConnectionUnitTest::testConnectionOpen test fails because it expects that the connction is PDO and PDO methods are available. Filed #2867700: ConnectionUnitTest::testConnectionOpen fails if the driver is not implementing a PDO connection for that.

Not sure if documenting this somewhere would really have any impact.

Call me weird, but if you don't know whether it has an impact than it certainly has one :) You know that kind of stuff already, but what if someone stumbles upon on. They wonder why its there etc. The amount of hidden costs in maintaining software is a LOT.

Adding a related issue.

Does anybody know is there any Drush issue to allow Drush si with a --db-url option that will be according to the changes in this issue? ATM Drush can only cope with core db drivers AFAICS

I fully agree with @dawehner in #86.1, it is very weird to see a HTTP library in the DB layer. Why can't we continue to use parse_url() and send around the array that results from it?

@david_garcia, the interdiff from #69 shows, to me at least, that the patch simply ported some code incorrectly:

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -498,29 +492,67 @@ public static function convertDbUrlToConnectionInfo($url, $root) {
-      $db_url .= '/' . $db_info['default']['database'];
...
+      ->withPath($db_info['default']['database']);

One of the goals of the database layer is to be made available as a generic PHP component outside of Drupal, and, IMO, adding a dependency on a HTTP library hinders that effort.

@david_garcia Drush will not work with contributed database drivers. Try with the Drupal Console...

Nice, thank you. I tried it to establish a test webserver on Travis via drupal server, works fine. Still, it would be nice to support drupal site:install with a kind of --db-url option after this patch is in, it would simplify installation for testing purposes (it would be the same info passed on to PHPUnit in the SIMPLETEST_DBvariable for Kernel tests).

But given the state of PHP, the state of Drupal, and how things have evolved, IMHO it makes little to no sense to still hold such a statement.

I'm not sure what are the states you are referring to, but #1826054: [Meta] Expose Drupal Components outside of Drupal is still open and actively (more or less) discussed.

Adding a related issue.

I think a database system maintainer should be asked in this case of conflict.

I doubt this can be RTBC with "needs change record" and "needs issue summary update"

Added a draft Change record https://www.drupal.org/node/2896416, editing from @david_garcia proposal in the IS, and adding some of my points from #87.

This patch is a must have if we want to allow any proper testing of contributed/custom database drivers. An experimental Drupal database driver for Doctrine DBAL has been using the patch in #88 for Travis testing for a couple of months with no problems. Just setting a different environment variable for SIMPLETEST_URL allows to identify the combination of database platform/database driver to execute the tests with (see https://travis-ci.org/mondrake/drudbal).

With regards to discussion about adding dependency from guzzlehttp/psr7 to manipulate URI's, I can only say I found very handy to have its methods available to be able to add an additional URL querystring parameter without having to write additional code (see the example in the draft CR).

Launched a retest on #88 yesterday, it came back green. RTBC from my point of view.

Not sure this can make into 8.3.x, but the 8.4.x failures of the patch in #88 seem related to a commit of #2891911: Random fail in Drupal\Tests\locale\Functional\LocaleTranslationUiTest::testStringTranslation that was reversed. Triggered new tests.

Applying patches is surprisingly easy these days with composer :) I think we should really not worry anymore, when a specific patch goes in, as long it does at some point.

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -455,36 +452,33 @@ public static function ignoreTarget($key, $target) {
+      'database' => substr($uri->getPath(), 1),

Should we be using ltrim here instead? Presence of isAbsolutePathReference on Uri class suggests it doesn't always start with a leading / ?

#77.1 references out of scope changes, these still exist in the current patch.

#67 asks for subsystem maintainer review, which hasn't happened yet - pinged david-strauss via his contact form.

Let's give him a week or two to review. If someone is willing to put their hand up to become database subsystem co-maintainer, that'd be great - please open an issue.

I'm reviewing this issue at the request of Lee (larowlan).

I'm not a fan of calling it just "namespace." As we can see from Alex's comment (#11), doing so can create confusion. I would at least change it to "php-namespace" or "driver-namespace" if we take the approach of putting it into the query string.

What I'd prefer, though, is moving the driver specification to the scheme. Specifying the Drupal-level driver should imply the underlying (PHP extension-level) driver. Valid characters in a scheme include ".", "-", and "+". Using one of those instead of backslashes (probably periods) would allow specifying the PHP namespace within the scheme. We could abbreviate common namespaces to avoid becoming too verbose in the scheme (if there's objection to a lengthy scheme). Seeing the PHP namespace written in the query string with lots of URI escaping (for the backslashes) is ugly, anyway.

I don't have a strong opinion on adding a dependency on Guzzle for URI parsing, especially because I can't find (via Ctrl-F) any explanation on this issue why it's preferable over parse_url(). I imagine it's because the "namespace" query parameter needs to get parsed out, and parse_url() only returns the whole query string. If so, then that's even more reason to put the driver namespace into the scheme.

@david_garcia Please post interdiffs, yes, using patch files is really antique, but at least let's make it not harder than it could be.

Many of the test failures were due to a call to file_scan_directory() so I have replaced that with a (over-?)simplified version of the function. Let's see what this gives us...

1. Corrected missing rename of StubConnection.php to Connection.php. (N.B. not included in interdiff)
2. Hard-coded DRUPAL_ROOT because it is undefined here.
3. Removed redundant DatabaseDriverDiscovery parameter drupalFunctionPattern.
4. Corrected test failures
5. Corrected coding standards errors

Thanks a lot! This runs fine with a contrib db driver too: https://travis-ci.org/mondrake/drudbal/builds/318605474?utm_source=githu...

We are deprecating 3 functions in install.inc now, and no longer passing the 'namespace' in the querystring. I think we need to update both the IS and the CR with this evolution.

1. +++ b/core/includes/install.inc
   @@ -141,15 +142,13 @@ function drupal_install_profile_distribution_version() {
   + *
   + * @deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.x. Use
   + *   \Drupal\Core\Database\DatabaseDriverDiscovery component.
     */
    function drupal_detect_database_types() {
   -  $databases = drupal_get_database_types();
   -
   -  foreach ($databases as $driver => $installer) {
   -    $databases[$driver] = $installer->name();
   -  }
   -
   -  return $databases;
   +  $discovery = new DatabaseDriverDiscovery();
   +  return $discovery->detectDatabaseTypes();
    }
   

   AFAIK, we need a @see referencing the CR, a @trigger_error at the beginning of each method repeating the deprecation information, addition of silenced deprecation messages to the list in Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations, and a follow-up to remove the deprecated usages (yes, a lot of stuff:()

2. +++ b/core/lib/Drupal/Core/Database/DatabaseDriverDiscovery.php
   @@ -0,0 +1,137 @@
   +  public function __construct() {
   +    // Hard-coded DRUPAL_ROOT (because bootstrap.inc is not accessible).
   +    $this->drupalRoot = dirname(dirname(dirname(dirname(dirname(__DIR__)))));
   +    // See composer.json for discoverable namespaces.
   +    $this->discoveryLocations = [
   +      'Drupal\\Core\\Database\\Driver' => '/core/lib/Drupal/Core/Database/Driver',
   +      'Drupal\\Driver\\Database' => '/drivers/lib/Drupal/Driver/Database'
   +    ];
   +  }
   

   dirname(dirname(dirname(dirname(dirname(__DIR__))))) is ugly... can we just change the discoveryLocations to something like 'Drupal\\Core\\Database\\Driver' => '../../../../..core/lib/Drupal/Core/Database/Driver', and drop drupalRoot altogether?

3. +++ b/core/modules/system/tests/src/Kernel/Scripts/DbCommandBaseTest.php
   @@ -1,10 +1,5 @@
   -/**
   - * @file
   - * Contains \Drupal\Tests\system\Kernel\Scripts\DbCommandBaseTest.
   - */
   -
   

   I think we should keep this since there are 2 classes in this file, and according to CS only single-class php files can miss the annotation

Working on update of IS and CR.

Updated IS and CR.

Just realized... do we need separate tests for the new DatabaseDriverDiscovery class?

I reviewed the patch. I am sorry for being a nitpick.

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1472,4 +1474,62 @@ public function __sleep() {
   +   * @param string $root
   +   *   The root directory of the Drupal installation.
   ...
   +  public static function convertDbUrlToConnectionInfoHelper(UriInterface $uri, $root, array $connection_options) {
   

   Why do we need the second parameter? Its not used in the method. Remove?

   Update: I found that it being used by SQLite. Should we document that here?

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1472,4 +1474,62 @@ public function __sleep() {
   +    $uri = $uri->withUserInfo($user, $password);
   

   Not sure if we need to do something if the value for $user is empty. Do we have testing for this?

3. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1472,4 +1474,62 @@ public function __sleep() {
   +   * @return $this|\Psr\Http\Message\UriInterface
   

   This method will never return $this.

4. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1472,4 +1474,62 @@ public function __sleep() {
   +    $user = isset($connection_options['username']) ? $connection_options['username'] : '';
   

   Nit. Can we change the variable name $user to $username, because that is the name we use in the Drupal DB layer.

5. +++ b/core/modules/system/tests/src/Kernel/Scripts/DbCommandBaseTest.php
   @@ -58,18 +53,16 @@ public function testSpecifyDatabaseDoesNotExist() {
   +      '-db-url' => Database::getConnectionInfoAsUrl('default')
   ...
   +      '--database-url' => Database::getConnectionInfoAsUrl('default')
   
   @@ -91,9 +84,8 @@ public function testPrefix() {
   +      '-db-url' => Database::getConnectionInfoAsUrl('default'),
   

   The parameter value "default" can be removed. It is the default parameter value.

6. +++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
   @@ -436,4 +437,30 @@ public function getFullQualifiedTableName($table) {
   +  public static function convertDbUrlToConnectionInfoHelper(UriInterface $uri, $root, array $database) {
   

   The parameter name should be $connection_options and not $database.

7. +++ b/core/lib/Drupal/Core/Database/DatabaseDriverDiscovery.php
   @@ -0,0 +1,137 @@
   + * Component used to perform database driver
   + * discovery.
   

   Nit. Can be put on one line.

8. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -498,29 +494,64 @@ public static function convertDbUrlToConnectionInfo($url, $root) {
   +   *   A custom namespace to use.
   

   Nit. "THE" custom namespace to use.

9. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -455,36 +452,35 @@ public static function ignoreTarget($key, $target) {
   +      throw new \RuntimeException("Can not convert $url to a database connection, class $driver_class does not exist");
   

   There is no documentation in the docblock for this exception.

10. +++ b/core/lib/Drupal/Core/Database/Database.php
    @@ -498,29 +494,64 @@ public static function convertDbUrlToConnectionInfo($url, $root) {
    +
    

    Nit. This empty line can be removed.

11. +++ b/core/lib/Drupal/Core/Database/Database.php
    @@ -498,29 +494,64 @@ public static function convertDbUrlToConnectionInfo($url, $root) {
    +      throw new \RuntimeException("Database connection $key not defined");
    

    Again. There is no documentation in the docblock for this exception.

12. +++ b/core/lib/Drupal/Core/Database/DatabaseDriverDiscovery.php
    @@ -0,0 +1,137 @@
    +    // The internal database driver name is any valid PHP identifier.
    

    I am sorry, but I do not understand this line. What is a valid PHP identifier?

13. +++ b/core/lib/Drupal/Core/Database/DatabaseDriverDiscovery.php
    @@ -0,0 +1,137 @@
    +          while (FALSE !== ($driverName = readdir($handle))) {
    

    Should this line not be written as while (($driverName = readdir($handle)) !== FALSE) {

14. +++ b/core/lib/Drupal/Core/Database/DatabaseDriverDiscovery.php
    @@ -0,0 +1,137 @@
    +              if (isset($files[$driverName])) {
    +                throw new \Exception("Conflicting database driver name: " . $driverName);
    +              }
    

    Not sure about this. If somebody wants to make a better/faster/improved version of say for instance the Postgres driver, then that is not possible.

15. +++ b/core/lib/Drupal/Core/Database/DatabaseDriverDiscovery.php
    @@ -0,0 +1,137 @@
    +          while (FALSE !== ($driverName = readdir($handle))) {
    +            // Skip this file if it matches the nomask or starts with a dot.
    +            if ($driverName[0] != '.') {
    +              if (isset($files[$driverName])) {
    +                throw new \Exception("Conflicting database driver name: " . $driverName);
    +              }
    +              $files[$driverName] = $namespace . '\\' . $driverName;
    

    Can we add some documentation about what the value can and/or should be. It is a bit vague to me.

16. +++ b/core/lib/Drupal/Core/Database/DatabaseDriverDiscovery.php
    @@ -0,0 +1,137 @@
    +  public function discoverDatabaseDrivers() {
    

    Missing documentation about the possible exception that can be thrown. Also the return value is not documented. This is a complicated array. Lots of documentation for this please.

17. +++ b/core/lib/Drupal/Core/Database/DatabaseDriverDiscovery.php
    @@ -0,0 +1,137 @@
    +    $drivers = [];
    

    Can we move this line to just before the line with: foreach ($files as $driver => $namespace) {.

18. The database drivers are file scanned every time. Should we not cache this?

I'll be working on #122, #125 and #126

This is adding only the deprecation triggers per #122.1. Running a test to see if they pop up as expected, will silence in next patch.

This patch is

• silencing the deprecations
• made several changes to the DatabaseDriverDiscovery class - include renaming the methods to something more understandable and specific to what they are doing (the function names in install.inc are misleading)
• dropping the Database::getDatabaseConnectionClass in favour of a more generic Database::getDatabaseDriverNamespace that can be used also for other purposes (for example, in #2867788: Log::findCaller fails to report the correct caller function with non-core drivers.).

With regards to #126.14 - yes, you can write an alternative db driver for a db already covered by a core driver - but you have to specify its name differently otherwise you'll get namespace clashes.

This addresses #122 and #126.7/12/13/14/15/16/17/18.

Left out: #125 (tests) and #126.1-6/8-11

the patch...

This adds tests for the DatabaseDriverDiscovery class. The 'Stub' database driver in the Database unit test directory is expanded wit a stub installer too, so we can test its discovery. Additional change to runtime code to match the testing.

Left out: #126.1-6/8-11

#133 was a conflict in the list of silenced deprecations.

@mondrake: The changes that you made in #127 - #135 look good to me. There are some points left to fix, so back to "Needs work".

Addressed the remaining points from #126: 1-6 and 8-11.

Two small CS issues fixed.

#139 is an apcu failure

Very hard to get tests to pass these days...

Thinking about DatabaseDriverDiscoveryTest, this assumes that during a test run all of the core drivers (mysql, sqlite, pgsql) are installable. This is true for DrupalCI (and TravisCI), but on local runs this may lead to failure if the dbs are not installed. Let's see if we can sort out something that can work in these cases.