Problem/Motivation

#3129043: Move core database drivers to modules of their own moved core's database drivers into modules. As part of that, it added backwards compatibility in Settings::initialize() to autoload from those module directories. However, it added that autoloading too aggressively, because it's also doing it for drivers with the namespace Drupal\Driver\..., if the driver is one of "mysql", "pgsql", or "sqlite".

https://www.drupal.org/project/mysql56 is an example of a project that provides a Drupal\Driver\... implementation of a "mysql" driver. This project works in Drupal 9.3, but fails in Drupal 9.4 due to incorrect autoloading being added for this project's namespace into the core/modules/mysql/... directory.

Steps to reproduce

Proposed resolution

Change the BC autoloading to only apply for the correct (i.e., "Drupal\{$driver}") namespace.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#10 diff.txt590 byteseffulgentsia
#10 3284502-10.0.x.patch8.02 KBeffulgentsia
#9 interdiff.txt4.14 KBeffulgentsia
#9 3284502-9.patch8.01 KBeffulgentsia
#3 3284502-3-with-fix.patch5.43 KBeffulgentsia
#3 3284502-3-test-only.patch3.25 KBeffulgentsia
#2 dbdriver-less-aggressive-autoload.patch2.18 KBeffulgentsia

Comments

effulgentsia created an issue. See original summary.

effulgentsia’s picture

effulgentsia commented
StatusFileSize
new dbdriver-less-aggressive-autoload.patch2.18 KB
effulgentsia’s picture

effulgentsia commented
StatusFileSize
new 3284502-3-test-only.patch3.25 KB
new 3284502-3-with-fix.patch5.43 KB

Here's a test-only patch that proves the bug, and a patch that combines the test with the fix in #2.

The last submitted patch, 3: 3284502-3-test-only.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

I saw this yesterday and thought about asking for test coverage, but didn't think it was going to be easy - but your neat unit test proves me wrong!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Site/Settings.php
@@ -198,6 +180,17 @@ public static function initialize($app_root, $site_path, &$class_loader) {
+          foreach (['mysql', 'pgsql', 'sqlite'] as $driver) {
+            $module = $driver;
+            if ((strtolower($info['driver']) === $driver) && (trim($info['namespace'], '\\') === "Drupal\\{$module}\\Driver\\Database\\{$driver}")) {
+              $info['autoload'] = "core/modules/{$module}/src/Driver/Database/{$driver}/";
+            }
+          }

The previous switch construction avoids unnecessary loops if you don't have autoload set. Let's go back to that.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

We also need a patch for 10.0.x because this code is still on that branch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Priority: Normal » Critical

As per @xjm this is at least a major regression. It feels critical if you are using https://www.drupal.org/project/mysql56 as you will not be able to update to 9.4.

effulgentsia’s picture

effulgentsia commented
Status: Needs work » Needs review
StatusFileSize
new 3284502-9.patch8.01 KB
new interdiff.txt4.14 KB

This changes the loop to a switch/case per #6. Because that creates a little code duplication, I also duplicated the test cases to make sure pgsql and sqlite are similarly tested.

effulgentsia’s picture

effulgentsia commented
StatusFileSize
new 3284502-10.0.x.patch8.02 KB
new diff.txt590 bytes

Here's the 10.0.x patch as well. It only differs by context lines, as shown by the diff.

effulgentsia’s picture

effulgentsia commented

It feels critical if you are using https://www.drupal.org/project/mysql56 as you will not be able to update to 9.4.

Yeah, I tagged it as a soft blocker, because the workaround would be for sites using that module to fill in the 'autoload' key in their settings.php, but it would be annoying for each site to have to do that (and otherwise unnecessary since that module already adds itself to the autoloader separately), so getting this committed to 9.4.0 would be much appreciated!

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

All the code changes look good to me.
Testing has been added.
The points of @alexpott have been addressed.
For me it is RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 85120a6 and pushed to 10.0.x. Thanks!
Committed and pushed 145af574f3 to 9.5.x and 5a6aa7be7d to 9.4.x. Thanks!

  • alexpott committed d3f1a82 on 10.0.x 
    Issue #3284502 by effulgentsia, alexpott: [regression] Drupal 9.4 breaks...

  • alexpott committed 145af57 on 9.5.x 
    Issue #3284502 by effulgentsia, alexpott: [regression] Drupal 9.4 breaks...

  • alexpott committed 5a6aa7b on 9.4.x 
    Issue #3284502 by effulgentsia, alexpott: [regression] Drupal 9.4 breaks...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Opened #3284970: Reduce complexity in \Drupal\Core\Site\Settings::initialize as a quick follow after thinking about the changes here some more.

Status: Fixed » Closed (fixed)

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