Deprecate support for database drivers placed in DRUPAL_ROOT/drivers

A CR has been added.

Fixed the failing test Drupal\Tests\Core\Database\ConditionTest::testContribCondition(). Changed the used namespace from the deprecated D8 style to the new D9 style.

Rerolled the patch for #3126658: Support enclosing reserved words with brackets..

Thanks for the patch.

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -208,12 +208,19 @@ public function __construct(\PDO $connection, array $connection_options) {
   +    // Drupal\\Driver\\Database\\ is deprecated.
   +    if (isset($connection_options['namespace']) && (substr($connection_options['namespace'], 0, 23) === 'Drupal\Driver\Database\\')) {
   +      @trigger_error('Support for database drivers where the namespace starts with "Drupal\\\\Driver\\\\Database\\\\" are deprecated in drupal:9.1.0 and support will be removed in drupal:10.0.0. Not by Drupal core supported database drivers have to use the namespace "Drupal\\\\MODULE_NAME\\\\Driver\\\\Database\\\\DRIVER_NAME". See https://www.drupal.org/node/3123251', E_USER_DEPRECATED);
   

   a) Is it intentional that the comment and trigger error are using double backslashes but the code uses:

   'Drupal\Driver\Database\\'?

   Seems like it's missing two backslashes or the last double backslash should be a single backslash, but then I would expect the test to fail.

   UPDATE: Ok, I see how this is correct (// signifies the end of the namespace). I wonder if it's worth updating the comment to make this more clear.

   b) "Not by Drupal core supported database drivers"

   Not sure what this means.

2. +++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
   @@ -432,4 +432,17 @@ public function testIdentifierQuotesAssertString() {
   +   * Tests the deprecation of database drivers where the namespace starts with Drupal\\Driver\\Database\\.
   

   Nitpick: Wrap at 80 chars.

3. +++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
   @@ -432,4 +432,17 @@ public function testIdentifierQuotesAssertString() {
   +   * @expectedDeprecation Support for database drivers where the namespace starts with "Drupal\\Driver\\Database\\" are deprecated in drupal:9.1.0 and support will be removed in drupal:10.0.0. Not by Drupal core supported database drivers have to use the namespace "Drupal\\MODULE_NAME\\Driver\\Database\\DRIVER_NAME". See https://www.drupal.org/node/3123251
   

   Nitpick: Wrap at 80 chars? Maybe not for expectedDeprecation message.

   UPDATE: I looked at a few @expectedDeprecation messages and see they are all on one line.
   
   

4. Here is D9 dev code I found that has Drupal\Driver\Database:

   core/tests/Drupal/Tests/Core/Database/DatabaseTest.php

   'D8 custom fake' => [FALSE, 'Drupal\Driver\Database\corefake'],
   

   and

   $this->additionalClassloader->addPsr4("Drupal\\Driver\\Database\\corefake\\"...
   

   (Sidenote: Code refers to "D8").

   core/tests/Drupal/Tests/Core/Database/ConditionTest.php

   This one is updated in the patch.

   $contrib_namespace = 'Drupal\Driver\Database\mock';
   

   core/tests/Drupal/Tests/Core/Database/InstallerObjectTest.php

   use Drupal\Driver\Database\fake\Install\Tasks as FakeInstallTasks;
   use Drupal\Driver\Database\corefake\Install\Tasks as CustomCoreFakeInstallTasks;
   ...
   $additional_class_loader->addPsr4("Drupal\\Driver\\Database\\fake\\"...
   $additional_class_loader->addPsr4("Drupal\\Driver\\Database\\corefake\\"...
   

   core/tests/fixtures/database_drivers/core/corefake/Connection.php

   use Drupal\Driver\Database\fake\Connection as BaseConnection;
   

   core/tests/fixtures/database_drivers/module/corefake/src/Driver/Database/corefake/Connection.php

   use Drupal\Driver\Database\fake\Connection as BaseConnection;
   

   core/tests/fixtures/database_drivers/custom/corefake/Connection.php

   namespace Drupal\Driver\Database\corefake;
   

   core/tests/fixtures/database_drivers/custom/fake/Install/Tasks.php

   namespace Drupal\Driver\Database\fake\Install;
   

5. Applying the patch failed so maybe it needs a reroll:

   [mac:kristen:drupal-9.0.x-dev]$ patch -p1 < 3123461-6.patch
   patching file core/lib/Drupal/Core/Database/Connection.php
   Hunk #1 FAILED at 208.
   1 out of 1 hunk FAILED -- saving rejects to file core/lib/Drupal/Core/Database/Connection.php.rej
   patching file core/tests/Drupal/Tests/Core/Database/ConditionTest.php
   patching file core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
   

Back to "Needs work" for at least item 5 in #7.

@Kristen Pol: Thank you for your review.

For #7.1.a: Yes, it is intentional. In the settings.php file you have the database connection array and for contrib/custom database drivers you have the namespace key. The value for that key is something like "Drupal\\Driver\\Database\\oracle". For PHP the backslash character is a special character. In PHP it is used as the escape character. See: https://www.php.net/language.types.string. In the if-statement I made the comparison to 'Drupal\Driver\Database\\'. For PHP this will become the string: Drupal\Driver\Database\. In PHP we have the backslash character as the string escape character and we use it in namespaces. When you then start working with namespaces as strings it will become a bit confusing. I have changed the if-statement so that it will be less confusing?

For #7.1.b: Updated the test to: Contributed and custom database drivers have to use the namespace "Drupal\\MODULE_NAME\\Driver\\Database\\DRIVER_NAME"

For #7.2: Fixed.

For #7.3: That is how it works.

For #7.4: Unfortunately we are only allowed to deprecate the functionality not removing it. We shall have to wait for Drupal 10.0 before we can remove all that stuff.

For #7.5: This patch is for 9.1. We are not allowed to deprecate stuff in 9.0. The patch does not have to apply to 9.0 (mac:kristen:drupal-9.0.x-dev).

Thanks for the speedy response and great comments. Let me take a look!

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -217,8 +217,8 @@
   +    if (isset($connection_options['namespace']) && (substr($connection_options['namespace'], 0, 22) === 'Drupal\Driver\Database')) {
   +      @trigger_error('Support for database drivers where the namespace starts with "Drupal\\\\Driver\\\\Database\\\\" are deprecated in drupal:9.1.0 and support will be removed in drupal:10.0.0. Contributed and custom database drivers have to use the namespace "Drupal\\\\MODULE_NAME\\\\Driver\\\\Database\\\\DRIVER_NAME". See https://www.drupal.org/node/3123251', E_USER_DEPRECATED);
   

   Nice!

2. +++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
   @@ -433,10 +433,10 @@
   +   * Tests the deprecation of Drupal 8 style database drivers.
   

   Looks good!

3. +++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
   @@ -433,10 +433,10 @@
   +   * @expectedDeprecation Support for database drivers where the namespace starts with "Drupal\\Driver\\Database\\" are deprecated in drupal:9.1.0 and support will be removed in drupal:10.0.0. Contributed and custom database drivers have to use the namespace "Drupal\\MODULE_NAME\\Driver\\Database\\DRIVER_NAME". See https://www.drupal.org/node/3123251
   

   Looks good.

For #7.1.a: Understood. Looks good. Thanks for the changes.

For #7.1.b: Wording change looks good.

For #7.2: Truncation looks good.

For #7.3: Thanks.

For #7.4: Gotcha.

For #7.5: Whoops. Ok, applied cleanly against 9.1 branch!

[mac:kristen:drupal-9.1.x-dev]$ patch -p1 < 3123461-8_0.patch 
patching file core/lib/Drupal/Core/Database/Connection.php
patching file core/tests/Drupal/Tests/Core/Database/ConditionTest.php
patching file core/tests/Drupal/Tests/Core/Database/ConnectionTest.php

Ok, this looks RTBC to me but is there a way for me to manually test this?

@Kristen Pol: To manual test this, you have to take the database driver for OracleDB (https://www.drupal.org/project/oracle) or the one for Microsoft SQL Server (https://www.drupal.org/project/sqlsrv) and install a Drupal site. You need of course a running OracleDB or Microsoft SQL Server to do so.

Ah. Not sure I can do that as I'm on a Mac and maybe those cost money to install. Will someone need to manually test before this can be RTBCed?

Will someone need to manually test before this can be RTBCed?

Would be great is somebody did, but for me no it can be RTBC'd without. There are only a couple of contrib database drivers out there: OracleDB, MS SQL Server, MemSQL and DruDBAL. The last one is from @mondrake and he has already moved his module over to using #3120096: Support contrib database driver directories in a fixed location in a module. When this issue lands I will create an issue on all their modules issue queues informing them that they shall have to move over to using #3120096: Support contrib database driver directories in a fixed location in a module.

@Kristen Pol: By the way: Oracle has a free version for everybody: https://www.oracle.com/database/technologies/appdev/xe.html. And Microsoft has also a free version: https://www.microsoft.com/en-us/sql-server/sql-server-downloads.

I'm not sure this is a case where deprecation is enough. Drupal 8 style contrib database drivers don't get included in update status reports, so for someone who's installed one, there is no notification that a new version is available. In other words the deprecation message helps the driver authors, but not the people that need to update.

There are not loads of options, but maybe a hook_requirements() to detect when a driver is being used in the deprecated location?

Updated the patch for the comment #16.

And regardless of deprecations being on the most up-to-date module versions is recommended when you update from D9 to D10.

This is true, but database drivers aren't modules, they don't even ship with version information.

There's nothing you can do till they do - so we'd be putting something non-actionable on the status report which never goes down well

Maybe add it to 9.2, or whatever 9.LAST is in a follow-up?

1) I reviewed the patch in #19 and compared the @trigger_error and @expectedDeprecation messages against the suggestion in #16 and they match except for the quotes. The double quotes used in the patch seem like the correct approach rather than the single quotes in the suggested text based on usage in other core code.

2) One minor thing that I didn't comment on before is there is a new line added in an unrelated section that technically shouldn't be added but it doesn't seem like a big deal.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -208,12 +208,19 @@ public function __construct(\PDO $connection, array $connection_options) {
     assert(count($this->identifierQuotes) === 2 && Inspector::assertAllStrings($this->identifierQuotes), '\Drupal\Core\Database\Connection::$identifierQuotes must contain 2 string values');
+

3) The patch applied cleanly:

[mac:kristen:drupal-9.1.x-dev]$ patch -p1 < 3123461-19.patch 
patching file core/lib/Drupal/Core/Database/Connection.php
patching file core/tests/Drupal/Tests/Core/Database/ConditionTest.php
patching file core/tests/Drupal/Tests/Core/Database/ConnectionTest.php

Based on this, #15, and the discussion between @alexpott and @catch, it doesn't appear like anything else needs to happen in this patch, so marking this RTBC.

Tagging for needs follow-up, for some kind of site-admin facing message even if we do that in a later release.

Also is there an already an issue open for converting the core database drivers to extensions?

Removed the isset($connection_options['namespace']) from the if-statement.
Moved the added deprecation part to after when no namespace set that add one.

Also is there an already an issue open for converting the core database drivers to extensions?

Waiting for doing that until #3124354: Remove unnecessary classes from the database drivers has landed.

Fixed for comment #28.

Thanks for the update.

1) I have confirmed that the change in #29 addresses the requested change from #28.

2) The patch applies cleanly:

[mac:kristen:drupal-9.1.x-dev]$ patch -p1 < 3123461-29.patch
patching file core/lib/Drupal/Core/Database/Connection.php
patching file core/tests/Drupal/Tests/Core/Database/ConditionTest.php
patching file core/tests/Drupal/Tests/Core/Database/ConnectionTest.php

3) All other requested changes above have been addressed, so marking RTBC.

Also is there an already an issue open for converting the core database drivers to extensions?

Yes, @catch there is now #3129043: Move core database drivers to modules of their own.

NW for the release note, and also since the patch no longer applies. Thanks!

Oh we also still need a followup for "some kind of site-admin-facing message" per #22.

Followup created for "some kind of site-admin-facing message". See: #3153443: A site-admin-facing message for the deprecation of database drivers in DRUPAL_ROOT/drivers.

Straight reroll.

The only thing left is the "NW for the release note" from @xjm in comment #34.

Release note added.

Back to RTBC.

Back to RTBC.

Committed 5730ee6 and pushed to 9.1.x. Thanks!