Problem/Motivation

The module that is providing the current database driver cannot override backend services, plugins and EntityStorage classes during installation.

Proposed resolution

Automatically install/enable the module that is providing the current database driver during install process. Doing so right before or after the installation of the "system" module. In that way the module that is providing the current database can override everything it needs to (during the install process) in all other modules.

Remaining tasks

Write patch
Review
Commit

User interface changes

None

API changes

Two new methods are added to the class Drupal\Core\Database\Connection: getProvider() and enableModuleProvidingDatabaseDriver(). The method getProvider() takes no parameters and returns the name of the module that is providing the database driver or "core" when there is no such module. The method enableModuleProvidingDatabaseDriver() also takes no parameters and enables the module that is providing the database driver. If there is no such module or that module is already enabled then the method does nothing.

Data model changes

None

Release notes snippet

TBD

CommentFileSizeAuthor
#86 3129534-86.patch20.11 KBdaffie
#86 interdiff-3129534-84-86.txt5.99 KBdaffie
#84 3129534-84.patch25.72 KBdaffie
#84 interdiff-3129534-81-84.txt835 bytesdaffie
#81 3129534-81.patch25.95 KBdaffie
#81 interdiff-3129534-79-81.txt5.58 KBdaffie
#79 3129534-79.patch25.9 KBdaffie
#79 interdiff-3129534-69-79.txt646 bytesdaffie
#69 3129534-69.patch25.92 KBdaffie
#69 interdiff-3129534-67-69.txt2.27 KBdaffie
#67 3129534-67.patch25.89 KBdaffie
#67 interdiff-3129534-65-67.txt6.83 KBdaffie
#65 3129534-65.patch21.81 KBdaffie
#65 interdiff-3129534-63-65.txt6.51 KBdaffie
#64 interdiff_58_63.txt1008 bytesanmolgoyal74
#63 interdiff_58_63.patch1008 bytesanmolgoyal74
#63 3129534-63.patch16.04 KBanmolgoyal74
#58 3129534-58.patch16.04 KBdaffie
#58 interdiff-3129534-56-58.txt1.29 KBdaffie
#56 3129534-56.patch16.02 KBdaffie
#56 interdiff-3129534-53-56.txt960 bytesdaffie
#54 3129534-53.patch16 KBdaffie
#54 interdiff-3129534-47-53.txt3.13 KBdaffie
#47 3129534-47.patch16.03 KBdaffie
#47 interdiff-3129534-44-47.txt620 bytesdaffie
#44 3129534-44.patch16.03 KBdaffie
#44 interdiff-3129534-43-44.txt1.39 KBdaffie
#43 3129534-43.patch16.02 KBdaffie
#43 interdiff-3129534-35-43.txt7.53 KBdaffie
#35 3129534-35.patch16.27 KBdaffie
#35 interdiff-3129534-33-35.txt882 bytesdaffie
#33 interdiff-27-33.txt3.15 KBnaresh_bavaskar
#33 3129534-33.patch16.27 KBnaresh_bavaskar
#27 3129534-27.patch16.19 KBdaffie
#27 interdiff-3129534-25-27.txt1.96 KBdaffie
#25 3129534-25.patch16.16 KBdaffie
#25 interdiff-3129534-24-25.txt4.18 KBdaffie
#24 3129534-24.patch11.68 KBdaffie
#24 interdiff-3129534-14-24.txt8.06 KBdaffie
#18 do_not_commit_module_required_by_database_driver_uninstall_validator.patch6.23 KBdaffie
#14 3129534-14.patch3.9 KBdaffie
#3 3129534-3-combined-patch-2664322-140-and-2850292-16-and-2847855-40.patch61.34 KBdaffie
#3 3129534-3-for-review-only.patch3.98 KBdaffie

Issue fork drupal-3129534

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

daffie created an issue. See original summary.

daffie’s picture

Title: Automaticly enable the module that is providing the current database driver » Automatically enable the module that is providing the current database driver
Beakerboy’s picture

Somewhat related to this, would it make sense to move the Core date_sql services, like pgsql.views.date_sql, out of views and into the database driver module? Any contrib driver will likely need to implement this service and locate it in their driver module to ensure it is loaded with the database to pass views tests.

daffie’s picture

Somewhat related to this, would it make sense to move the Core date_sql services, like pgsql.views.date_sql, out of views and into the database driver module? Any contrib driver will likely need to implement this service and locate it in their driver module to ensure it is loaded with the database to pass views tests.

Yes, that is the idea. :)

mondrake’s picture

#4 this is talking, +1! And it has been discussed out there at #2657888: Add Date function support in DBTNG for quite a while.

