Support contrib database driver directories in a fixed location in a module

This would be great! Thanks for working on this.

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1739,6 +1786,12 @@ public static function createUrlFromConnectionOptions(array $connection_options)
   +    // Allow contrib database drivers to have a driver name that is not the same
   +    // as its schema name.
   +    if ($database_type != $connection_options['driver']) {
   +      $db_url .= '?driver=' . $connection_options['driver'];
   +    }
   

   As I pointed out in slack I think this should be ?module and not ?driver. ne thing we can’t avoid in Drupal (yet) is that extension name is synonomous with namespace i.e . node has to refer to \Drupal\node and what driver in your version means is what \Drupal\{driver} it is in. I think it is better to call an apple an apple and use ?module instead of driver. In this case ?module means the module that provides the driver. Modules are an existing concept with how to load it’s namespace the way we want it to ie. \Drupal\MODULE. And one is an existing concept with super awkward code loading (driver). So using ?module points out

   • that this is module provided driver and which one
   • that we’re going to load it the same way we load other module code.

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1731,7 +1743,42 @@ public static function createUrlFromConnectionOptions(array $connection_options)
   +    // Try to get the DATABASE_TYPE constant from the given $connection_class.
   +    // Use that value as the schema name if it extists or default to the driver
   +    // name.
   +    $reflection = new \ReflectionClass($connection_class);
   +    if ($reflection->hasConstant('DATABASE_TYPE')) {
   +      $database_type = $reflection->getConstant('DATABASE_TYPE');
   +    }
   +    else {
   +      $database_type = $connection_options['driver'];
   +    }
   

   We've already determined this. We've determined if a module is providing a database driver above ... if "Drupal\\{$driver}\\Driver\\Connection"; exists then $driver is the module. Also I think we should support this naming scheme first before the old contrib on of Drupal\\Driver\\Database\\{$driver}\\Connection because we need to deprecate that in Drupal 9 for removal in Drupal 10 because this change makes it obsolete.

3. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -457,14 +457,38 @@ public static function convertDbUrlToConnectionInfo($url, $root) {
   +    // Driver module/namespace names cannot have the character dash and driver
   +    // schema names cannot have the character underscore (the PHP function
   +    // parse_url does not allow it). For example the contrib driver
   +    // "pgsql-fallback" uses the module/namespace name "pgsql_fallback".
   +    $driver = str_replace('-', '_', $driver);
   

   If we pass around the module then we don't need this awkwardness. The whole concept of database type vs driver name should be deprecated. We should have a driver name and the module that provides it (or core it is it core).

1. +++ b/core/includes/install.inc
   @@ -180,6 +184,25 @@ function drupal_get_database_types() {
   +    $path_src = DRUPAL_ROOT . '/' . $module->getPath() . '/src/Driver/Install/Tasks.php';
   

   +1 to #13.1. I would also suggest being explicit in the namespace that it's a database driver, and add the driver name (which might be different than the module name). In other words, scan MODULE_PATH/src/Driver/Database for potentially multiple drivers (since we can't infer the driver name from the module name). I don't know if there's any good examples of a module that would want to provide multiple database drivers, but an example of a module with a different name than the driver is https://www.drupal.org/project/mysql56, where I'd like to have mysql56 as the module name and mysql as the driver name.

2. +++ b/core/includes/install.inc
   @@ -180,6 +184,25 @@ function drupal_get_database_types() {
   +      $drivers[$module->getName()] = $path_src;
   

   This prevents the above goal of separating module name and driver name.

3. +++ b/core/includes/install.inc
   @@ -180,6 +184,25 @@ function drupal_get_database_types() {
   +      $class_loader->addClassMap(['Drupal\\' . $module->getName() . '\\Driver\\Install\\Tasks' => $path_src]);
   

   If we're only adding this one class to the autoloader, then when we get to the "Setup database" step of the web installer, then it errors out when trying to instantiate the connection class, because that's not in the autoloader (at this point in the installation, the module isn't enabled). Do we want to add the entire driver namespace to the autoloader here, or should we make it the module's responsibility to add its autoload entry to its composer.json?

4. +++ b/core/includes/install.inc
   @@ -1097,6 +1120,12 @@ function db_installer_object($driver) {
   -    return new $task_class();
   +    if (class_exists($task_class)) {
   +      return new $task_class();
   +    }
   +    else {
   +      $task_class = "Drupal\\{$driver}\\Driver\\Install\\Tasks";
   +      return new $task_class();
   +    }
   

   We should flip these, so that we check the module first. But also, if we separate module name and driver name, how do we know which module to check?

I'm kind of wondering if we even want to keep going down the road of searching within each module. What if we just keep Drupal's existing namespace of Drupal\\Driver\\Database\\DRIVER_NAME, and if modules want to add to this namespace, they can put the desired autoload entry into their composer.json. And then for scanning within drupal_get_database_types(), can we scan for what in $class_loader->getPrefixesPsr4() matches Drupal\\Driver\\Database\\...? That would also allow projects that want to provide a driver to not be modules if they don't have to be. E.g., their code can remain in /vendor, out of the webroot, if there's nothing module-like that they want to offer.

If we don't complete this issue before beta, then can we get #3119853: Add database driver installer path to Composer project templates in before beta instead, and then keep refining this issue post-beta? Because it looks like this issue isn't removing the scanning for what's in the "drivers" directory, just making it obsolete. But for as long as we continue to have the "drivers" directory, #3119853: Add database driver installer path to Composer project templates would be a helpful way of managing it.

@daffie we don't need to be so concerned with the URL structure. The representation of a db connection as a URL is wrong and we need to deprecate that in Drupal 9 and turn it into a json string that is the same as the databases array in PHP.

Yes driver can't contain an underscore but that's already the case. But it shouldn't affect your module. Taking the example of the pgsql_fallback module. Imo this should have a driver called pgsql. So the urls will be pgsql://usename:password@localhost/database?module=pgsql_fallback. This should map to a namespace of \Drupal\pgsql_fallback\Driver\Datbase\pgsql. This will allow modules to have more that one driver.

We need to change drupal_get_database_types() to key on "module:driver" and also we need to make everything work as expected when module = core.

Just want to leave a note here that I won't have time to review this until next week at the soonest. Please feel free to carry on and commit without me. For drivers of the same name as core ones, I want to point out #3119853-8: Add database driver installer path to Composer project templates, but there's still plenty of other improvements here that would be needed for drivers of different names, so I hope this lands in beta or soon after.

The "beta target" tag means the change is allowed and prioritized for commit during the beta phase (i.e., between the release of 9.0.0-beta1 and 9.0.0-rc1.)

This issue does not meet the criteria for allowed changes during the beta phase. (It includes API additions, etc.) Therefore, it would need release management signoff to go into 9.0.x as a beta target after 9.0.0-beta1 ships. This issue might be a candidate for a release management exception as a contrib blocker. Let's tag it as such if that's appropriate. Let's also update the issue summary to fill out a summary of the API additions, internal API changes, potential disruptive impacts, etc.

Thanks everyone!

Oh also, if this meets the criteria for a major task or bug, we should also promote it accordingly. Thanks!

I discussed the return value changes to drupal_get_database_types() with @daffie. I think we can get away without having to make them if we change the patch to make the same assumption about database drivers as core makes about modules. If you place a node module in modules/contrib it will take precedence of core/modules/node. In fact the system will behave as if the core node module does not exist.

Therefore we could return use just the driver in the key to preserve BC but use a defined precedence to say what happens if contrib provides the a driver with the same name as core. The order of precedence - increasing in priority would be core, old contrib namespace, the new contrib namespace added here.

With respect to the fallback drivers being added to sort mysql 5.6 and postgres 9 I don't think these drivers should use the same name as the core driver since it's not really necessary.

1. +++ b/core/includes/install.inc
   @@ -177,11 +181,39 @@ function drupal_get_database_types() {
   +          $class_loader = new ClassLoader();
   +          $class_loader->addClassMap(['Drupal\\' . $module->getName() . '\\Driver\\' . $driver_file->filename . '\\Install\\Tasks' => $tasks_file]);
   +          $class_loader->register(TRUE);
   

   We shouldn't construct our own class loader here. We can use the one we already have.

2. +++ b/core/includes/install.inc
   @@ -1082,13 +1114,16 @@ function install_profile_info($profile, $langcode = 'en') {
   + * @param $module
   + *   The name of the module that is the provider of the driver. For the by core
   + *   supported drivers the module name is the driver name.
   ...
   -function db_installer_object($driver) {
   +function db_installer_object($module, $driver) {
   

   Let's swap the args, default to NULL and trigger a deprecation if $module is not provided. There are some contrib usages - http://grep.xnddx.ru/search?text=db_installer_object&filename= - and we can support the old namespaces for now. We should do the deprecation after we've deprecated support for the old custom driver namespace and potentially moved core drivers into their own modules.

3. +++ b/core/includes/install.inc
   @@ -1096,7 +1131,13 @@ function db_installer_object($driver) {
        return new $task_class();
      }
      else {
   -    $task_class = "Drupal\\Core\\Database\\Driver\\{$driver}\\Install\\Tasks";
   -    return new $task_class();
   +    $task_class = "Drupal\\{$module}\\Driver\\{$driver}\\Install\\Tasks";
   +    if (class_exists($task_class)) {
   +      return new $task_class();
   +    }
   +    else {
   +      $task_class = "Drupal\\Core\\Database\\Driver\\{$driver}\\Install\\Tasks";
   +      return new $task_class();
   +    }
      }
   

   We need to check $task_class = "Drupal\\{$module}\\Driver\\{$driver}\\Install\\Tasks"; before $task_class = "Drupal\\Driver\\Database\\{$driver}\\Install\\Tasks"; so that drivers in the module namespace have the top priority.

4. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1685,6 +1685,19 @@ public static function createConnectionOptionsFromUrl($url, $root) {
   +    // If the driver is provided by a module and the module name is not equal to
   +    // the driver name, then add the module name to the return array.
   +    if (isset($url_components['query'])) {
   +      $query = $url_components['query'];
   +      $query_variables = explode('&', $query);
   +      foreach ($query_variables as $query_variable) {
   +        list($query_variable_key, $query_variable_value) = explode('=', $query_variable, 2);
   +        if (($query_variable_key == 'module') && ($database['driver'] != $query_variable_value)) {
   +          $database['module'] = $query_variable_value;
   +        }
   +      }
   

   This should use parse_str();... ie.

       if (isset($url_components['query'])) {
         parse_str($url_components['query'], $query);
         if ($query['module']) {
           $database['module'] = $query['module'];
         }
       }
   

   Also I think we should set module if we have it.

5. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -1739,6 +1752,12 @@ public static function createUrlFromConnectionOptions(array $connection_options)
   +    // Only set the module name when the module name is not the same as the
   +    // driver name.
   +    if (isset($connection_options['module']) && ($connection_options['driver'] != $connection_options['module'])) {
   +      $db_url .= '?module=' . $connection_options['module'];
   +    }
   

   I think we should set $module if $connection_options['module'] is set. We don't need the additional complexity here.

6. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -455,16 +455,34 @@ public static function convertDbUrlToConnectionInfo($url, $root) {
   +    // Allow the driver name to be something else then the schema name.
   +    $url_components = parse_url($url);
   +    if (isset($url_components['query'])) {
   +      $query = $url_components['query'];
   +      $query_variables = explode('&', $query);
   +      foreach ($query_variables as $query_variable) {
   +        list($query_variable_key, $query_variable_value) = explode('=', $query_variable, 2);
   +        if ($query_variable_key == 'module') {
   +          $module = $query_variable_value;
   +        }
   +      }
   +    }
   

   As above - this should use parse_str. Also the comment needs updating.

7. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -455,16 +455,34 @@ public static function convertDbUrlToConnectionInfo($url, $root) {
        // Discover if the URL has a valid driver scheme. Try with custom drivers
        // first, since those can override/extend the core ones.
        $connection_class = $custom_connection_class = "Drupal\\Driver\\Database\\{$driver}\\Connection";
        if (!class_exists($connection_class)) {
   -      // If the URL is not relative to a custom driver, try with core ones.
   -      $connection_class = "Drupal\\Core\\Database\\Driver\\{$driver}\\Connection";
   +      // If the URL is not the default custom driver, try with the new custom
   +      // ones.
   +      $connection_class = "Drupal\\{$module}\\Driver\\{$driver}\\Connection";
          if (!class_exists($connection_class)) {
   -        throw new \InvalidArgumentException("Can not convert '$url' to a database connection, class '$custom_connection_class' does not exist");
   +        // If the URL is not relative to a custom driver, try with core ones.
   +        $connection_class = "Drupal\\Core\\Database\\Driver\\{$driver}\\Connection";
   +        if (!class_exists($connection_class)) {
   +          throw new \InvalidArgumentException("Can not convert '$url' to a database connection, class '$custom_connection_class' does not exist");
   +        }
          }
   

   The new module namespace should have priority

The patch attached addresses these points.

The big issue we now need to deal with is that we need to add a real test of a installing with a module provided driver. I'm pretty sure we have a big problem at the moment since the container is cached in the database and that has the module list and that is used to populate the autoloader. I think we need to add autoloading instructions to the database array. In fact I think we need to add a driver_autoload property that contains the path to the module's driver directory. That way we'll be able to set up the autoloader in \Drupal\Core\Site\Settings::initialize() to be able to load the cached container using a module provided database driver.

@daffie to answer

If I wanted to create an alternative implementation for the node module, I should create a contrib module called node.

. You can do this by putting your node-overriding module inside your project. I.e. create a project on d.o called funkynode and inside that have a directory module/node that contains your override of the node module. This whole problem space is driven by how we place modules anywhere and everywhere. And do not use composer to build this but of the autolooader. We do it ourselves in \Drupal\Core\DrupalKernel::classLoaderAddMultiplePsr4()

The IS still needs to be updated with the proposed API changes here. How will the change affect existing custom drivers? Is there a BC version we can backport to D8?

Re #33 we're trying to ensure that we do not break support at all for Drupal 8 custom drivers and that this change only results in new improved functionality and ease-of-use. Once we'll settled a few questions we'll update the issue summary and add a change record.

Here are my thoughts on how to address #31.

• We need to write the location of the database driver to the database info array in the installer. When we get a db URL we can scan all the modules for the provided module to set it correctly. This information should be written to settings.php as part of \Drupal\Core\Installer\Form\SiteSettingsForm::submitForm()
• I think we can set up the class loader using the info we add there in \Drupal\Core\Site\Settings::initialize()
• We need another key name in the db info… I’ve was thinking about namespace_autoload_dir or something… not sure… we’ve got namespace already and this telling the system where to load that namespace from.
• \Drupal\Core\Database\Database::convertDbUrlToConnectionInfo() will need to scan the module directories to populate the same info.

And then, fingers-crossed, we’ll have no copy custom/contrib db driver support!

Here's a patch the adds proper autoloading (ie. the test no longer has to autoload the test drivers) and you can install Drupal using the test drivers! $settings['extension_discovery_scan_tests'] = TRUE; put that in your settings.php and you'll be able to select them in the installer.

I think this patch might fail because the test drivers are using the same driver name as core's and therefore replace them - so the interactive installer tests are going to use them :) let's find out.

I also think we'll be able to refactor the patch to have less duplicate functionality.

So looking at http://grep.xnddx.ru/search?text=createConnectionOptionsFromUrl&filename= we can do better for BC (and performance and simplicity) by setting module and namespace_dir in \Drupal\Core\Database\Database::convertDbUrlToConnectionInfo() and not each driverzs implementation of \Drupal\Core\Database\Connection::createConnectionOptionsFromUrl().

1. Fixed
2. Because this method can be used prior to Drupal being installed / the container existing. It's never called as part of the regular run time.
3. Done
4. Namespace is required and if it is not set you'll get exceptions already.
5. This is where settings.php is included - it sets the $databases variable.
6. We have the ability to tell the system to discover test module by setting extension_discovery_scan_tests to true in settings.php. For our test system we also need to always scan tests when drupal_valid_test_ua() this is how we can have 100s of test modules and ensure they don't affect anything outside of tests.
7. I think that this is an implementation detail that's not necessary. It's not how db drivers are autoloaded in real run-time code. That happens in Settings::initialize() - and that is worth documenting.
8. It still does throw the exception - it's just changed. Added the new exception case.
9. Going to do this next... still to do.
10. Fixed
11. I don't think we do. We set the namespace to $namespace = "Drupal\\$module\\Driver\\$driver"; - in this situation it's impossible for that method to return FALSE. It's either a string or an exception.

Looking at #42 I'm surprise you don't have an entry for 'module'

Ah I see - I realised that now we're writing namespace_dir to the connection options array we didn't need to write the module to the array as well. So the code in \Drupal\Core\Database\Connection::createUrlFromConnectionOptions() is wrong. Because we don't have 'module' in the connection options. Need to fix that.

Fixing #44.

One other consideration is that maybe we need to cope with contrib drivers like http://grep.xnddx.ru/search?text=createUrlFromConnectionOptions&filename= - so we need to do the same as we do for \Drupal\Core\Database\Database::convertDbUrlToConnectionInfo() and move the module stuff to (\Drupal\Core\Database\Database::getConnectionInfoAsUrl(). But this is tricky because then we have to parse the URL we're creating an add to the query string. But the URLs we're creating are fake. The scheme is not valid for example. So *maybe* it's acceptable to say that if a current contrib driver wants to move to the new module provided namespace they have to update the implementation to cope with this. The implementation here adds module information in \Drupal\Core\Database\Database::getConnectionInfoAsUrl() to make things as simple as possible for contrib.

I keep on forgetting that #3112476: Always set $info['namespace'] on database connection info has not landed yet :( we really need that patch prior to do this one so we can make correct assumptions about this property being set and to make core and contrib db driver info the same.

Addressing #41.9

So for me there are two things left to do here:

1. Move the contrib namespace from Drupal\mymodule\Driver\mydriver to Drupal\mymodule\Database\Driver\mydriver as there other drivers - maybe not in Drupal-land yet but this also nicely mirrors the core namespaces like Drupal\Core\Database\Driver\mysql - consistency ftw!
2. Add an installer test to prove that module provided drivers are listed and can be successfully used

+++ b/core/lib/Drupal/Core/Installer/Form/SiteSettingsForm.php
@@ -68,7 +68,7 @@
-    $include_tests = Settings::get('extension_discovery_scan_tests') || drupal_valid_test_ua();
+    $include_tests = Settings::get('extension_discovery_scan_tests') && drupal_valid_test_ua();

@@ -155,7 +155,7 @@
-    $include_tests = Settings::get('extension_discovery_scan_tests') || drupal_valid_test_ua();
+    $include_tests = Settings::get('extension_discovery_scan_tests') && drupal_valid_test_ua();

+++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
@@ -195,7 +195,7 @@ public function scan($type, $include_tests = NULL) {
-      $include_tests = Settings::get('extension_discovery_scan_tests') || drupal_valid_test_ua();
+      $include_tests = Settings::get('extension_discovery_scan_tests') && drupal_valid_test_ua();

This change is not a fix - it's a break. It is supposed to be an OR. Otherwise we can't set extension_discovery_scan_tests in settings.php and see the same as a BrowserTestBase.

+++ b/core/tests/Drupal/FunctionalTests/Installer/InstallerNonDefaultDatabaseDriverTest.php
@@ -0,0 +1,60 @@
+    // Add the database drivers from testing modules.
+    $this->settings['settings']['extension_discovery_scan_tests'] = (object) [
+      'value' => TRUE,
+      'required' => TRUE,
+    ];

This is not necessary as far as I know. drupal_valid_test_ua() will return something.

Ah I see why you were doing that because you want tests to use the regular core drivers. Well imo we should change the module provided drivers to not override core drivers. The use-case for overriding a core driver is very small and I don't believe we should be showing it in the main demo of this functionality - ie the driver_test module. We should encourage people developing the fallback drivers to name their drivers things like mysql_fallback.

@daffie just because the driver is for Postgres or Mysql the driver does not have to be called psql or mysql. It can be whatever we like. That's why we have \Drupal\Core\Database\Connection::databaseType(). I think overriding core drivers should not be the norm. I don't think that alternative MySQL implementations should replace the core one - I think they should be available alongside it. So we you look at the list in the installer you see the full range of options. The whole magically replace code stuff Drupal does is very confusing and I feel it's against https://en.wikipedia.org/wiki/Principle_of_least_astonishment. It's very hard to explain to some why placing code in one place makes functionality that was available go away.

Also the interdiff on #51 seems to be incorrect.

++ #52 looks like another drupalism based on hook_world_alter

EDIT mysql-legacy and pgsql-unsupported

So re the module in the connection url string and not in the connection options. The module key is only a shorthand for namespace / namespace_dir because from knowing the module we can determine the two things. But we cannot do that at runtime because scanning module directories on every request would be extremely expensive. As I've said before the whole URL to represent a database connection is wrong headed and needs to be deprecated in favour of a json string in an environment variable. There's already tonnes of things that go in connection options that cannot be represented on the URL - namespace, collation, init_commands, pdo. So I disagree that it is confusing or wrong. Also the only time these URLs are needed is to setup SIMPLETEST_DB or for a CLI install (eg drush si). In both cases the more usual thing to do is to pre-populate settings.php with this info anyway. The whole underscore vs dash thing highlights the complete awkwardness of representing db connections as URLs. It feels really wrong that we're driving design decision based on a URL representation of something that is not a URL - so it looks like we're going to need the - to _ conversion thing :( ho hum.

@daffie thanks for explaining about the interdiff... makes sense now.

Re the Drupal\{$module_name}\Driver\{$driver_name} vs Drupal\{$module_name}\Database\Driver\{$driver_name} as pointed out before a driver is not necessarily a database concept. I guess your concern is that the database folder is only going to have a single folder called Driver in it. I guess that's possible so maybe Drupal\{$module_name}\DatabaseDriver\{$driver_name} is a compromise but it is possible the a module might put other things in Drupal\{$module_name}\Database. Not sure but I am sure that we should improve Drupal\{$module_name}\Driver\{$driver_name}. Whatever we decide will be the same for core drivers provided as modules and contrib / custom.

Re #58.3

The fixes to Drupal\Core\Database\Install\Tasks are necessary.

+++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
@@ -226,7 +243,7 @@ public function getFormOptions(array $database) {
-          ':input[name=driver]' => ['value' => $this->pdoDriver],
+          ':input[name=driver]' => ['value' => $this->driver],

@@ -239,7 +256,7 @@ public function getFormOptions(array $database) {
-          ':input[name=driver]' => ['value' => $this->pdoDriver],
+          ':input[name=driver]' => ['value' => $this->driver],

This is actually a bug fix. This should never have been using the $this->pdoDriver - fortunately the fix is BC compat (nice one) and also easy to add a deprecation to in 9.1.x and also not that important - a field that would normally be required won't be. Fun.

  /**
   * Returns the type of database driver.
   *
   * This is not necessarily the same as the type of the database itself. For
   * instance, there could be two MySQL drivers, mysql and mysql_mock. This
   * function would return different values for each, but both would return
   * "mysql" for databaseType().
   *
   * @return string
   *   The type of database driver.
   */
  abstract public function driver();

The above docs from Drupal 9 HEAD show that the way that pdoDriver is being uses to decide whether a field is required is wrong. Doing this properly is revealing all the skeletons. So we definitely need to create the follow-up to deprecate thew BC layer.

Re #58.4 nice spot. But this also shows us that we shouldn't be using 'module' here at all. We can pass in the namespace and use that. The nice thing here is the once the namespace is required we can simplify this method and remove all the ifs :)

Add now we no longer need this code... should have removed in #59.

Fixing the tests. The database_statement_monitoring_test module is a bit dodgy and the way \Drupal\KernelTests\Core\Cache\EndOfTransactionQueriesTest::getDatabaseConnectionInfo() works to override the namespace without providing a complete driver is meh. But the fix is pretty simple - and makes the drivers provided by this module a little bit more complete. In the future we could change this module to provide complete drivers and use ability of modules to provide db drivers and make the implementation a whole less hacky.

I think we still need to deal the _ in driver name not being allowed in URL schema issue. Because I think pqsqlfallback is not as nice a namespace / driver name as pgsql_fallback. I guess we could bump that to a follow-up as it is not essential for delivering the functionality. But it is not that bad to do...

Patch

• fixes the test on the other db drivers
• completes the @todo's added by the patch
• Changes the way that $driver is determined in \Drupal\Core\Database\Install\Tasks::getFormOptions() to work for all drivers and make it less presumptive about the future - see #3123240: Provide a better way to get driver name and database type

Thanks @daffie @alexpott for doing this. +1 on the approach in general. I will try to get my experimental DruDbal driver fit with this, and report back.

Did some more thinking about the underscore in driver name issue. It's funny because we suggest it in the docs for \Drupal\Core\Database\Connection::driver() in HEAD but actually it doesn't work at all with out "let's represent a DB connection as a URL even though its not" approach. We could go down the path of converting - to _ and vice versa when handling URLs but then what happens when someone wants a - in there driver name? (Hint; it'll break without a lot of work). As I've said before we need to ditch the URL approach anyway - it introduces hidden complexity as is only needed to pass as a parameter into site-install commands and for SIMPLETEST_DB. So I no longer think we should even try to handle it. However that doesn't mean we can't do better in the current patch in terms of setting a standard for naming database drivers.

Database drivers map to PSR-4 namespaces. PSR-4 namespaces have clear rules about how classes and namespaces are supposed to be. CamelCase. So let's use that. It makes the test driver namespace much more readable and works perfectly with our db connection as URL out-of-the-box.

I think this patch is now complete for a code perspective and now I'm going to review the issue summary and change record - thanks for working on this already @daffie

I've adjusted DruDbal per #71, it used to be a driver-only codebase and now it's a module. See:

PR with changes in GitHub: https://github.com/mondrake/drudbal/pull/172/files

Test run of the driver on TravisCI with the Database group of tests: https://travis-ci.org/github/mondrake/drudbal/builds/668624989?utm_sourc...

The test also installs the site in a interactive-like mode, and that passes too.

Notes:
1. It's important to make clear you need to have a {module}.info.yml file in the code base, otherwise the driver is not discovered
2. You need to specifiy ?module in the DB URL for tests; also, if your querystring already has other parts, these are preserved (good!)
3. Calling db_installer_object without the $namespace parameter on sites installed with a contrib driver leads to

Error: Class 'Drupal\Core\Database\Driver\dbal\Install\Tasks' not found in /home/travis/drupal8/core/includes/install.inc on line 1144 
#0 /home/travis/drupal8/core/install_report.php(33): db_installer_object('dbal')
#1 {main}
Error: Class 'Drupal\Core\Database\Driver\dbal\Install\Tasks' not found in db_installer_object() (line 1144 of /home/travis/drupal8/core/includes/install.inc).

i.e. tries to load a class with a namespace built on D8 pattern. Not a big deal though - I encountered it in an external script, not Drupal. Also, calls to db_installer_object in contribland are very limited and would have to be adjusted anyway http://grep.xnddx.ru/search?text=db_installer_object&filename=

RTBC for me, thanks

Done some formatting of the issue summary and change record - the content is great. Nice work @daffie.

+++ b/core/modules/system/tests/modules/fakedriver/src/DatabaseDriver/fakedriver/Connection.php
@@ -0,0 +1,64 @@
+namespace Drupal\fakedriver\DatabaseDriver\fakedriver;
+
+use Drupal\Core\Database\Connection as CoreConnection;
+
+/**
+ * Fakedriver implementation of \Drupal\Core\Database\Connection.
+ */
+class Connection extends CoreConnection {

Was discussing with @daffie whether we should change this to Drupal\fakedriver\DatabaseDriver\FakeDriver when I realised now we have the drivertest module this thing is not needed at all. It's only duplicating test coverage we already have in the patch.

Less code to review ftw.

Nice spot @daffie even less test code ftw :)

Not really a full review, but a couple of questions.
The test failure is a random fail so moving back to RTBC.

1. +++ b/core/assets/scaffold/files/default.settings.php
   @@ -105,6 +105,10 @@
     *
   + * Drivers contained in contributed or custom modules need to set the
   + * "namespace" and "namespace_dir" properties. This allows their database driver
   + * to cache the container.
   + *
   

   It's the configuration that needs to set it rather than the driver? Maybe 'need to have the ... properties set'?

2. +++ b/core/includes/install.inc
   @@ -177,11 +180,37 @@ function drupal_get_database_types() {
   +
   +  /** @var \Composer\Autoload\ClassLoader $class_loader */
   +  $class_loader = \Drupal::service('class_loader');
   +  // We cannot use the file cache because it does not always exists.
   +  $extension_discovery = new ExtensionDiscovery(DRUPAL_ROOT, FALSE, []);
   +  $modules = $extension_discovery->scan('module', $including_test_drivers);
   +  foreach ($modules as $module) {
   

   nit: s/exists/exist/

3. +++ b/core/includes/install.inc
   @@ -177,11 +180,37 @@ function drupal_get_database_types() {
   +      $driver_files = $file_system->scanDirectory($module_driver_path, $mask, ['recurse' => FALSE]);
   +      foreach ($driver_files as $driver_file) {
   +        // We are testing every module if they have the file Tasks.php in the
   +        // directory MODULE_ROOT/src/DatabaseDriver/{$driver}/Install/. The
   +        // chance that a non-database driver has that file in the given
   +        // directory is almost zero.
   +        $tasks_file = $module_driver_path . '/' . $driver_file->filename . '/Install/Tasks.php';
   +        if (file_exists($tasks_file)) {
   

   Would it be worth a follow-up to add an optional interface for Tasks.php, making it required in Drupal 10?

4. +++ b/core/modules/system/tests/modules/driver_test/src/DatabaseDriver/DrivertestMysql/Update.php
   @@ -0,0 +1,10 @@
   +use Drupal\Core\Database\Driver\mysql\Update as CoreUpdate;
   +
   +/**
   + * MySQL test implementation of \Drupal\Core\Database\Query\Update.
   + */
   +class Update extends CoreUpdate {}
   

   So is this necessary for all contrib drivers now?

Thanks for the careful updates for the technical changes.

I've read the IS, CR, and release note carefully (not the patch), and it's still not clear to me whether existing drivers that don't use the new fixed location will continue to work. That's the big BC break that we would need to worry about here. Do we support both the old and new ways, or are existing drivers that are not affected by or have worked around this issue going to break? Is the old way deprecated, etc.? We should specifically address that in the release note and the beginning of the CR.

The release note should briefly state:

1. What it was before
2. What's changed
3. Who's affected
4. What they should do, with a link to the CR that is accessible (unique, specific words in an <a> tag that are meaningful out of context, not just the URL)

Thanks!

Re #82.4 - that's not something changed by this patch. It's the current state of affairs. There are some old issues where some dubious arguments are made that this situation is a good thing but it's not changed by this patch.

Fixing #82.1 and .2

Re #83.3 well we have the abstract class \Drupal\Core\Database\Install\Tasks that all dbs currently need to extend in order to be seen in the installer. I think any discussion about interfaces in the database system needs to take in an overview of the entire API because we've got the similar classes contained in core/lib/Drupal/Core/Database/Query to deal with too... eg \Drupal\Core\Database\Query\Delete & \Drupal\Core\Database\Query\Select etc...

Re #84 Existing driver contrib / custom drivers don't have to do anything. They will continue to work as they did. If they want to take advantage of this new functionality then they can move code around and no longer require their users to copy code to DRUPAL_ROOT/drivers. That said we should open a follow-up to deprecate support for DRUPAL_ROOT/drivers and remove it in Drupal 10.

OK so #82.4 it is already necessary for contrib drivers, but the test drivers have been getting away without doing it?

I've opened a follow-up to #3123461: Deprecate support for database drivers placed in DRUPAL_ROOT/drivers

I've addressed #84 - improving the CR and release note.

As the changes in #86 are to comments and the issue management task have been addressed - back to rtbc.

@catch the database monitoring thing is an almighty hack - it's using the core drivers and then switching the namespace on them. It's completely wrong. But fixing that here whilst we have to copy so much of the driver feels very very painful. What it does in \Drupal\KernelTests\Core\Cache\EndOfTransactionQueriesTest::getDatabaseConnectionInfo() is completely unsupported and is probably only working because the driver names match core driver names. There's no way for the database_statement_monitoring_test module to be autoloadable at the time when the container is being built.

I'm afraid this needs a reroll now after commit of #2278971: Deprecate Connection::supportsTransactions().

We can move the text in default.settings.php so this applies to both 9.1.x and 9.0.x - feels like the best thing to do.

This is amazing work!

I finally had some time to read over the latest patch. I agree with limiting the scope with the follow-up issues.

@alexpott , In #91 there was a small typo error don;t, which is replaced to don't

Overall, I like the approach of #96, so removing the "Needs framework manager review" tag.

I'd like to propose some extensive (hence the large increment.txt) but relatively superficial changes, included in this patch:

1. Rename the modules' database driver namespace from DatabaseDriver to Driver\Database. Because I'd like to err on the side of fewer top-level namespaces within modules than more of them. While we might not ever come up with another kind of driver other than a database driver, if we do, then I'd like us to not have to create another top-level namespace within modules for them.
2. Rename the 'namespace_dir' connection info key to 'autoload'. Partly to avoid using an abbreviation (which we try to avoid within Drupal), and partly for nominal parity with the 'autoload' key in composer.json (though in Composer, it's a structure in order to support more features, whereas here we only need a string).
3. Rename getModuleNamespaceDir() to findDriverAutoloadDirectory(). In particular there, "find" rather than "get" to surface the distinction between just reading the 'autoload' key vs. finding what should populate it.
4. Remove the addition of the $including_test_drivers parameter. I don't see why we need it. ExtensionDiscovery::scan() already checks a setting and a test runner agent, which I think should be sufficient. If this patch fails some tests, I'd like to try to fix them via those already existing mechanisms.
5. Replace the regular expressions on namespaces with helper methods that just call explode().
6. Remove/simplify some comments and expand others.

@alexpott, @daffie: I hope you like these changes, but if not, we can adjust whatever we need to.

I also have some questions, but I'll post those in a separate comment.

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -457,18 +462,167 @@ public static function convertDbUrlToConnectionInfo($url, $root) {
+  public static function getDatabaseClass($driver, $module, $class) {
+    $namespaces = [];
+    if ($module) {
+      // Drupal 9 style module supplied driver namespace.
+      $namespaces[] = "Drupal\\{$module}\\Driver\\Database\\{$driver}\\$class";
+    }
+    // Drupal 8 style custom driver namespace.
+    $namespaces[] = "Drupal\\Driver\\Database\\{$driver}\\$class";
+    // Core drivers are checked last.
+    $namespaces[] = "Drupal\\Core\\Database\\Driver\\{$driver}\\$class";
+    foreach ($namespaces as $namespace) {
+      if (class_exists($namespace)) {
+        return $namespace;
       }
     }
+    return NULL;
+  }

I have a few questions about this method:

1. It's only called from one place, so is it premature to add it, especially with such a generic name? Maybe better to inline it for now, and have a separate issue to provide it as an API for other use cases? If we really want to have it separate for code readability, can we make it private to start with and then a separate issue to open it up? Mostly, I want us to be free to change its name once we get clearer on its purpose.
2. From the one place where it is called, is this the logic we want? In particular, if $module is passed in the db URL, are we sure we want to fall back to code that isn't in the module? Or would it be better to error out in that case?

Condensed the issue summary and updated it for #98.

1. Re #98.1 - @daffie and I had discussed DatabaseDriver vs Driver\Database. quite a bit above. I'm ambivalent on the change but @daffie was strongly in favour of DatabaseDriver. It'd be great if in future we have a discussion prior to making a large change like this so we get everyone on board. This patch evolved through a lot of discussion between @daffie and I and more voices are definitely appreciated but let's try to hash things out first.
2. #98.2 - I like autoload - it does what it says. Nice.
3. #98.3 - +1 nice change.
4. #98.4 Nice spot - less complexity in runtime code BUT...
   

   +++ b/core/tests/Drupal/Tests/Core/Database/DatabaseTest.php
   @@ -1,146 +1,177 @@
   +      // Mock the container so we don't need to mock drupal_valid_test_ua().
   +      // @see \Drupal\Core\Extension\ExtensionDiscovery::scan()
   +      $this->root = dirname(dirname(dirname(dirname(dirname(dirname(__DIR__))))));
   ...
   +namespace {
   +
   +  if (!function_exists('drupal_valid_test_ua')) {
   +
   +    function drupal_valid_test_ua($new_prefix = NULL)
   +    {
   +      return FALSE;
   +    }
   +
      }
   

   For me, this is almost a reason to pass in $including_test_drivers ... also now the code in setUp is not needed. Fortunately we can achieve what we need and only affect a single test case rather than the entire test.

5. #98.5.... hmmm...
   

   +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -636,4 +674,62 @@ protected static function getDatabaseDriverNamespace(array $connection_info) {
   +  /**
   +   * Checks whether a namespace is within the namespace of a Drupal module.
   +   *
   +   * This can be used to determine if a database driver's namespace is provided
   +   * by a Drupal module.
   +   *
   +   * @param string $namespace
   +   *   The namespace (for example, of a database driver) to check.
   +   *
   +   * @return bool
   +   *   TRUE if the passed in namespace is a sub-namespace of a Drupal module's
   +   *   namespace.
   +   */
   +  protected static function isWithinModuleNamespace(string $namespace) {
   +    [$first, $second, $rest] = explode('\\', $namespace, 3);
   +
   +    // The namespace for Drupal modules is Drupal\MODULE_NAME, and the module
   +    // name must be all lowercase. Second-level namespaces containing uppercase
   +    // letters (e.g., "Core", "Component", "Driver") are not modules.
   +    // @see \Drupal\Core\DrupalKernel::getModuleNamespacesPsr4()
   +    // @see https://www.drupal.org/docs/8/creating-custom-modules/naming-and-placing-your-drupal-8-module#s-name-your-module
   +    return ($first === 'Drupal' && strtolower($second) === $second);
   +  }
   

   I'm not a huge fan of this change. I really don't like relying on case to determine whether a namespace is a module. I'd rather relax this to saying everything is a module that's not 'Core' or 'Driver'. But if we do keep the above formulation then is the Database class the right place for this? This feels like it is more like a special fact of Drupal's autoloading than something that is database specific. This needs more discussion. Leaving alone for now.

   I've inlined getModuleNameFromNamespace() and getModuleRelativeNamespace() - having these as separate functions is not necessary and pollutes the Database API for little reason.
   Performance really is never a worry here as parsing from a URL is never really done - as I've pointed out quite a few times on this issue. Also we should completely remove parsing from a URL and replace with a JSON string - again I've pointed this out many times. So added protected API surface here is also unwarranted.

6. #99.1 yeah lets inline it - that addresses a concern I had with this method and \Drupal\Core\Database\Connection::getDriverClass()
7. #99.2 We already error in this case so I've tighten up the code and added a specific test.

Re #103

1. Fixed in #104
2. We were already doing this. We do this to provide the module to database drivers implementations of \Drupal\Core\Database\Connection::createUrlFromConnectionOptions() - I've documented this.
3. I've removed getModuleNameFromNamespace and getModuleRelativeNamespace in #104. I agree that we should have more testing of isWithinModuleNamespace() is we move it somewhere more appropriate and make it public. Atm it is protected so is not an API to be tested and we have test coverage of it in all the other tests added by this patch.
4. Good spot - fixed.
5. Fixed in #104
6. Fixed in #104

Patch attached also removes some duplicate @internal documentation.

Forgot to actually remove Database::getDatabaseClass(). Done that now... and a little bit more clean-up of the code.

I'm debating a lot whether ww should inline \Drupal\Core\Database\Database::isWithinModuleNamespace() because the only usage outside \Drupal\Core\Database\Database::findDriverAutoloadDirectory() is in \Drupal\Core\Database\Database::getConnectionInfoAsUrl() and in D9 we should deprecation DB connections as URLs and replace them with JSON strings. Opened #3125257: Deprecate support for database connections as URLs and replace with JSON strings to do this.

Also providing a 9.1.x patch as the recent changes reverted #91. Whilst it is possible to create a patch that applies to both branches I think the location of the documentation is better so let's take the pain of rolling against multiple branches.

Whoops forgot to pull locally on 9.1.x. That's why the 9.1.x patch above contains unexpected changes.

I've refactored \Drupal\Core\Database\Database::convertDbUrlToConnectionInfo() further to account for removing Database::getDatabaseClass() - there's now less change from HEAD.

Re #107 nope - that's just fine... you can dereference an array like that an ignore the first element.

I've updated the issue summary and the change record for the latest changes.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1660,10 +1660,6 @@ public function __sleep() {
-   * @internal
-   *   This method should not be called. Use
-   *   \Drupal\Core\Database\Database::convertDbUrlToConnectionInfo() instead.
-   *
+   * @internal
+   *   This method should only be called from
+   *   \Drupal\Core\Database\Database::convertDbUrlToConnectionInfo().
+   *

I missed from my earlier review that marking this (and the other method) @internal is not a change. Removing it from the list of changes in the IS.

#109 looks great to me. I like all of the improvements that are in there since #101.

I think the only feedback that hasn't been addressed yet is:

1. +++ b/core/assets/scaffold/files/default.settings.php
   @@ -224,6 +234,20 @@
   + *     'namespace' => 'Drupal\mymodule\Driver\Database\mydriver',
   + *     'autoload' => 'modules/mymodule/src/Driver/Database/mydriver/',
   

   I agree with #104.1 that I should not unilaterally decide on Driver\Database vs. DatabaseDriver. I'm ok with being overruled on that and changing it back to DatabaseDriver. If we decide on that, then I'm willing to update the patch to do it, and I think it's worth getting right the first time, because once we mint a new top-level namespace for Drupal modules, it's disruptive to change later. I'm unclear on what the benefit of DatabaseDriver is. Is it a concern that it's premature to pretend that Driver is a generic concept, when database drivers are our only type of driver?

2. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -511,4 +637,28 @@ protected static function getDatabaseDriverNamespace(array $connection_info) {
   +  protected static function isWithinModuleNamespace(string $namespace) {
   +    [$first, $second] = explode('\\', $namespace, 3);
   +
   +    // The namespace for Drupal modules is Drupal\MODULE_NAME, and the module
   +    // name must be all lowercase. Second-level namespaces containing uppercase
   +    // letters (e.g., "Core", "Component", "Driver") are not modules.
   +    // @see \Drupal\Core\DrupalKernel::getModuleNamespacesPsr4()
   +    // @see https://www.drupal.org/docs/8/creating-custom-modules/naming-and-placing-your-drupal-8-module#s-name-your-module
   +    return ($first === 'Drupal' && strtolower($second) === $second);
   +  }
   

   #104.5 suggests checking based on an explicit list of non-module names (like "Core" and "Driver") rather than reserving ahead of time all non-lowercase names. And also, maybe moving this out of the Database class. I agree with moving it out of the Database class, and maybe making it a public static method in Extension (in which case, isWithinExtensionNamespace() would be the more accurate name), but wasn't sure if I wanted to add another API addition to this issue. Regardless, we do need to decide on whether to do this by casing or by an explicit list of non-module names.

The only benefit of DatabaseDriver as far as I know is less directory structure. The other thing is that Drupal\MODULE\Driver\Database is the opposite of Drupal\Core\Database\Driver - which is a shame. In many ways I'm not sure it matters too much what we decide tbh. There are other types of drivers in Drupal and even in core - foro example \Drupal\FunctionalJavascriptTests\DrupalSelenium2Driver - so let's go with Drupal\MODULE\Driver\Database.

Re #112.2 given #3125257: Deprecate support for database connections as URLs and replace with JSON strings I'm more in favour of inlining the logic - less API decisions.

I've inlined Database::isWithinModuleNamespace() - if we want to add this to Extension or some other class we can do that in a follow-up.

It's called from two places, which makes it less appealing to inline. Can we bring it back as a helper method but make it private and add a @todo linking to #3125476: Add an API for determining whether a given PHP namespace or class name is within the namespace of an extension?

Here I have tried to address comment #116.

Re #115 - okay let's split the difference at private. I'm still not 100% convinced given I'm not sure we should add the API #3125476: Add an API for determining whether a given PHP namespace or class name is within the namespace of an extension - in an ideal world we would not have to any of this and use composer 100% for class loading. Doing composer require drupal/MODULE and that not affecting the class loader is very surprising to PHP developers coming from Symfony and other frameworks. But in order to fix that we have to revisit what installing a module is... so not a lot of appetite for that. But at least if this is private and we deprecate and then remove db connections as URLs we can inline this again.

@daffie yeah that was bothering me too. I've improved the test to make everything a bit more obvious and less duplication.

@ravi.shankar sorry but this is an x-post.

This is something that would normally be minor-only, and hence for 9.1.x. However this is primarily intended to support being able to install the compatibility database drivers for Drupal 9.0.x more easily (i.e. for older versions of postgres, mysql, sqlite). While it adds a new API feature, it is not adding a lot of new API surface that any code is going to call, it's just about enabling different driver layouts and locations.

So untagging for release manager review, I think it's OK for this to go into 9.0.x. Did not do a full review of the patch for everything else yet so leaving status unchanged.

I condensed the CR: https://www.drupal.org/node/3123251/revisions/view/11812745/11815140

Adding issue credits.

Thank you, for everyone's great work on this, especially @daffie and @alexpott!

As mentioned in #98, the changes I posted there were all superficial, and have either been approved by @daffie and @alexpott or changed by them since then. Therefore, I still feel eligible to commit this.

Pushed to 9.1.x and cherry picked (with minor conflict resolution) to 9.0.x and 8.9.x. The reason for 8.9.x is that we're still trying to keep that and 9.0.x as close to identical as possible, other than deprecation removals and library updates. Also, this way a contrib database driver can take advantage of the new namespace and be simultaneously compatible with 8.9 and 9.0, to facilitate an upgrade from one to the other.

Tagging for including in release notes. The IS already includes the note to add.