Problem

  • global $databases is a legacy global state that leaks into all code.

Proposed solution

  1. Database connection info is read from settings.php exactly once in drupal_settings_initialize().

    → Replace global $databases with Database::setConnectionInfo().

  2. Remove global $databases.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

sun’s picture

Assigned: Unassigned » sun
Status: Postponed » Needs review
FileSize
10.1 KB

And now a working patch. Lovely.

Status: Needs review » Needs work

The last submitted patch, 2: drupal8.database-global.2.patch, failed testing.

sun’s picture

Issue summary: View changes

Hm. 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)

sun’s picture

Status: Needs work » Needs review
FileSize
12.7 KB

That 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).

Status: Needs review » Needs work

The last submitted patch, 5: drupal8.database-global.5.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
13.43 KB

This one should pass.

ParisLiakos’s picture

not sure about the changes in (Web)TestBase to rtbc this myself, but this patch looks lovely

Crell’s picture

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -293,6 +307,17 @@ public static function addConnectionInfo($key, $target, $info) {
+  /**
+   * Sets database connection information.
+   *
+   * @param array $databases
+   *   A multi-dimensional array specifying database connection parameters.
+   */
+  final public static function setConnectionInfo(array $databases) {
+    self::$rawDatabaseInfo = $databases;
   }

The 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.

sun’s picture

FileSize
21.43 KB
13.55 KB

Changing 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.

Status: Needs review » Needs work

The last submitted patch, 10: global.database.10.patch, failed testing.

The last submitted patch, 10: global.database.10.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
21.45 KB
533 bytes

Oopsie. Fixed wrong parameters for Database::addConnectionInfo().

Status: Needs review » Needs work

The last submitted patch, 13: global.database.13.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
22.24 KB
816 bytes

Ah, 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.

sun’s picture

@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 to parseConnectionInfo() are removed.

andypost’s picture

Migrate on sandbox already changed the logic about databases, so it's fine now http://goo.gl/3DcZ98

sun’s picture

Note 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.

sun’s picture

FileSize
21.43 KB

#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 :-)

sun’s picture

Now how awesome looks that?

 function drupal_settings_initialize() {
   // Export these settings.php variables to the global namespace.
-  global $base_url, $databases, $cookie_domain, $config_directories, $config;
+  global $base_url, $cookie_domain, $config_directories, $config;

:-)

  1. #1757536: Move settings.php to /settings directory, fold sites.php into settings.php will kill and vastly improve conf_path().
  2. #2183591: Replace global $config with a property on the Settings singleton will take care of $config.
  3. Would be nice to create an issue for killing $config_directories.
  4. The remaining $base_url + $cookie_domain + drupal_request_initialize() will be killed by #2016629: Refactor bootstrap to better utilize the kernel (or follow-ups to that).
  5. #2180109: Change the current_user service to a proxy will hopefully find a final solution for 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 onto ThemeHandler or into random Request attributes.

sun’s picture

20: global.database.20.patch queued for re-testing.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

This 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?

sun’s picture

20: global.database.20.patch queued for re-testing.

Crell’s picture

I'm not an installer expert, but the DB parts of this look OK to me now. Thanks, sun!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

We need a change record here

alexpott’s picture

Status: Needs work » Needs review
FileSize
509 bytes
21.62 KB

Also 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

sun’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record
FileSize
21.69 KB
1.62 KB

Draft change notice: https://drupal.org/node/2204083

  1. Fixed inaccurate @return type of getAllConnectionInfo().
  2. Renamed setConnectionInfo() into setMultipleConnectionInfo() for consistency + clarity.

With that, we should be back to RTBC.

Crell’s picture

Agreed. (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. :-) )

sun’s picture

Yeah, 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 into Drupal\Component\Database, my opinion would likely change. But then again, we'd be able to move everything else and just simply keep Drupal\Core\Database\Database where it is. ;)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome 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!

alexpott’s picture

Status: Fixed » Postponed

Reverted 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.

Xano’s picture

Has that Drush fix been deployed to the bots yet?

sun’s picture

Status: Postponed » Needs review
FileSize
21.43 KB
3.36 KB

Status: Needs review » Needs work

The last submitted patch, 34: global.database.34.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
24.5 KB
3.08 KB
Crell’s picture

Status: Needs review » Reviewed & tested by the community

The bots seem to be happy now. If not, we won't know until we commit it. :-)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Hopefully the test bost is happy with this now.

  • Commit 2b9cec6 on 8.x by Dries:
    Issue #2176621 by sun, alexpott: Remove global .
    

Status: Fixed » Closed (fixed)

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