Problem/Motivation

The BC layer introduced in Drupal 9.4 to deal with old style database settings fails on database replicas. It triggers a warning:

Warning: Undefined array key "driver" in Drupal\Core\Site\Settings::initialize() (line 169 of /var/www/html/web/core/lib/Drupal/Core/Site/Settings.php).

Drupal\Core\Site\Settings::initialize('/var/www/html/web', 'sites/default', Object) (Line: 1065)
Drupal\Core\DrupalKernel->initializeSettings(Object) (Line: 699)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

The critical part is that drivers or namespace settings specifically for one or more replicas are ignored and a potentially required autoloading doesn't happen.

In addition the still existing Drupal 7 settings BC code is now dead code because the Drupal 8 BC code causes the condition to be always FALSE.

Steps to reproduce

Add a replica according to the docs and you'll get the warning. Add a custom driver for the replica and the autoloader will not be triggered.

Proposed resolution

Re-organize the code as it was in 9.3 and apply the BC logic after the replica settings have been parsed and converted.

Another advantage will be that database related code is back in Database.php instead of Settings.php.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mkalkbrenner created an issue. See original summary.

mkalkbrenner’s picture

Status: Active » Needs review
FileSize
7.29 KB
hkirsman’s picture

The patch worked for me.

pandaski’s picture

Status: Needs review » Reviewed & tested by the community

I confirmed this is raising the warning

$databases['default']['replica'][]

[warning] Undefined array key "driver" Settings.php:169
[warning] Undefined array key "namespace" Settings.php:187

I also tested the patch in the local environment, it can fix the issue.

catch’s picture

The patch looks fine, but is this something we could add a test for?

longwave’s picture

I think it should be possible to extend ExistingDrupal8StyleDatabaseConnectionInSettingsPhpTest to do this, though it would need to add a replica connection instead of just replacing existing settings.

daffie’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.05 KB
9.35 KB

Added the testing to ExistingDrupal8StyleDatabaseConnectionInSettingsPhpTest.

The only change is the added testing.

daffie’s picture

The last submitted patch, 9: 3294695-9-test-only-should-fail.patch, failed testing. View results

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @daffie.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Looks like we also need a 10.x version of the patch.

longwave’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
9.31 KB
longwave’s picture

Issue tags: -Needs reroll
daffie’s picture

Status: Reviewed & tested by the community » Needs work

I made the test by mistake to only work for MySQL. It should also work for PostgreSQL and SQLite.

longwave’s picture

daffie’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/tests/Drupal/FunctionalTests/ExistingDrupal8StyleDatabaseConnectionInSettingsPhpTest.php
@@ -35,6 +36,19 @@ protected function setUp(): void {
+    $contents .= "  'driver' => 'mysql',\n";

It a bit strange to me that the test works for PostgreSQL and SQLite with the driver hardcoded to "mysql." Maybe we should change it. It also can go in as is, therefor for me it is RTBC.

  • catch committed bd3a600 on 10.0.x
    Issue #3294695 by daffie, longwave, mkalkbrenner, pandaski: Drupal 8 BC...
  • catch committed e129a5b on 10.1.x
    Issue #3294695 by daffie, longwave, mkalkbrenner, pandaski: Drupal 8 BC...
  • catch committed 9d3ad41 on 9.4.x
    Issue #3294695 by daffie, longwave, mkalkbrenner, pandaski: Drupal 8 BC...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed/cherry-picked the respective patches to the respective branches for 10.1.x down to 9.4.x, thanks!

  • catch committed e7ef291 on 9.5.x
    Issue #3294695 by daffie, longwave, mkalkbrenner, pandaski: Drupal 8 BC...
longwave’s picture

Re #17 oh I was so busy looking at the namespace line I didn't even see that! But it doesn't seem to matter so this will do for the test.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -286,12 +318,28 @@ abstract class Database {
+        $class_loader->addPsr4($info['namespace'] . '\\', $app_root . '/' . $info['autoload']);
+++ b/core/lib/Drupal/Core/Site/Settings.php
@@ -160,57 +160,7 @@ public static function initialize($app_root, $site_path, &$class_loader) {
-          $class_loader->addPsr4($info['namespace'] . '\\', $app_root . '/' . $info['autoload']);

I don't yet know why moving this line caused a regression for people updating from 9.4.6 to 9.4.7 or higher, but it appears to have.

With 9.4.8, if my $databases info does not contain an 'autoload' key and I run drush status, I get:

Error: Cannot instantiate abstract class Drupal\Core\Database\Schema in Drupal\Core\Database\Connection->schema() (line 1386 of /path/to/docroot/core/lib/Drupal/Core/Database/Connection.php).

I suspect the reason to be that the class_exists($driver_class) line in getDriverClass() fails due to the autoloader not containing the mysql module's directory, so therefore this falls back to the abstract Schema class that can't be instantiated.

If I patch -p1 -R < 3294695-16-9.5.x.patch the patch in #16, then drush status works as expected. See also #3290924-24: [regression] With Drupal 9.4, can no longer call Database::getConnection() from within settings.php due to driver classes not yet in autoloader where this is being reported.

I have not yet traced why Drush's bootstrap doesn't reach the code in the location that this issue moved it to.

effulgentsia’s picture

I don't yet know why moving this line caused a regression

The reason is that the new location is hidden in an if block.

Before, in Settings::initialize():

Database::addConnectionInfo($key, $target, $info);
if (isset($info['autoload'])) {
  $class_loader->addPsr4($info['namespace'] . '\\', $app_root . '/' . $info['autoload']);
}

After, in Database::addConnectionInfo():

if (empty(self::$databaseInfo[$key][$target])) {
  ...
  if (isset($info['autoload']) && $class_loader && $app_root) {
    $class_loader->addPsr4($info['namespace'] . '\\', $app_root . '/' . $info['autoload']);
  }
}

What this means is that if code within settings.php called Database::addConnectionInfo() or Database::setMultipleConnectionInfo() first, without passing in $class_loader, then later when Settings::initialize() continues, prior to this issue it would invoke the ->addPsr4() method, but now that if (empty(self::$databaseInfo[$key][$target])) { stops that.

That makes this regression a slightly different variant of #3290924: [regression] With Drupal 9.4, can no longer call Database::getConnection() from within settings.php due to driver classes not yet in autoloader and can be dealt with there rather than re-opening this issue or creating a new one.