Introduce database driver extensions and autoload database drivers' dependencies

Ouch… I have been bitten by this too. Yes we need to be able to add additional namespaces to the class loader very early. What I am afraid of in this patch is performance… scanning the directory to find the module at every request is not very fancy. Need to think about it.

Just thinking. We have this comment in Drupal\Core\Site\Settings:

        // If the database driver is provided by a module, then its code may
        // need to be instantiated prior to when the module's root namespace
        // is added to the autoloader, because that happens during service
        // container initialization but the container definition is likely in
        // the database. Therefore, allow the connection info to specify an
        // autoload directory for the driver.

So, if a db driver module is dependent on some other modules, the same as above applies to the dependencies too.

I am wondering if, as part of installation, we could stash the 'additional autoloads' in the $databases array in settings.php, by looking at the dependencies set in {module}.info.yml. Then at every request we'll just pick those without parsing the directories.

Something like, to illustrate the idea:

$databases['default']['default'] = array (
  'database' => '/var/www/d91/sites/default/files/sqlite-drudbal',
  'username' => NULL,
  'password' => NULL,
  ...
  'dependencies' => array(
    'mysql' => array(
      'namespace' => 'Drupal\\mysql\\Driver\\Database\\mysql',
      'autoload' => 'core/modules/mysql/src/Driver/Database/mysql/',
    ),
    'sqlite' => array(
      'namespace' => 'Drupal\\sqlite\\Driver\\Database\\sqlite',
      'autoload' => 'core/modules/sqlite/src/Driver/Database/sqlite/',
    ),
  ),
);

