Problem/Motivation

#3112476: Always set $info['namespace'] on database connection info results in \Drupal\Core\Database\Database::getDatabaseDriverNamespace not being needed. Let's deprecate it for removal in D10.

Also we should deprecate calling \Drupal\Core\Database\Connection::__construct() without a 'namespace' key in the $connection_options array. Which will allow us to simplify \Drupal\Core\Database\Connection::getDriverClass and remove the override in \Drupal\database_statement_monitoring_test\LoggedStatementsTrait

Proposed resolution

Do it.

Remaining tasks

User interface changes

None

API changes

\Drupal\Core\Database\Database::getDatabaseDriverNamespace is deprecated.

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#20 interdiff-19-20-3115388.txt1.96 KBsalah1
#20 3115388-20-deprecate-getDatabaseDriverNamespace.patch6.37 KBsalah1
#19 interdiff-16-19-3115388.txt2.67 KBsalah1
#19 3115388-19-deprecate-getDatabaseDriverNamespace.patch4.72 KBsalah1
#16 3115388-16-deprecate-getDatabaseDriverNamespace.patch2.05 KBsalah1
#16 interdiff-9-16-3115388.txt1.11 KBsalah1
#11 3115388-23.patch1.17 KBneelam_wadhwani
#9 3115388-9-deprecate-getDatabaseDriverNamespace.patch1.88 KBsalah1
#9 interdiff-8-9-3115388.txt2.03 KBsalah1
#8 interdiff-6-7-3115388.txt1.01 KBsalah1
#8 3115388-7-deprecate-getDatabaseDriverNamespace.patch2 KBsalah1
#6 3115388-6-deprecate-getDatabaseDriverNamespace.patch2.01 KBsalah1
#6 interdiff-3-6-3115388.txt2.18 KBsalah1
#3 3115388-deprecate-getDatabaseDriverNamespace.patch1.99 KBsalah1
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Title: Make methods private in \Drupal\Core\Database\Log because they are going to obsolete » Deprecate \Drupal\Core\Database\Database::getDatabaseDriverNamespace as it is no longer needed
Version: 8.9.x-dev » 9.1.x-dev
Issue summary: View changes
salah1’s picture

salah1
Location VA, U.S
CreditAttribution: salah1 as a volunteer commented
Status: Active » Needs review
FileSize
3115388-deprecate-getDatabaseDriverNamespace.patch1.99 KB

Here is patch to deprecate the method \Drupal\Core\Database\Database::getDatabaseDriverNamespace.
Thanks for reviewing it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs review » Needs work

Thanks for working on this patch.

  1. +++ b/core/lib/Drupal/Core/Database/Database.php
@@ -365,6 +365,8 @@
+    // @todo https://www.drupal.org/project/drupal/issues/3115388
+    // Remove the deprecated getDatabaseDriverNamespace  in Drupal 10.0.
     $namespace = static::getDatabaseDriverNamespace(self::$databaseInfo[$key][$target]);

