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
Comment | File | Size | Author |
---|---|---|---|
#86 | 3129534-86.patch | 20.11 KB | daffie |
#86 | interdiff-3129534-84-86.txt | 5.99 KB | daffie |
Issue fork drupal-3129534
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:
- 3129534-automatically-enable-the changes, plain diff MR !79
Comments
Comment #2
daffie CreditAttribution: daffie commentedComment #3
daffie CreditAttribution: daffie commentedBlocked on #2664322: key_value table is only used by a core service but it depends on system install.
Comment #4
BeakerboySomewhat 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.
Comment #5
daffie CreditAttribution: daffie commentedYes, that is the idea. :)
Comment #6
mondrake#4 this is talking, +1! And it has been discussed out there at #2657888: Add Date function support in DBTNG for quite a while.
Comment #8
daffie CreditAttribution: daffie commentedComment #9
daffie CreditAttribution: daffie commentedThis issue is blocked by #2664322: key_value table is only used by a core service but it depends on system install.
Comment #10
daffie CreditAttribution: daffie commentedThis issue is blocking #3129043: Move core database drivers to modules of their own.
Comment #11
daffie CreditAttribution: daffie commented#2664322: key_value table is only used by a core service but it depends on system install has landed.
Comment #12
daffie CreditAttribution: daffie commentedThe patch fails to apply.
Comment #13
daffie CreditAttribution: daffie commentedComment #14
daffie CreditAttribution: daffie commentedPatch rerolled.
Comment #15
daffie CreditAttribution: daffie commentedComment #16
mondrakeIMHO in this issue we should also enforce that the database driver module cannot be manually uninstalled afterwards.
Comment #17
BeakerboyIn 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.
Comment #18
daffie CreditAttribution: daffie commentedThank 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.
Comment #20
BeakerboyCould this patch include either a stubbed database module for mocking, or code that creates a mock of a database module?
Comment #21
daffie CreditAttribution: daffie commentedI do not see how that can be done. If you know a way to do it, then please show it.
Comment #22
BeakerboyI 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?
Comment #23
BeakerboyI 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.
Comment #24
daffie CreditAttribution: daffie commentedAdding 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.
Comment #25
daffie CreditAttribution: daffie commentedAdded 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?
Comment #27
daffie CreditAttribution: daffie commentedFixed the coding standard violation and the SQLite testbot failure.
Comment #28
BeakerboyTesting #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
Comment #29
BeakerboyIs 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".
Comment #30
daffie CreditAttribution: daffie commentedCreated a combined patch in #3129043: Move core database drivers to modules of their own.
Comment #31
quietone CreditAttribution: quietone as a volunteer commentedI 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.
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
The examples on the standards page, https://www.drupal.org/node/1354, use FALSE not false.
To get this onto one line how about, "Ensures installed modules providing a database driver are not uninstalled"?
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'."
This is nearly identical to the parent method. Should work be done here to reduce the duplicate code or in a followup?
Instead of dburl can we just use the environment name 'SIMPLETEST_DB' ?
Comment #32
naresh_bavaskarComment #33
naresh_bavaskarAddressed #31 changes (except 5th). Please review
Comment #35
daffie CreditAttribution: daffie commented@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.Comment #37
daffie CreditAttribution: daffie commentedTestbot is green again.
Comment #38
quietone CreditAttribution: quietone as a volunteer commentedI 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.
Comment #41
mondrakeMade 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?
Comment #42
mondrakeComment #43
daffie CreditAttribution: daffie commented@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.
Comment #44
daffie CreditAttribution: daffie commentedChanged 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.
Comment #45
daffie CreditAttribution: daffie commentedFor 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.
Comment #46
mondrake1.
it can be
string|null
in the docblock2.
it could be
#45 I'm fine.
Comment #47
daffie CreditAttribution: daffie commentedUpdated 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.
Comment #48
mondrakeThank you. Agree, #46.2 is just my personal preference.
Comment #49
alexpottShould we be inspired by plugins and other systems are use getProvider() here and return 'core' for a non-module provider?
Comment #50
mondrake#49 +1 - change naming of the method and signature since in that case a value will be always returned and won't be nullable.
Comment #51
daffie CreditAttribution: daffie commentedThat 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()".
Comment #52
alexpottI 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.
Why call ->getModuleName() twice here...
$module === $this->connection->getModuleName()
should be fine.Comment #53
daffie CreditAttribution: daffie commentedWorking on this.
Comment #54
daffie CreditAttribution: daffie commentedChanged the name from
Connection::getModuleName()
toConnection::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.
Comment #55
mondrakeThanks, now I really find this hard to read... and would suggest to have the variables set outside the
if
.Comment #56
daffie CreditAttribution: daffie commentedFor comment #55: Fixed.
Comment #57
mondrakeThanks, loooks great now.
nit: no need to put
$connection
in bracketsnit: "core" when the driver is not provided as part of a module
Comment #58
daffie CreditAttribution: daffie commentedFixing the nits from comment #57.
Comment #59
alexpottOne 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.
Comment #60
daffie CreditAttribution: daffie commentedI 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.
@alexpott: Could you explain a little bit more.
Comment #61
catchWe'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?
Comment #62
mondrakeComment #63
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedFixed the typo.
Comment #64
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedMistakenly created interdiff with .patch extension.
Comment #65
daffie CreditAttribution: daffie commentedI 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.Comment #66
mondrakeThe 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
Comment #67
daffie CreditAttribution: daffie commentedIt is only repeated 3 times. And yes, I created a helper method in the Connection class, as it needs an active database connection.
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.
Good idea! Done that and added testing for it.
Comment #68
mondrakeI 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?)
These could be both
$this->assertSession()->pageTextContains()
, and it's not necessary to wrap the expected test int()
.maybe "Enable the module that is providing the database driver if required."
"be" is redundant.
Comment #69
daffie CreditAttribution: daffie commentedFor #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.
Comment #70
mondrakeThanks! RTBC assuming this is green.
Comment #71
daffie CreditAttribution: daffie commentedAdded the API additions to the IS.
Comment #72
catchMinor-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.
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?
Possibly a silly question - how does the status report even load if the driver is pointing to a not-enabled module?
Comment #73
daffie CreditAttribution: daffie commentedFor #72.1-2: @alexpott said in comment #49 the following:
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.
Comment #74
mondrakeThe 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.
Comment #75
catch#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?
Comment #76
catchOK 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') {
?>
Comment #77
daffie CreditAttribution: daffie commentedI have tried to make the suggested change:
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.Comment #78
catchEven if we need $connection later, we can still remove the ternary no?
Comment #79
daffie CreditAttribution: daffie commentedMade the requested change.
Comment #80
mondrakeI have a few nits and a more general question:
#72.2 suggests to name this variable as
$provider
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).
return typehint to
:void
#72.2 suggests to name this variable as
$provider
return typehint to
:void
return typehint to
:void
Comment #81
daffie CreditAttribution: daffie commented@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.
Comment #82
mondrakeThanks. I do not have a proposal for #80.2, really. Was more a question. Let's see if we have feedback from committers.
Comment #83
catchWith #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.
Comment #84
daffie CreditAttribution: daffie commentedRemoved the call to
setPathname()
. #2719315: Try to install system module like any other module has not landed, therefore I used the method just like as core does now in other places.Comment #85
catchDiscussed 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.
Comment #86
daffie CreditAttribution: daffie commentedRemoved the new method
Connection::enableModuleProvidingDatabaseDriver()
from the patch and the same for were the method was used.Comment #87
mondrakeLooks like all concerns addressed.
Comment #89
catchThanks for the various revisions. This looks good to me now.
Committed f838dbc and pushed to 9.2.x. Thanks!
Comment #90
daffie CreditAttribution: daffie commentedThank you to all who helped get this issue landed!
Comment #93
andypostIs there a reason why change record is not published yet?
The unused variable introduced and now it's blocker, filed follow-up #3320483: Remove unused variable $pos in system.install
Comment #94
quietone CreditAttribution: quietone at PreviousNext commentedPublish the CR