The last submitted patch, 3: 3129534-3-for-review-only.patch, failed testing. View results

daffie’s picture

daffie’s picture

Title: Automatically enable the module that is providing the current database driver » [PP-1] Automatically enable the module that is providing the current database driver
Status: Needs review » Postponed
daffie’s picture

daffie’s picture

Title: [PP-1] Automatically enable the module that is providing the current database driver » Automatically enable the module that is providing the current database driver
Status: Postponed » Needs review
daffie’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

The patch fails to apply.

daffie’s picture

daffie’s picture

Issue tags: -Needs reroll
FileSize
3.9 KB

Patch rerolled.

daffie’s picture

Status: Needs work » Needs review
mondrake’s picture

Status: Needs review » Needs work

IMHO in this issue we should also enforce that the database driver module cannot be manually uninstalled afterwards.

Beakerboy’s picture

In the sqlsrv module, I implement the views.date_sql service. There are some Core Kernel and Functional tests that will default to the MySQL implementation unless another one exists. Currently, I have to patch core with #2966272: Must declare the scalar variable "@@session" in views date filters to get them to pass. I assumed that “automatically enabling the module” would mean I would not have to use this patch, but the tests still fail. KernelTestBase probably needs to manually enable the module to allow contrib drivers to run The core tests without having to enable it manually.

Alternatively, The tests that use this service should be moved into each driver’s module test space.

daffie’s picture

Thank you @mondrake and @Beakerboy for your input on this issue!

For comment #16 from @mondrake: I fully agree that we should enforce that the module that is providing the current database driver cannot be uninstalled. We can do that by making every module that is providing a database driver required. Only that will result in that on a Drupal site with a MySQL backend the modules with the database driver for PostgreSQL and SQLite also will be installed. For me that is not something that I would like to happen. The alternative is to create a module uninstall validator. I have added a patch with such a validator. The problem is that I cannot test the validator, because at the moment I cannot test with a database driver that is provided by a module. We can do that in #3129043: Move core database drivers to modules of their own or in a followup for that issue.

For comment #17 from @Beakerboy: I fully agree that Kernel and Functional tests should have the module that is providing the database driver have enabled. The problem is that I cannot create tests for that functionality, because at the moment I cannot test with a database driver that is provided by a module. We can do that in #3129043: Move core database drivers to modules of their own or in a followup for that issue.

Status: Needs review » Needs work

The last submitted patch, 18: do_not_commit_module_required_by_database_driver_uninstall_validator.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Beakerboy’s picture

Could this patch include either a stubbed database module for mocking, or code that creates a mock of a database module?

daffie’s picture

Status: Needs work » Needs review

Could this patch include either a stubbed database module for mocking, or code that creates a mock of a database module?

I do not see how that can be done. If you know a way to do it, then please show it.

Beakerboy’s picture

I agree that you cannot write an actual Kernel Test that runs against a driver which resides in a module. However, a test CAN be made to test that KernelTestBase WILL include such a driver IF one exists, no? I imagine Drupal\Tests\Core\Test would already tests similar stuff. If not, wouldn’t that be the place for it?

Beakerboy’s picture

I dove into some of the code and found the driver_test module. There has to be a way to construct a connection string that uses one of these drivers, and pass that to a mocked KernelTestBase. Then assert that the class requests to install the module that provides the specified driver.

daffie’s picture

Adding support for not being able to uninstall the module that is providing the current database driver with testing.

Still to do is the request from @Beakerboy for support in KernelTestBase.

daffie’s picture

Added the functionality that the module that is providing the database driver is automatically enabled in KernelTestBase. Also added testing for the functionality. All todo's are now done.

@Beakerboy: Could your test the patch with your database driver for MS SQL-server?

Status: Needs review » Needs work

The last submitted patch, 25: 3129534-25.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

daffie’s picture

Fixed the coding standard violation and the SQLite testbot failure.

Beakerboy’s picture

Testing #27 on Sqlsrv introduces a few regressions:

Unable to uninstall the module in:
Drupal\FunctionalTests\Installer\InstallerExistingConfigMultilingualTest::testConfigSync

Drupal\FunctionalTests\Installer\InstallerExistingConfigSyncDirectoryMultilingualTest::testConfigSync

Drupal\FunctionalTests\Installer\InstallerExistingConfigTest::testConfigSync

Extension handler returns incorrect results in:
Drupal\Tests\system\Kernel\Extension\ModuleHandlerTest::testModuleList

“Installed modules” array incorrect in:

Drupal\KernelTests\Core\Asset\ResolvedLibraryDefinitionsFilesMatchTest::testCoreLibraryCompleteness