Nice, and even nicer the tests.

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -510,6 +510,25 @@ public static function convertDbUrlToConnectionInfo($url, $root) {
+        $parent_class = $namespace . '\\ParentDatabaseDriver';

maybe we do not need a separate class for that, just a static array (or better a public static method) on the Connection class

[
 $provider => $namespace,
]

then also you won't need to pass array $dependencies to Connection::createConnectionOptionsFromUrl, Connection could know it by itself.

Also this will work for 'one level' of dependencies only - i.e. driver B extends from driver A, not for driver C that extends from B that extends from A, but that would seem overkill to me, so fine.

We also need to reflect this in the settings.php docs...

Aha... need to think if we could have alternatives here. Need some time...

working on this

This changes the approach a bit to rely on the dependencies set in module.info.yml to determine the autoload dependencies for the db driver, so we do not need a new class. PSR4 allows specifying only the base namespace of a module and autodetermine the driver code in the subnamespace so we are fine without needing to change anything in the db URI.

Needs more tuning and fixing a unit test that is failing on DrupalCI but locally works fine - no idea why.

Streamlined a bit.

CS fixes

Changed to MR and deprecated Database::findDriverAutoloadDirectory()

Let's postpone on #3257714: Remove loading from the Drupal\Drivers namespace from drupal_get_database_types() that would help further cleanup in install.inc.

Actually the MR fully incorporates #3257714: Remove loading from the Drupal\Drivers namespace from drupal_get_database_types(), so this can be reviewed independently

Thank you @daffie. Now we only have to add a test for the case a driver module specifies a dependency that does not exist. We need another test module with a fake driver and a missing dependency.

Let's see this, we need to ensure the broken test module does not get caught in the middle of a normal extension discovery.

I made a small edit to the CR

Rerolled

rerolled

rebased

rebased

Rebased.

Per #30 I am moving credit from the other issue to here.

I doubt this will make 10.0. Repurposing for 10.1.

Rebased

There's a fail test.

Yeah, something new was added that the MR is not reflecting. Looking into it.

I think failure is due to #3293446: Create less static cache in ExtensionDiscovery during KernelTests

I've added some questions to MR code review.

The biggest thing is should we take this as an opportunity to introduce a DatabaseDriverList object extending from ExtensionList and DatabaseDriver extending from Extension.

Made the easy changes. Do we need to try

to introduce a DatabaseDriverList object extending from ExtensionList and DatabaseDriver extending from Extension

here or it would be ok as a follow-up? I agree it makes sense, but I'd rather explore it independently, so that if we end up with ripples we can iron them out there.

Filed #3314325: Introduce DatabaseDriver and DatabaseDriverList classes to manage installable driver extensions for follow-up.

@mondrake @daffie I'm not sure about leave new API for a follow-up. We're already adding new API for a feature request here. If we going to rework that in a follow-up that doesn't make much sense to me.

so we need doing it here. pity we find that out after 8 months in RTBC.

looking into it.

@mondrake that's really fair. I'm sorry I had got round to a deep review and trying to think about the architectural implications. I will try to keep the reviews coming to help get this across the line asap.

Closed MR 1626, and opened a new MR to introduce the new extension objects. Very first go, not for review, wanted to see how much this fails.

Net of the FunctionalJavascript test failure that are bogging the bots today, this should be working now.

Points:

• Database::convertDbUrlToConnectionInfo() should be working also outside of container context - for kernel tests but also for any third party script that needs to determine drivers (e.g. cli installation etc). Here I added an helper that returns the service or instantiates directly a DatabaseDriverList class depending on whether the container is available or not.
• because of the above, I tried limiting as much as possible other injected classes
• because of the above, I'm not sure it makes sense to extend from ExtensionList and Extension. For now I did, but throwing \LogicExcpetion for all methods that are not implemented and that would not makes sense to inherit.
• I think we could now think about deprecating drupal_get_database_types() and drupal_detect_database_types(), too.
• Not really sure how the DatabaseDriverList class should interact with the cache.

Following on #57, deprecated drupal_get_database_types () and drupal_detect_database_types().

Thanks for reviews,

please add a comment to the d.o. issue after commenting in the MR, unfortunately MR comments do not update timestamps at the moment and there’s risk to miss progress.

I am addressing some of these review points.

Freeing assignment.

Added a deprecation test for the deprecated install.inc functions.

Changes and comments on the MR.

Interesting reading about the Extension class, #2959989: Deprecate Extension::__call() magic

Rerolled. Would be lovely to have some input as how to progress here.

How bad it is that MR comments do not bump the timestamp of the d.o. issue...

Let's see what happens with your suggestion @daffie - a patch for the moment so not to pollute the MR

Now use module\driverName as the key for the list of drivers.

It's complicated... but I think it's feasible. We need to change all internals to use the module\driver convention, and change install_get_form() to swap calls without the module name to add the module name, so that scripts can still use legacy logic.

Let's see if this one gets green, then I'll comment and address #87.

Interdiff is vs. MR2844, For some reason I cannot open a separate branch as I would like: https://drupal.slack.com/archives/C51GNJG91/p1672150197341309

Opened MR 3169 (thanks @drumm) and fixed #87.2 and #87.3.

Tested MR3169 on the DruDbal experimental driver, it passes ok https://github.com/mondrake/drudbal/pull/315

#87.1 and .7 - in fact here we are only preparing values for the database selection form submission, not for settings.php directly. Tried to clarify in last commint on the MR.

Thanks @daffie for reviews and input.

I start thinking to make another step, and instead of using 'module\driver' as the internal name of the extension and of the list keys, use the driver namespace instead (e.g. 'Drupal\mysql\Driver\Database\mysql').

Like that, we would anyway ensure uniqueness and avoid introducing another convention. And btw that has the benefit of being explicitly written to settings.php as such in the 'namespace' database connection info array, whereas 'module\driver' will have to be always determined programmatically.

In the end, we're making the 'driver' key in the database connection info array redundant, but that could be for another future issue.

Last commit in the MR is essentially implementing #99, bar the test adjustments.

So, where PHP doesn't get to, PHPStan to the rescue:

10:54:03 Running PHPStan on *all* files.
10:54:03  ------ ----------------------------------------------------------------------- 
10:54:03   Line   core/tests/Drupal/FunctionalTests/Installer/InstallerExistingBrokenDa  
10:54:03          tabaseSettingsTest.php                                                 
10:54:03  ------ ----------------------------------------------------------------------- 
10:54:03   37     Class                                                                  
10:54:03          Drupal\driver_test\Driver\Database\DrivertestMysqlDeprecatedVersion    
10:54:03          not found.                                                             
10:54:03          💡 Learn more at https://phpstan.org/user-guide/discovering-symbols    
10:54:03   39     Class                                                                  
10:54:03          Drupal\driver_test\Driver\Database\DrivertestMysqlDeprecatedVersion    
10:54:03          not found.                                                             
10:54:03          💡 Learn more at https://phpstan.org/user-guide/discovering-symbols    
10:54:03  ------ ----------------------------------------------------------------------- 
10:54:03 
10:54:03 
10:54:03  [ERROR] Found 2 errors                                                         
10:54:03 
10:54:03 
10:54:03 PHPStan: failed

Thanks for the housekeeping of IS and CR, @daffie!

The latest changes looks good to me.

Found an issue with InstallerTestBase while working on the mysqli driver, fixed.

I'm concerned about the number of low level changes we're having to make after making database drivers modules, it feels a bit like all the special casing we've had over the years for install profiles. Having said that the code here seems OK in itself. I think this should be a target for early in the 10.2.x cycle so it has time to bed in. We should resolve @quietone's recent comment though so marking needs work for that.

I have tried @quietone suggestion, but removing the line tests still fail. Reverted and reset to RTBC. Deprecations now are for 10.2.x.

Left some comments on the MR, huge effort here folks 💪

Thank for the review @larowlan! NW to go through the points.

Removed code from Database::findDriverAutoloadDirectory to use the new API. It's OK but had an impact on a test. Can I get a review of latest changes before continuing with the rest of the points.

I have addressed all points from @larowlan.

I took this for a spin locally, and tested that I could install with drush
Was able to install with `drush si demo_umami` and the site looked good with a click about.

I agree with @catch in #112 that its best to get this into 10.2.x early and let any issues shake out.

So in line with that, committed to 11.x - thanks all!

💃

Adjusted CR for branch, and published it.

I bet it's good parent #2024083: [META] Improve the extension system (modules, profiles, themes)

It seems like this issue caused regression on Windows: #3383616: "core_version_requirement key must be present" on core modules on Windows