Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem
global $databases
is a legacy global state that leaks into all code.
Proposed solution
-
Database connection info is read from
settings.php
exactly once indrupal_settings_initialize()
.→ Replace
global $databases
withDatabase::setConnectionInfo()
. -
Remove
global $databases
.
Comment | File | Size | Author |
---|---|---|---|
#36 | interdiff.txt | 3.08 KB | sun |
#36 | global.database.36.patch | 24.5 KB | sun |
#34 | interdiff.txt | 3.36 KB | sun |
#34 | global.database.34.patch | 21.43 KB | sun |
#28 | interdiff.txt | 1.62 KB | sun |
Comments
Comment #1
dawehnerComment #2
sunAnd now a working patch. Lovely.
Comment #4
sunHm. Only Migrate test failures... I can see how that might be futzing with database connections. Will have a look later, unless someone beats me to it.
For now, just correcting the issue summary to be accurate. (the signature of
getConnectionInfo()
is no longer changed)Comment #5
sunThat was a bit harder to figure out:
install_settings_form()
consumes its user input from (A) interactive form, (B) settings.php, or (C) non-interactive installer parameters, but in the latter two cases, the table prefix information in the database connection info may already be an array — but the 'prefix' form element only accepts a single table prefix (the default prefix for all tables).Comment #7
sunThis one should pass.
Comment #8
ParisLiakos CreditAttribution: ParisLiakos commentednot sure about the changes in (Web)TestBase to rtbc this myself, but this patch looks lovely
Comment #9
Crell CreditAttribution: Crell commentedThe changes to the Database class would be much simpler if we parsed this data immediately. We're setting it here, just parse it and go. There's no need to save the otherwise useless unparsed version.
Comment #10
sunChanging that felt a bit out of scope for this issue, but it probably makes sense to do it here.
→ Fixed connection info parsing in Database class.
Note: The phpDoc fixes are not unrelated changes — I had to figure out what the actual parameter and return value data types of the other methods are in order to change the connection info parsing.
Fixing the connection info parsing also allows us to remove some @todos from
TestBase
.Comment #13
sunOopsie. Fixed wrong parameters for Database::addConnectionInfo().
Comment #15
sunAh, Migrate again.
Fixed MigrateTestBase::prepare() injects a connection instead of connection info into SqlBase.
Color me very confused how this did not blow up in the past. I can only assume that some lucky coincidence of global state caused this to work.
Comment #16
sun@Crell, does the latest patch address your concern?
The former
$rawConnectionInfo
has been removed and instead,Database::addConnectionInfo()
ensures to prepare/parse every added database connection info array only once. All other calls toparseConnectionInfo()
are removed.Comment #17
sunComment #18
andypostMigrate on sandbox already changed the logic about databases, so it's fine now http://goo.gl/3DcZ98
Comment #19
sunNote that we might want to do #2036259: Move $drupal_hash_salt to settings() first, since the hunk to
drupal_get_hash_salt()
is essentially copied from there.Comment #20
sun#2036259: Move $drupal_hash_salt to settings() has landed, so here is the same patch without the change to
drupal_get_hash_salt()
.Still looks ready to me :-)
Comment #21
sunNow how awesome looks that?
:-)
conf_path()
.$config
.$config_directories
.$base_url
+$cookie_domain
+drupal_request_initialize()
will be killed by #2016629: Refactor bootstrap to better utilize the kernel (or follow-ups to that).global $user
soon.→ Once all of those are done, we're pretty much left with
global $theme
* only, for which we should have a dedicated issue, so we can discuss how to properly resolve that, without tacking the values ontoThemeHandler
or into randomRequest
attributes.Comment #22
sun20: global.database.20.patch queued for re-testing.
Comment #23
longwaveThis looks good and all changes in the patch make sense to me, maybe someone with a better understanding of the installer should also check it over before commit though?
Comment #24
sun20: global.database.20.patch queued for re-testing.
Comment #25
Crell CreditAttribution: Crell commentedI'm not an installer expert, but the DB parts of this look OK to me now. Thanks, sun!
Comment #26
alexpottWe need a change record here
Comment #27
alexpottAlso in fixing Drush to work with this I've discovered we need a way to get all the database information. See https://github.com/alexpott/drush/tree/sql-engine
Comment #28
sunDraft change notice: https://drupal.org/node/2204083
With that, we should be back to RTBC.
Comment #29
Crell CreditAttribution: Crell commentedAgreed. (Although part of me wants to eliminate setMultipleConnectionInfo() entirely and just tell people to loop over the add method themselves; it's the same runtime cost. I won't fight for that, though; I'll burn my karma elsewhere. :-) )
Comment #30
sunYeah, I can totally relate to that thought. :-)
I was considering that too, but ultimately thought that a helper method on the Database class (1) avoids to have this code elsewhere [potentially duplicated even] and (2) doesn't actually introduce something new, since the entire rest of the class contains a fair level of similar assumptions on the $databases info structure already.
The day we move
Drupal\Core\Database
intoDrupal\Component\Database
, my opinion would likely change. But then again, we'd be able to move everything else and just simply keepDrupal\Core\Database\Database
where it is. ;)Comment #31
webchickAwesome work! I'm actually familiar with at least some of this code from my days getting the DB to auto-create if it doesn't exist in the installer. :) I read through the patch and all of the changes seem valid to me. $GLOBALS--;
Committed and pushed to 8.x. Thanks!
Comment #32
alexpottReverted this since it broke testbots. We need https://github.com/drush-ops/drush/pull/483 to merged in first and then pushed to all the bots. Postponing on this or drush dependency being removed from testbots.
Comment #33
XanoHas that Drush fix been deployed to the bots yet?
Comment #34
sunLet's see. Updated for #2245249: Convert installer forms to FormInterface
Comment #36
sunUpdated for #1808220: Remove run-tests.sh dependency on existing/installed parent site
Comment #37
Crell CreditAttribution: Crell commentedThe bots seem to be happy now. If not, we won't know until we commit it. :-)
Comment #38
Dries CreditAttribution: Dries commentedCommitted to 8.x. Hopefully the test bost is happy with this now.