Always set $info['namespace'] on database connection info

If I understand this patch correctly, the namespace part of the driver "settings" will become required. Is this what we want?

Actually, I think that as things go atm, the 'namespace' key would be the important one, and maybe the 'driver' one is not that necessary anymore...

See Database::openConnection, the $driver variable is unused.

Sorry, to you @mondrake. I was not talking about the "driver" setting, but I was talking database driver "settings". Just like @alexpott is talking about it in comment #5.

#7 @daffie exactly, I am pushing further and IMHO the 'driver' key in the database settings is redundant and this would suffice:

 * @code
 * $databases['default']['default'] = [
 *   'database' => 'databasename',
 *   'username' => 'sqlusername',
 *   'password' => 'sqlpassword',
 *   'host' => 'localhost',
 *   'port' => '3306',
 *   'namespace' => 'Drupal\Core\Database\Driver\mysql',
 *   'prefix' => '',
 *   'collation' => 'utf8mb4_general_ci',
 * ];
 * @endcode

:)

Then if you need a label for the driver, you can get it from Connection::driver()...

For me is making the namespace setting required very bad for the Drupal end user/site owner. The driver setting is simple for them to understand and changing that will require a lot of communication and also the namespace setting is more difficult. Therefore am I against this change.

For me this issue is a small part of a bigger problem. Drupal is divided in different parts. Everything in the directory /core/lib is/should be general code for that is not part of a specific module or theme. The specific code for a module or theme belongs in that module or theme. For database drivers this is different. All code is in the directory /core/lib, including all database driver specific code. For me this is wrong. Just like with modules and themes, the database driver specific stuff needs to be moved to core/databases or for contrib to /databases.
There are in Drupal core a lot places where there is code that starts with something likeif (Database::getConnection()->databaseType() == 'mysql/pgsql/sqlite') {, not only in code but also in tests. Those code parts should be moved to their specific database driver. It should not be anywhere else. For that database drivers need module like capabilities. Just like themes are a template directory with module capabilities, database drivers should be a driver directory with module like capabilities. Just like the contrib database drivers are that already. The three by code supported should be changed in the same way.
Problems like contrib database drivers that need some test to do something different now cannot do that. They cannot make changes to test like the by core supported drivers can (see: #3110546: Allow contributed modules (mostly database drivers) to override tests in core).
The problem we have in this issue should then be solved very easily. Lets start by assuming that we will require that the driver name and the module name must be the same. The namespace for any database driver will then be Drupal\$module_name\Driver.

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -224,6 +224,10 @@ abstract class Database {
+    // Fallback for Drupal 7 settings.php if namespace is not provided.
+    if (empty($info['namespace'])) {
+      $info['namespace'] = 'Drupal\\Core\\Database\\Driver\\' . $info['driver'];
+    }

If I understand this change correctly, if the (contrib) database driver uses another namespace then the default we are adding here, you are required to set the namespace part in the driver "settings". This is to me not the right solution.

@alexpott: You are right. I agree with this baby step. Therefor for me it is RTBC.

This change is just a baby step towards all drivers declaring a namespace by ensuring that all drivers always have the namespace key set

I am not sure what you mean:
1. Do want to have all drivers add a method that will return its namespace. Something like Connection::getNamespace(). If this is want you mean then lets do this in 9.0 as is a BC break.
Also add a hook_requirements that tests if the namespace key in the "driver settings" is set and if that is the case then return a REQUIREMENT_WARNING with the message that the setting of the namespace will be ignored.
2. If we want for all drivers that users must set the namespace key in the "driver settings" then that would be for me bad for the Drupal end user/site owner experience. For me the Drupal end user/site owner should have nothing to do with the namespace of the database driver. It will be confusing to a lot of them and this will not be good for the usability of Drupal. Please lets not do this.

IMHO this can be done now, without the deprecation, possibly also backported to D8. Then, deprecate it later in D9.1, with a test that is missing here.

Doing #16.

@mondrake: Could you create the followup for deprecating the method getDatabaseDriverNamespace() and updating the patch with the link to the followup. For me it would be RTBC again.

I find the changes in the Log tests OOS here, and a bit difficult to follow. I understand that current state can be improved, but I was finding having an sample backtrace easier to read and follow. My suggestion would be to defer the Log test changes to #2999962: Unify Database/Log::findCaller and Utility/Error::getLastCaller and just get rid of the usage of reflection in Log::getDriverNamespace here.

Reviewed as requested on Slack.

1. +++ b/core/lib/Drupal/Core/Database/Database.php
   @@ -502,6 +508,8 @@ public static function getConnectionInfoAsUrl($key = 'default') {
   +   * @todo https://www.drupal.org/node/3115388 Deprecate this in drupal:9.1.0.
   

   Should we do deprecate this in 9.0 and remove in 10.0?

2. +++ b/core/lib/Drupal/Core/Database/Log.php
   @@ -181,31 +176,4 @@ public function findCaller() {
   -  /**
   -   * Gets the debug backtrace.
   -   *
   -   * Wraps the debug_backtrace function to allow mocking results in PHPUnit
   -   * tests.
   -   *
   -   * @return array[]
   -   *   The debug backtrace.
   -   */
   -  protected function getDebugBacktrace() {
   -    return debug_backtrace();
   -  }
   

   I do not think that this should be removed in this issue.

3. +++ b/core/tests/Drupal/KernelTests/Core/Database/LoggingTest.php
   --- /dev/null
   +++ b/core/tests/Drupal/Tests/Core/Database/LogTest.php
   

   I think we should test with "full" stack. This test does not give me the confidence that the Log class will do what it should do.

I think that the Log testing can be improved. But for me it does not belong in this issue. I agree with @mondrake. Lets do that in #2999962: Unify Database/Log::findCaller and Utility/Error::getLastCaller.

I tested #22 with a contrib driver, and --group Database,views tests pass. OK for RTBC for me, but I cannot. Would be good to have input from guys developing MSSQL or Oracle contrib drivers.

Apparently it is policy not to deprecate in 9.0. The added comment is therefor correct.
The removal of the backtrace method has been removed from the patch.
We are going to improve the testing of Database/Log in #2999962: Unify Database/Log::findCaller and Utility/Error::getLastCaller. Which is great.
The changing of the namespace stuff is not a problem for a contrib driver. See the coment of @mondrake in comment #31.
The testbot failure is not related to this patch.
All code changes look good to me.
For me it is RTBC.

Back to RTBC.

Hi @Neslee: The interdiff file you have added does not correspond with the patch file changes. Your interdiff file let me to believe that you where making fundamental changes to the patch, also because you did not remove the reroll tag.

The patch from comment #32 needs a reroll, because there is one hunk that is failing. You have updated that hunk, only not in right way. The original hunk was:

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -488,7 +494,7 @@ public static function getConnectionInfoAsUrl($key = 'default') {
     if (empty($db_info) || empty($db_info['default'])) {
       throw new \RuntimeException("Database connection $key not defined or missing the 'default' settings");
     }
-    $connection_class = static::getDatabaseDriverNamespace($db_info['default']) . '\\Connection';
+    $connection_class = $db_info['default']['namespace'] . '\\Connection';
     return $connection_class::createUrlFromConnectionOptions($db_info['default']);
   }
 

You have changed it into:

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -614,7 +620,7 @@ public static function getConnectionInfoAsUrl($key = 'default') {
       $db_info['default']['module'] = explode('\\', $namespace)[1];
     }
 
-    $connection_class = $namespace . '\\Connection';
+    $connection_class = $db_info['default']['namespace'] . '\\Connection';
     return $connection_class::createUrlFromConnectionOptions($db_info['default']);
   }
 

The essence of the change was to remove the calling of static::getDatabaseDriverNamespace() and replace it by $db_info['default']['namespace'].

@daffie, ya i have rerolled the #32.

The essence of the change was to remove the calling of static::getDatabaseDriverNamespace() and replace it by $db_info['default']['namespace'].

@Neslee: The change you made in your reroll was wrong, because your change does not do that and it should do that.

In the method Drupal\Core\Database\Database::getDatabaseDriverNamespace() there is a call to static::getDatabaseDriverNamespace. That call needs to be replaced by $db_info['default']['namespace']. Just like in the patch from @alexpott in comment #32.

@daffie, got your point, updated the patch. Thanks for mentoring👍🏻

Reroll looks good.
Back to RTBC.

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -628,6 +634,8 @@ public static function getConnectionInfoAsUrl($key = 'default') {
    *
    * @return string
    *   The PHP namespace of the driver's database.
+   *
+   * @todo https://www.drupal.org/node/3115388 Deprecate this in drupal:9.1.0.
    */

It's now possible to do the 9.1.x deprecation if we have a separate patch.

@catch the deprecation is the follow-up to this patch

Or do you mean not split it?

@andypost I mean not splitting it, the follow-up was created when the 9.1.x branch wasn't open yet (but after we stopped committing new deprecations to 9.0.x).

The patch for 9.0 is for me RTBC. The one for 9.0 is missing one small thing:

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -628,8 +634,14 @@ public static function getConnectionInfoAsUrl($key = 'default') {
+    @trigger_error(__METHOD__ . " is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. There is no replacement as \$connection_info['namespace'] is always set. See https://www.drupal.org/node/3127769.", E_USER_DEPRECATED);

I am missing a test for this deprecation message.

I thought that we should have testing for all deprecation message's, only @alexpott is saying that in this case it is not necessary.
No problem with the removal of the unnecessary setting of $this->connectionOptions in SQLite driver.
All code changes look good to me.
The testbot is happy.
For me it is RTBC.

We never add tests for constructor deprecations, agreed in this case too where there's no actual code path to test.

Committed 2989dd0 and pushed to 9.1.x. Thanks!

Committed aebcf1d and pushed to 9.0.x. Thanks!

Publish the change record