Drupal\KernelTests\Core\Common\DrupalFlushAllCachesTest::testDrupalFlushAllCachesModuleList

Drupal\KernelTests\Core\Theme\StableLibraryOverrideTest::testStableLibraryOverrides

Beakerboy’s picture

Is this issue a duplicate of this #3125073: [PP-1] Enable the module with the selected database driver automatically. I'm tempted to move this back to "Needs work". The whole point of this issue is to automatically enable the module. If any site that uses this feature will have to deal with test regressions, then it seems the feature is not implemented fully or correctly. If the solution is "the fixes will have to come when another issue is fixed", then wouldn't that mean it is best to roll the two issues together into one, or do the other issue first? I think it's worth discussing. Imagine if this issue is pulled into core, then work does not continue to progress. Then we are left with a feature that cannot be used in a bug-free way. Things can easily lead to the old adage..."It's only temporary, unless it works".

daffie’s picture

quietone’s picture

Status: Needs review » Needs work

I applied the patch, read the code and played with the tests a bit and it all looks good. I noticed a few things that could be improved.

  1. +++ b/core/includes/install.inc
    @@ -619,6 +620,20 @@ function drupal_install_system($install_state) {
    +  // When the database driver is provided by a module, then install that module.
    

    I think it would help if the comment here includes the explanation given by alexpott in this comment, https://www.drupal.org/project/drupal/issues/3129043#comment-13595222

  2. +++ b/core/lib/Drupal/Core/Database/Connection.php
    @@ -1911,4 +1911,22 @@ public static function createUrlFromConnectionOptions(array $connection_options)
    +   *   false when there is no providing module.
    

    The examples on the standards page, https://www.drupal.org/node/1354, use FALSE not false.

  3. +++ b/core/lib/Drupal/Core/Extension/ModuleRequiredByDatabaseDriverUninstallValidator.php
    @@ -0,0 +1,69 @@
    + * Ensures modules cannot be uninstalled if enabled the database driver depend
    + * on them.
    

    To get this onto one line how about, "Ensures installed modules providing a database driver are not uninstalled"?

  4. +++ b/core/lib/Drupal/Core/Extension/ModuleRequiredByDatabaseDriverUninstallValidator.php
    @@ -0,0 +1,69 @@
    +      $reasons[] = $this->t('The module: @module_name is providing the database driver: @driver_name.',
    

    The use of semi-colons here makes this hard to read, for me. What about, "The module '@module_name' is providing the database driver '@driver_name'."

  5. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseDatabaseDriverModuleTest.php
    @@ -0,0 +1,68 @@
    +  protected function getDatabaseConnectionInfo() {
    

    This is nearly identical to the parent method. Should work be done here to reduce the duplicate code or in a followup?

  6. +++ b/core/tests/Drupal/KernelTests/KernelTestBaseDatabaseDriverModuleTest.php
    @@ -0,0 +1,68 @@
    +    // If the test is run with argument dburl then use it.
    

    Instead of dburl can we just use the environment name 'SIMPLETEST_DB' ?

naresh_bavaskar’s picture

Assigned: Unassigned » naresh_bavaskar
naresh_bavaskar’s picture

Assigned: naresh_bavaskar » Unassigned
Status: Needs work » Needs review
FileSize
16.27 KB
3.15 KB

Addressed #31 changes (except 5th). Please review

Status: Needs review » Needs work

The last submitted patch, 33: 3129534-33.patch, failed testing. View results

daffie’s picture

@quietone: Thank you for your review!

Fixed the testbot failure from the patch from comment #33.

For #31.5: The test needs to extend the default implementation of the parent method. We are testing that when the database driver is provided by a module that that the providing module is automatically enabled. The default test drivers are at the moment not provided by a module. Therefore we cannot use them for this test. The method getDatabaseConnectionInfo is extended to change the database driver to one that is provided by a module. After #3129043: Move core database drivers to modules of their own has landed, the by core supported database driver are provided by a module and therefore can now be used in testing. I have created a followup to change the tests. See #3172024: Use the by core supported database drivers for testing that the providing module is automatically enabled.

Status: Needs review » Needs work

The last submitted patch, 35: 3129534-35.patch, failed testing. View results

daffie’s picture

Status: Needs work » Needs review

Testbot is green again.

quietone’s picture

Issue summary: View changes

I reviewed the patch and all the points in #31 are addressed. I think this is good to go. Unfortunately, I am not confident to set this to RTBC.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

mondrake’s picture

Status: Needs review » Needs work

Made some inline comments of the MR. Besides: do we need an update hook to enable the driver module for drivers that are already module based, are running a site already, but are not enabled?

mondrake’s picture

daffie’s picture

Status: Needs work » Needs review
FileSize
7.53 KB
16.02 KB

@mondrake: I have tried to update the MR. I got an authentication access error. Not sure what I am doing wrong. I generated a new git password and that did not work either.

I fixed all the remarks from @mondrake.

daffie’s picture

FileSize
1.39 KB
16.03 KB

Changed the patch for drupal_install_system() in includes/install.inc as it was generating a lot of errors.
Also updated the docblock for Connection::getModuleName() as it was generating coding standard violation.

daffie’s picture

For the need for an update hook to enable the driver module for drivers that are already module based. As far as I know there is only one database driver that has their driver in a fixed location in the module. That is the PostgreSQL fallback database driver and as far as I can see nobody is using it. The other third party database drivers are using the still using method whereby the driver files need to be copied to the drivers directory. That is by the way deprecated. See #3123461: Deprecate support for database drivers placed in DRUPAL_ROOT/drivers. Therefor I do no see the need for an update hook.

mondrake’s picture

1.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1953,7 +1953,7 @@ public static function createUrlFromConnectionOptions(array $connection_options)
-   * @return ?string
+   * @return string

it can be string|null in the docblock

2.

+++ b/core/includes/install.inc
@@ -619,6 +620,19 @@ function drupal_install_system($install_state) {
+  // When the database driver is provided by a module, then install that module.
+  // System module declares schema and therefore depends on the database driver.
+  if (($connection = Database::getConnection()) && $module = $connection->getModuleName()) {

it could be

  $connection = Database::getConnection();
  $module = $connection ? $connection->getModuleName() : NULL;
  if ($module) {

#45 I'm fine.

daffie’s picture

FileSize
620 bytes
16.03 KB

Updated the docblock of includes/install.inc

@mondrake: If you want me to update the function drupal_install_system() as described in comment #46, then let me know. As the solution from the patch from comment #44 works. Both solutions are fine by me.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Thank you. Agree, #46.2 is just my personal preference.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1950,4 +1950,22 @@ public static function createUrlFromConnectionOptions(array $connection_options)
+  /**
+   * Get the module name of the module that is providing the database driver.
+   *
+   * @return string|null
+   *   The module name of the module that is providing the database driver, or
+   *   NULL when there is no providing module.
+   */
+  public function getModuleName(): ?string {

Should we be inspired by plugins and other systems are use getProvider() here and return 'core' for a non-module provider?

mondrake’s picture

Status: Reviewed & tested by the community » Needs work

#49 +1 - change naming of the method and signature since in that case a value will be always returned and won't be nullable.

daffie’s picture

Should we be inspired by plugins and other systems are use getProvider() here and return 'core' for a non-module provider?

That is something we can do, only I do not think that is very useful. In #3129043: Move core database drivers to modules of their own will the by core supported database drivers be moved to their own modules. The third party database drivers with their drivers in the directory DRUPAL_ROOT/drivers have been deprecated in #3123461: Deprecate support for database drivers placed in DRUPAL_ROOT/drivers. So after #3129043: Move core database drivers to modules of their own and Drupal 10.0 is released all database drivers will have a providing module.
Also having returning 'core' for a third party database driver with the driver in DRUPAL_ROOT/drivers is to me not the right return value. Only if the change is that important for you @alexpott, then I will make the change.

edit: I have no problem in change the method name to "getProvider()".

alexpott’s picture

I think not have a variant type for the return here is nice. And outweighs the downsides especially as the plan is to move to modules for all core drivers - so eventually there won't be anything that return 'core' but that's okay. I think we can document that core means either provided by core or the old way of adding drivers to a special location.

+++ b/core/lib/Drupal/Core/Extension/DatabaseDriverUninstallValidator.php
@@ -0,0 +1,68 @@
+    if (!empty($this->connection->getModuleName()) && $module == $this->connection->getModuleName()) {

Why call ->getModuleName() twice here... $module === $this->connection->getModuleName() should be fine.

daffie’s picture

Assigned: Unassigned » daffie

Working on this.

daffie’s picture

Changed the name from Connection::getModuleName() to Connection::getProvider().
Also the default value to return when there is no provider module has been changed to "core".

Thank you @alexpott and @mondrake for your help on this issue.

mondrake’s picture

+++ b/core/includes/install.inc
@@ -622,7 +622,7 @@ function drupal_install_system($install_state) {
-  if (($connection = Database::getConnection()) && $module = $connection->getModuleName()) {
+  if (($connection = Database::getConnection()) && ($module = $connection->getProvider()) && ($module !== 'core')) {

Thanks, now I really find this hard to read... and would suggest to have the variables set outside the if.

daffie’s picture

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, loooks great now.

  1. +++ b/core/includes/install.inc
    @@ -619,6 +620,21 @@ function drupal_install_system($install_state) {
    +  $module = ($connection) ? $connection->getProvider() : 'core';
    

    nit: no need to put $connection in brackets

  2. +++ b/core/lib/Drupal/Core/Database/Connection.php
    @@ -1950,4 +1950,22 @@ public static function createUrlFromConnectionOptions(array $connection_options)
    +   *   "core" when there is no providing module.
    

    nit: "core" when the driver is not provided as part of a module

daffie’s picture

alexpott’s picture

One thing that's possible right now is that you can swap from a mysql db to a postgres db by migrating the data and swapping the connection info. With this patch we'd then need to do two things ... install the new driver and uninstall the old driver.

I wonder if this points us in a different direction - maybe the module should be auto-enabled and appear as such but shouldn't actually be in core.extension. Tricky. Lot's of competing requirements.

daffie’s picture

maybe the module should be auto-enabled and appear as such but shouldn't actually be in core.extension

I have no problem with having all modules that provide a database driver be auto-enabled. Maybe we should also make them be hidden modules.
I am not sure what the result will be when a module is not added to core.extension. Personally I would like to have database driver modules, have all module capabilities that other modules have. Like creating settings in the admin part of Drupal and overriding backend services.

Tricky. Lot's of competing requirements.

@alexpott: Could you explain a little bit more.

catch’s picture

+++ b/core/includes/install.inc
@@ -619,6 +620,21 @@ function drupal_install_system($install_state) {
 
+  $connection = Database::getConnection();
+  $module = $connection ? $connection->getProvider() : 'core';
+  // When the database driver is provided by a module, then install that module.
+  // System module declares schema and therefore depends on the database driver.
+  if ($module !== 'core') {
+    $autoload = $connection->getConnectionOptions()['autoload'] ?? '';
+    if (($pos = strpos($autoload, 'src/Driver/Database/')) !== FALSE) {
+      $module_info_yml = substr($autoload, 0, $pos) . $module . '.info.yml';
+      if (file_exists($module_info_yml)) {
+        \Drupal::service('extension.list.module')->setPathname($module, $module_info_yml);
+        $kernel->getContainer()->get('module_installer')->install([$module], FALSE);
+      }
+    }
+  }
+

We're down to two tables in system_schema(), is it worth adding a @todo here to https://www.drupal.org/project/drupal/issues/2719315 (or a similar issue) for once system_schema() is empty? Having said that though, even if system stops having a direct dependency on the database driver, won't this need to happen in roughly the same place since it'll need to happen before the first module with a hook_schema() is installed? If so should it be moved out of drupal_install_system() to the immediately preceding step?

mondrake’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
anmolgoyal74’s picture

Status: Needs work » Needs review
FileSize
16.04 KB
1008 bytes

Fixed the typo.

anmolgoyal74’s picture

FileSize
1008 bytes

Mistakenly created interdiff with .patch extension.

daffie’s picture

One thing that's possible right now is that you can swap from a mysql db to a postgres db by migrating the data and swapping the connection info. With this patch we'd then need to do two things ... install the new driver and uninstall the old driver.

I agree with the part that when we start a migration and the driver is provided by a module, then that module should be enabled. For the uninstall part am I not so sure. Do we really want to uninstall such a module when migration are usually done in multiple runs, making sure that the migration went as it should. A better solution to me is to leave the module installed and let the site owner/developer uninstall the module when they are finished with the migration.

Did the same for the class Drupal\Core\Command\DbCommandBase which is used for CLI database dumps and imports.

I changed the added comment from the function drupal_install_system() and removed the reference to the system module.

mondrake’s picture

The module installation code is repeated 4 times, maybe that warrants a helper method of its own? In the main Database class?

I agree it’s better go only the install way, and not the uninstall one. In any case IIUC there’s no harm in having the module installed even if the driver is not in use, right? And, the purpose to have the module installed is to ensure the backend overridable services to be available, right? What else?

If we wanted to be on the super safe side, maybe we could add a reuquirement check on the status report to list an error if a module-provided driver running a site does not have the module installed. This would cover the case of tampering with settings.php

daffie’s picture

The module installation code is repeated 4 times, maybe that warrants a helper method of its own? In the main Database class?

It is only repeated 3 times. And yes, I created a helper method in the Connection class, as it needs an active database connection.

And, the purpose to have the module installed is to ensure the backend overridable services to be available, right? What else?

For me all specific database exceptions outside of the database drivers should be moved to the module that is providing that database driver. Just like contrib modules that are providing a database drivers must do.

If we wanted to be on the super safe side, maybe we could add a requirement check on the status report to list an error if a module-provided driver running a site does not have the module installed. This would cover the case of tampering with settings.php

Good idea! Done that and added testing for it.

mondrake’s picture

  1. +++ b/core/includes/install.inc
    @@ -619,6 +620,22 @@ function drupal_install_system($install_state) {
    +  $connection = Database::getConnection();
    +  $module = $connection ? $connection->getProvider() : 'core';
    +  // When the database driver is provided by a module, then install that module.
    +  // This module must be installed before any other module, as it must be able
    +  // to override any call to hook_schema() or any "backend_overridable" service.
    +  if ($module !== 'core') {
    +    $autoload = $connection->getConnectionOptions()['autoload'] ?? '';
    +    if (($pos = strpos($autoload, 'src/Driver/Database/')) !== FALSE) {
    +      $module_info_yml = substr($autoload, 0, $pos) . $module . '.info.yml';
    +      if (file_exists($module_info_yml)) {
    +        \Drupal::service('extension.list.module')->setPathname($module, $module_info_yml);
    +        $kernel->getContainer()->get('module_installer')->install([$module], FALSE);
    +      }
    +    }
    +  }
    +
    

    I wonder if it would be possible to change this to the new method too, maybe adding an optional argument to it to allow passing in the container (is it necessary BTW?)

  2. +++ b/core/lib/Drupal/Core/Database/Connection.php
    @@ -1950,4 +1950,35 @@ public static function createUrlFromConnectionOptions(array $connection_options)
         return $db_url;
    
    +++ b/core/modules/system/tests/src/Functional/System/DatabaseDriverProvidedByModuleTest.php
    @@ -0,0 +1,72 @@
    +    $this->assertRaw(t('Database driver provided by module'));
    +    $this->assertText(t('The current database driver is provided by the module: driver_test. The module is currently not enabled. You should immediately enable the module.'));
    

    These could be both $this->assertSession()->pageTextContains(), and it's not necessary to wrap the expected test in t().

  3. +++ b/core/lib/Drupal/Core/Database/Connection.php
    @@ -1968,4 +1968,17 @@ public function getProvider(): string {
    +   * Enable the module that is providing the database driver if one exists.
    

    maybe "Enable the module that is providing the database driver if required."

  4. +++ b/core/modules/system/system.install
    @@ -1126,6 +1126,24 @@ function system_requirements($phase) {
    +  // providing module is be enabled.
    

    "be" is redundant.

daffie’s picture

For #68.1: I think we can move this code to the new method, only the new method will become a bit ugly and I think that the overall readabilily will not be improved if we do that.

For #68.2-4: Fixed.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! RTBC assuming this is green.

daffie’s picture

Issue summary: View changes

Added the API additions to the IS.

catch’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/includes/install.inc
    @@ -619,6 +620,22 @@ function drupal_install_system($install_state) {
     
    +  $connection = Database::getConnection();
    +  $module = $connection ? $connection->getProvider() : 'core';
    +  // When the database driver is provided by a module, then install that module.
    +  // This module must be installed before any other module, as it must be able
    +  // to override any call to hook_schema() or any "backend_overridable" service.
    +  if ($module !== 'core') {
    +    $autoload = $connection->getConnectionOptions()['autoload'] ?? '';
    +    if (($pos = strpos($autoload, 'src/Driver/Database/')) !== FALSE) {
    +      $module_info_yml = substr($autoload, 0, $pos) . $module . '.info.yml';
    +      if (file_exists($module_info_yml)) {
    +        \Drupal::service('extension.list.module')->setPathname($module, $module_info_yml);
    +        $kernel->getContainer()->get('module_installer')->install([$module], FALSE);
    +      }
    +    }
    +  }
    +
    

    Minor-ish but I had to read it a couple of times - if we never need $module to be set to 'core', could we just use NULL or FALSE here instead? 'core' isn't a module, and since we're not using it we don't need to give it any meaning beyond FALSE.

  2. +++ b/core/lib/Drupal/Core/Database/Connection.php
    @@ -1950,4 +1950,35 @@ public static function createUrlFromConnectionOptions(array $connection_options)
     
    +  /**
    +   * Get the module name of the module that is providing the database driver.
    +   *
    +   * @return string
    +   *   The module name of the module that is providing the database driver, or
    +   *   "core" when the driver is not provided as part of a module.
    +   */
    +  public function getProvider(): string {
    +    [$first, $second] = explode('\\', $this->connectionOptions['namespace'], 3);
    +
    +    // The namespace for Drupal modules is Drupal\MODULE_NAME, and the module
    +    // name must be all lowercase. Second-level namespaces containing uppercase
    +    // letters (e.g., "Core", "Component", "Driver") are not modules.
    +    // @see \Drupal\Core\DrupalKernel::getModuleNamespacesPsr4()
    +    // @see https://www.drupal.org/docs/8/creating-custom-modules/naming-and-placing-your-drupal-8-module#s-name-your-module
    +    return ($first === 'Drupal' && strtolower($second) === $second) ? $second : 'core';
    +  }
    +
    

    OK this is the reason, because this method either provides a module name or 'core'.

    So I guess the real question is under what circumstances with $connection be FALSE there - could we add a comment explaining when that will happen? Or should we just move all the logic into an if ($connection = Database::getConnection()) or similar to be more explicit? Also should the local $module variable actually be $provider?

  3. +++ b/core/modules/system/tests/src/Functional/System/DatabaseDriverProvidedByModuleTest.php
    @@ -0,0 +1,72 @@
    +      $database['port'] = $connection_info['default']['port'];
    +    }
    +    $settings['databases']['default']['default'] = (object) [
    +      'value'    => $database,
    +      'required' => TRUE,
    +    ];
    +    $this->writeSettings($settings);
    +
    +    $this->drupalGet('admin/reports/status');
    +    $this->assertSession()->statusCodeEquals(200);
    +
    +    // The module driver_test is not enabled and is providing to current
    +    // database driver. Check that the correct error is shown.
    +    $this->assertSession()->pageTextContains('Database driver provided by module');
    +    $this->assertSession()->pageTextContains('The current database driver is provided by the module: driver_test. The module is currently not enabled. You should immediately enable the module.');
    +  }
    

    Possibly a silly question - how does the status report even load if the driver is pointing to a not-enabled module?

daffie’s picture

For #72.1-2: @alexpott said in comment #49 the following:

Should we be inspired by plugins and other systems are use getProvider() here and return 'core' for a non-module provider?

Now I do not mind changing it back to have a default return value of NULL, both solutions are fine by me. It would be nice if @catch & @alexpott come to some sort of agreement on which solution it is going to be. ;-)

For #72.3: The database driver directory is auto loaded by core. Therefore database driver can be used without the providing module being loaded.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

The endgame would be to move core database drivers to a module if I understand correctly, so in the end 'core' as a provider will only be a transient status.

catch’s picture

#73.1
woohoo I will talk to him :)

#73.2
Right but doesn't this mean the system_requirement will never show on runtime for a non-core module then - i.e. that the test passes only because it relies on autoloading of core database driviers?

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs framework manager review

OK talked to @alexpott briefly about this, we think the following should be OK - it shouldn't be possible to get here without a database connection so remove some of the assignments and ternary.


$provider = Database::getConnection()->getProvider();
// When the database driver is provided by a module, then install that module.
// This module must be installed before any other module, as it must be able
// to override any call to hook_schema() or any "backend_overridable" service.
if ($provider !== 'core') {
?>

daffie’s picture

Status: Needs work » Reviewed & tested by the community

I have tried to make the suggested change:

diff --git a/core/includes/install.inc b/core/includes/install.inc
index 02ef87dfab..6f35077cd4 100644
--- a/core/includes/install.inc
+++ b/core/includes/install.inc
@@ -620,8 +620,7 @@ function drupal_install_system($install_state) {
     ->set('profile', $install_state['parameters']['profile'])
     ->save();
 
-  $connection = Database::getConnection();
-  $module = $connection ? $connection->getProvider() : 'core';
+  $module = Database::getConnection()->getProvider() : 'core';
   // When the database driver is provided by a module, then install that module.
   // This module must be installed before any other module, as it must be able
   // to override any call to hook_schema() or any "backend_overridable" service.

In my test patch issue (https://www.drupal.org/project/drupal/issues/3112735#comment-13963864) has the testbot run with the suggested change and the testbot failed with 5919 failures (https://dispatcher.drupalci.org/job/drupal_patches/73833/).
Therefore the suggested change does not work and I am changing the status back to RTBC.

Edit: It fails, because the variable $connection is needed a little bit later.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Even if we need $connection later, we can still remove the ternary no?

daffie’s picture

mondrake’s picture

I have a few nits and a more general question:

  1. +++ b/core/includes/install.inc
    @@ -619,6 +620,22 @@ function drupal_install_system($install_state) {
    +  $module = $connection->getProvider();
    

    #72.2 suggests to name this variable as $provider

  2. +++ b/core/lib/Drupal/Core/Database/Connection.php
    @@ -1950,4 +1950,35 @@ public static function createUrlFromConnectionOptions(array $connection_options)
    +  /**
    +   * Enable the module that is providing the database driver if required.
    +   */
    +  public function enableModuleProvidingDatabaseDriver() {
    +    $module = $this->getProvider();
    +    if ($module !== 'core' && !\Drupal::moduleHandler()->moduleExists($module)) {
    +      $autoload = $this->getConnectionOptions()['autoload'] ?? '';
    +      if (($pos = strpos($autoload, 'src/Driver/Database/')) !== FALSE) {
    +        \Drupal::service('module_installer')->install([$module]);
    +      }
    +    }
    +  }
    +
     }
    

    Is the Connection class the right place for this method? In other database related issue I took away that the API is meant to minimise using other components (IIRC it was about using Guzzle URI component for the parsing of the database connection string in URL form).

  3. +++ b/core/lib/Drupal/Core/Database/Connection.php
    @@ -1950,4 +1950,35 @@ public static function createUrlFromConnectionOptions(array $connection_options)
    +  public function enableModuleProvidingDatabaseDriver() {
    

    return typehint to :void

  4. +++ b/core/modules/system/system.install
    @@ -1126,6 +1126,24 @@ function system_requirements($phase) {
    +    $module = $connection->getProvider();
    

    #72.2 suggests to name this variable as $provider

  5. +++ b/core/modules/system/tests/src/Functional/System/DatabaseDriverProvidedByModuleTest.php
    @@ -0,0 +1,72 @@
    +  public function testDatabaseDriverIsProvidedByModuleButTheModuleIsNotEnabled() {
    

    return typehint to :void

  6. +++ b/core/modules/system/tests/src/Kernel/Scripts/DbCommandBaseTest.php
    @@ -102,6 +102,40 @@ public function testPrefix() {
    +  public function testDatabaseDriverProvidedByModule() {
    

    return typehint to :void

daffie’s picture

@mondrake: Thank you for your review.

Addressed all your points, with the exception of point #80.2. I have added the method to the connection class, because to me that is the logical place to add the functionality. If you have a suggestion were the functionality could be better placed, then please say so. I did not place the method in the Drupal\Core\Database\Database class, because then the method would need a parameter with the connection for which to enable its module.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Thanks. I do not have a proposal for #80.2, really. Was more a question. Let's see if we have feedback from committers.

catch’s picture

Status: Reviewed & tested by the community » Needs review

With #80.2 all I can think of is putting it in a static helper class somewhere - that would keep it out of the database layer if we ever wanted to put that in Drupal\Component. The static helper class is not exactly clean, but it does keep the mess isolated.

@alexpott also pointed out in slack that setPathName() shouldn't be necessary, see #2719315: Try to install system module like any other module. Back to needs review again.

daffie’s picture

catch’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Command/DbCommandBase.php
@@ -55,7 +55,13 @@ protected function getDatabaseConnection(InputInterface $input) {
     }
 
-    return Database::getConnection('default', $key);
+    $connection = Database::getConnection('default', $key);
+
+    // When the database driver is provided by a module, then that module must
+    // be enabled.
+    $connection->enableModuleProvidingDatabaseDriver();
+
+    return $connection;

Discussed this a bit more with @alexpott.

One way to get rid of the new method on the connection class is... not to add it at all.

This means we'll potentially be running queries (although only on secondary database connections) without the driver module being installed. However, installing a module when a database query is run via the cli is quite an unpredictable config change on a site. Same goes for migrate.

I think we should be able to remove those three hunks and the associated test coverage and commit the rest. Potentially could move discussion around that situation to a follow-up.

daffie’s picture

Removed the new method Connection::enableModuleProvidingDatabaseDriver() from the patch and the same for were the method was used.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

Looks like all concerns addressed.

  • catch committed f838dbc on 9.2.x
    Issue #3129534 by daffie, anmolgoyal74, mondrake, naresh_bavaskar,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for the various revisions. This looks good to me now.

Committed f838dbc and pushed to 9.2.x. Thanks!

daffie’s picture

Assigned: daffie » Unassigned

Thank you to all who helped get this issue landed!

Status: Fixed » Closed (fixed)

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

andypost’s picture

Is there a reason why change record is not published yet?

+++ b/core/modules/system/system.install
@@ -1126,6 +1126,24 @@ function system_requirements($phase) {
+      if (($pos = strpos($autoload, 'src/Driver/Database/')) !== FALSE) {

The unused variable introduced and now it's blocker, filed follow-up #3320483: Remove unused variable $pos in system.install

quietone’s picture

Publish the CR