@@ -488,6 +490,8 @@ public static function getConnectionInfoAsUrl($key = 'default') {
+    // @todo https://www.drupal.org/project/drupal/issues/3115388
+    // Remove the deprecated getDatabaseDriverNamespace  in Drupal 10.0.
     $connection_class = static::getDatabaseDriverNamespace($db_info['default']) . '\\Connection';

    This should actually remove the usages. 

  2. +++ b/core/lib/Drupal/Core/Database/Database.php
@@ -502,8 +506,13 @@ public static function getConnectionInfoAsUrl($key = 'default') {
+   * @deprecated in drupal:9.0.0 and is removed from drupal:10.0.0.
+   * $info['namespace] is always set now.
+   * @see https://www.drupal.org/project/drupal/issues/3115388
...
+    @trigger_error('getDatabaseDriverNamespace() is deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. $info[''namespace''] is always set now. See https://www.drupal.org/project/drupal/issues/3115388.', E_USER_DEPRECATED);

    We're trying to only add new deprecations in 9.1.0. This needs point to a change record (see link in the sidebar).

salah1’s picture

salah1
Location VA, U.S
CreditAttribution: salah1 as a volunteer commented
Assigned: Unassigned » salah1

So, you mean on 1) remove that part as it relates to usage and 2) add the change record link instead of the issue link?

salah1’s picture

salah1
Location VA, U.S
CreditAttribution: salah1 as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff-3-6-3115388.txt2.18 KB
3115388-6-deprecate-getDatabaseDriverNamespace.patch2.01 KB

Made the changes in #4(atleast the way i understood it).
Let me know if we need to tweak it more.
Thanks

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented 
+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -365,6 +365,8 @@
     $namespace = static::getDatabaseDriverNamespace(self::$databaseInfo[$key][$target]);

@@ -488,6 +490,8 @@ public static function getConnectionInfoAsUrl($key = 'default') {
     $connection_class = static::getDatabaseDriverNamespace($db_info['default']) . '\\Connection';

Here we actually need to remove the calls to the method as we've deprecated it.

salah1’s picture

salah1
Location VA, U.S
CreditAttribution: salah1 as a volunteer commented
FileSize
3115388-7-deprecate-getDatabaseDriverNamespace.patch2 KB
interdiff-6-7-3115388.txt1.01 KB

Correcting some quote issues caused the error in #3.

salah1’s picture

salah1
Location VA, U.S
CreditAttribution: salah1 as a volunteer commented
FileSize
interdiff-8-9-3115388.txt2.03 KB
3115388-9-deprecate-getDatabaseDriverNamespace.patch1.88 KB

@Alexpott, thanks for the review. Does this patch take care of the changes in #7?
Patch assumes $info['namespace'] is set already.
if $info['namespace']) is not set already, i can put in check like the following?

if (empty($info['namespace'])) {
+      $info['namespace'] = 'Drupal\\Core\\Database\\Driver\\' . $info['driver'];
+    }
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Database/Database.php
@@ -365,7 +365,7 @@
-    $namespace = static::getDatabaseDriverNamespace(self::$databaseInfo[$key][$target]);
+    $namespace = $info['namespace'];
     $driver_class = $namespace . '\\Connection';

    This should be $driver_class = self::$databaseInfo[$key][$target]['namespace'] . '\\Connnection';. There no need to assign a namespace var any more. 

  2. +++ b/core/lib/Drupal/Core/Database/Database.php
@@ -488,7 +488,8 @@ public static function getConnectionInfoAsUrl($key = 'default') {
-    $connection_class = static::getDatabaseDriverNamespace($db_info['default']) . '\\Connection';
+
+    $connection_class = $info['namespace'];

    This should be... $connection_class = $db_info['default']['namespace'] . '\\Connection';

Note that since #3112476: Always set $info['namespace'] on database connection info you don't need to check for the existence of the array key - that was the point of that change.

neelam_wadhwani’s picture

neelam_wadhwani CreditAttribution: neelam_wadhwani at Valuebound for Valuebound commented
FileSize
3115388-23.patch1.17 KB

Hello @alexpott
Done changes as asked for.
Kindly review patch.

neelam_wadhwani’s picture

neelam_wadhwani CreditAttribution: neelam_wadhwani at Valuebound for Valuebound commented
Assigned: salah1 » Unassigned
Status: Needs work » Needs review
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Title: Deprecate \Drupal\Core\Database\Database::getDatabaseDriverNamespace as it is no longer needed » PP-1: Deprecate \Drupal\Core\Database\Database::getDatabaseDriverNamespace as it is no longer needed
Status: Needs review » Postponed

Note that before we can actually do this issue we need to do #3112476: Always set $info['namespace'] on database connection info first. This is a follow-up to that one. Reviews of that issue would be appreciated.

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -366,7 +366,7 @@
+    $driver_class = self::$databaseInfo[$key][$target]['namespace'] . '\\Connnection';

Too many N's.

We still need to deprecate the method as is done in #9.

salah1’s picture

salah1
Location VA, U.S
CreditAttribution: salah1 as a volunteer commented

@neelam_wadhwani,
Thanks for helping.

I was working on this and it was assigned to me :)
Community guide says, “avoid issues being worked on and assigned to others”.

https://www.drupal.org/node/1319140
[under assign the issue...]

I only raise this because it’s better etiquette and not to duplicate work.
And it happened to me before.
Thanks for making this good experience for all.

salah1’s picture

salah1
Location VA, U.S
CreditAttribution: salah1 as a volunteer commented
Assigned: Unassigned » salah1

Will work on this.

salah1’s picture

salah1
Location VA, U.S
CreditAttribution: salah1 as a volunteer commented
Assigned: salah1 » Unassigned
FileSize
interdiff-9-16-3115388.txt1.11 KB
3115388-16-deprecate-getDatabaseDriverNamespace.patch2.05 KB

This should take care of the changes raised on #10.
I reviewed the related issue and understand context.
Just wanted to load this patch so when the issue is active again, it can move forward.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue summary: View changes

Added some more tasks to the issue summary.

salah1’s picture

salah1
Location VA, U.S
CreditAttribution: salah1 as a volunteer commented

Leaving this un-assigned so anybody with time can jump on it.

salah1’s picture

salah1
Location VA, U.S
CreditAttribution: salah1 as a volunteer commented
FileSize
3115388-19-deprecate-getDatabaseDriverNamespace.patch4.72 KB
interdiff-16-19-3115388.txt2.67 KB

Of the remaining tasks below, i crossed out the done parts.

Also we should deprecate calling \Drupal\Core\Database\Connection::__construct() without a 'namespace' key in the $connection_options array. Which will allow us to simplify \Drupal\Core\Database\Connection::getDriverClass and remove the override in \Drupal\database_statement_monitoring_test\LoggedStatementsTrait

This patch, includes few fixes for coding standards in (/core/lib/Drupal/Core/Database/Connection.php)

salah1’s picture

salah1
Location VA, U.S
CreditAttribution: salah1 as a volunteer commented
FileSize
3115388-20-deprecate-getDatabaseDriverNamespace.patch6.37 KB
interdiff-19-20-3115388.txt1.96 KB

This patch should take care of ALL the added tasks as well.
It ncludes few fixes for coding standards in (/core/lib/Drupal/Core/Database/Connection.php)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Postponed » Closed (duplicate)

As 9.1.x is now open we're doing this in #3112476: Always set $info['namespace'] on database connection info