Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Currently, class Drupal\Core\Database\Database has a lot of static stuff going on.
Initialized database connections, the pool of connection info arrays, and some meta stuff are all managed as static properties, and accessed via static methods.
As part of #2354475: [meta] Refactor the installer, (multi)site management, and pre-container bootstrap, I would like to turn the entire thing into a service, managed in the newly introduced CoreContainer.
This means, we need to have an actual object behind the static methods.
We still need to keep the static methods around for BC, but the logic needs to happen in the object.
Proposed solution
Phase 1
- Introduce
class DatabaseManager, with real object methods for all the static methods ofclass Database. - Have a
protected Database::$databaseManager instanceof DatabaseManager.
For phase 1, this will be lazy-instantiated the first time it is needed. - Let the static methods of
class Databasecall the respective method inself::$databaseManager.
OnlyparseConnectionInfo()will remain static, because it's a pure function.
Phase 2
- Introduce
class ConnectionInfoto wrap the connection info for a single database.
Methods will be getDriverClass() and openConnection(), and also toArray() for BC. - Optionally, introduce
class ConnectionInfoPoolto wrap the connection info of all databases.
The benefit is limited because all the manipulators (e.g. renameConnection()) still need to touch both the ConnectionInfoPool and the DatabaseManager.
So far, the changes are all invisible to the public API. In general, components are still forced to use the static methods like Database::getConnection(). The DatabaseManager object is not exposed.
However, unit tests can now test the DatabaseManager, ConnectionInfo and ConnectionInfoPool directly, without having to mess with the global state of the static properties. This is already a benefit.
Phase 3 (follow-up)
This phase will only happen when the CoreContainer / MiniContainer is introduced, as proposed in #2354475: [meta] Refactor the installer, (multi)site management, and pre-container bootstrap
- Introduce the CoreContainer / MiniContainer architecture (other issue).
This also brings a mechanic to initialize global state. - Introduce the core service $coreContainer->DatabaseManager.
- Replace the lazy instantiation of
Database::$databaseManagerwith an explicitDatabase::setDatabaseManager(), to be called from a CoreContainer mechanic. - Refactor unit tests, so that methods like
Database::renameConnection()are no longer needed.
I think this last phase is what will make the changes worthwhile.
But the prior phases 1 and 2 are a good preparation, and I think it is a good idea to keep them out of the CoreContainer scope.
| Comment | File | Size | Author |
|---|---|---|---|
| #41 | interdiff_38-39.txt | 2.49 KB | vsujeetkumar |
| #39 | 2363315-39.patch | 43.68 KB | vsujeetkumar |
| #38 | interdiff_37-38.txt | 2.29 KB | vsujeetkumar |
| #38 | 2363315-38.patch | 42.96 KB | vsujeetkumar |
| #37 | interdiff_36-37.txt | 5.11 KB | vsujeetkumar |
Comments
Comment #1
donquixote commentedComment #2
donquixote commentedhttps://github.com/donquixote/drupal/compare/D8-DatabaseManager-2363315-2
Comment #8
donquixote commentedI made the patches with format-patch like this:
git format-patch --stdout 8.0.x..cd1995a > ***/D8-2363315-2-DatabaseManager-005.patchUnfortunately this does not seem to work except for the first case. So I'm doing it again, this time with git diff.
Please look at the github link for interdiffs / the individual commits.
https://github.com/donquixote/drupal/compare/D8-DatabaseManager-2363315-2
Comment #10
donquixote commentedhttps://github.com/donquixote/drupal/compare/D8-DatabaseManager-2363315-10
Here is the corrected patch 006.
For interdiff see the last commit on github,
https://github.com/donquixote/drupal/commit/b6ff974d5c512fce4800e16a7019...
Comment #12
donquixote commentedhttps://github.com/donquixote/drupal/compare/D8-DatabaseManager-2363315-12
Pretty obvious fix:
https://github.com/donquixote/drupal/commit/43e4e8e5da0b64b32090260da9c8...
Comment #13
fabianx commentedLooks pretty good already.
I would love to see a static initialization instead of the getDatabaseHandler() override in every method call.
But given this is a BC layer if we can convert enough things to not use the Database:: static class, but instead the injected Database Manager directly, then it would be okay.
Comment #14
donquixote commentedWe could introduce a
Database::initStaticProperties()forDatabase::$databaseManager, likeComment #15
donquixote commentedIt's been a while.
Reroll, just in case.
https://github.com/donquixote/drupal/compare/D8-DatabaseManager-2363315-15
I still don't know if we should got for the ConnectionInfoPool here. Does not add so much benefit for now.
Comment #27
smustgrave commentedFollowing up if this still relevant issue?
Comment #28
andypostChanges looks handy, needs DB mantainers to check proper namespaces
Comment #29
andypostComment #30
ravi.shankar commentedWorking on this...will upload the patch soon.
Comment #31
ravi.shankar commentedTried to add reroll of patch #15 on Drupal 10.1.x, there is lots of changes since last patch, please review the patch.
Keeping the status needs work for now.
Comment #32
ajaypratapsingh commentedjust Rerolled patch against drupal 9.5.x
Comment #33
borisson_The last reroll needs phpcs fixes, but phpstan also seems to fail, that might be more difficult to fix.
I agree with #28, we should get a DB maintainer in to have a look at this before going much further?
Comment #34
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #36
vsujeetkumar commentedRe-roll patch created for 11.x. Keeps as in "Needs work" because of unaddressed comment #33.
Comment #37
vsujeetkumar commentedFixed CCF issues.
Comment #38
vsujeetkumar commentedFixed the CCF issue by removing the deprecated method. Please have a look
Comment #39
vsujeetkumar commentedFixed the CCF issue.
Comment #40
borisson_Setting to needs review to make the testbot run the patch
Comment #41
vsujeetkumar commentedAdding missing interdiff. Please have a look.
Comment #42
borisson_Back to needs work