Replace global $databases with settings()->get('databases')

Shouldn't there be some code which uses drupal_rewrite_settings() to replace existing settings for the upgrade?

+++ b/core/includes/install.core.incundefined
@@ -1075,7 +1076,9 @@ function install_database_errors($database, $settings_file) {
+    $settings['databases']['default']['default'] = $database;;

Ups, two semicolons.

Do we also need to discuss the security implications of having the database settings (which includes passwords) moved to a different place?

This looks good - just have to address #7.

Well ... we certainly don't have the perfect test coverage.

I'm totally not sure about the changes in drupal_rewrite_settings(). Note: This doesn't work yet.

Let's better wait on #1949724: Allow simpletest child sites to additionally load a test-specific settings.php to allow testing anonymous and configless updates

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -209,7 +209,7 @@
-    global $databases;
+    $databases = settings()->get('databases');

As long as we're doing this (which I support), we should pull this hard dependency out entirely. Pass the settings array into parseConnectionInfo(), and have code elsewhere that grabs it from settings and passes it in.

#1811730: Refactor Database to rely on the DIC solely seems to be a really related issue, especially because it does the same as this issue but way more.

This approach feels not perfect for now, but it allows you to refactor (maybe in that linked issue) way more.

Unfortunately chx tried to get #1811730: Refactor Database to rely on the DIC solely to work, and we kept running into issues that we likely cannot resolve any time soon. So let's at least do the minimum we are able to here, and maybe save the other issue for later.

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -445,4 +458,13 @@ public static function closeConnection($target = NULL, $key = NULL) {
+  /**
+   * Injects the database settings comming from the settings.php.
+   *
+   * @param array $settings
+   */
+  public static function setDatabaseSettings(array $settings) {
+    self::$databaseSettings = $settings;

Why is this a separate method from parseConnectionInfo()?

Well, basically the problem was that Database::parseConnectionInfo() is called all over the place in the Database class, it didn't saw a better way to do it.

Hm. Mostly it's called for lazy-init purposes. I'd be fine with dropping the lazy init and expecting parseConnectionInfo() to always be called before the class is useful. We actually have a couple of different ways to add connection information to the system; I'm fine with "it's your job to make sure there's something there before you call it, yo!"

After starting to work on that I realize that we would require the DB to open a connection via the container:

    $container->register('database', 'Drupal\Core\Database\Connection')
      ->setFactoryClass('Drupal\Core\Database\Database')
      ->setFactoryMethod('getConnection')
      ->addArgument('default');

Is there a way to get the settings service, call get() on there and pass that into a function call of parseConnectionInfo() in order to
bootstrap that before getConnection() can return the connection?

So i'm wondering whether we should really try to limit the scope of the issue and do the injection later?

We already failed twice with putting the database in the container, so +1 for limiting the scope of this issue as much as possible.

Agreed with Berdir. Let's not deal with the Container in this issue.

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -203,7 +195,7 @@
+      throw new ConnectionNotParsedException("Database information haven't been parsed yet");

This is the wrong exception to trow. With this change, "parsing" isn't a thing. There's parseConnectionInfo() which sets a bunch of connections at once, and there's addConnectionInfo(). You could use either one and it would work, so parseConnectionInfo() isn't required at all. What you want to throw is ConnectionNotDefinedException, which is already listed.

Basically, all we need to do here is assume that a dev has setup a connection info one way or another before calling getConnection(). If he didn't he gets an exception, which is already covered by openConnection().

Well UPGRADE.txt specifically says to make your settings.php writable, so we don't need to worry about the situation where it isn't thankfully.

13. Make your settings.php file writeable, so that the update process can
   convert it to the format of Drupal 8.x. settings.php is usually located in

     sites/default/settings.php

Don't we have some kind of check_requirements() thing we do before upgrading just like we do before installing? That sure would be helpful for this kind of thing.

#21: 1951216.database-settings.20.patch queued for re-testing.

I wasn't aware of this issue and worked on + completed a much simpler patch in #2176621: Remove global $databases

Actually, I disagree with the concrete proposal here, for two discrete reasons:

1. Settings is a read-only singleton. But there's no reason for database connection info to be read-only.

   It's perfectly fine if a module adds a custom connection on-demand, or, considering the testing framework, to swap out connections.

2. The Database class itself has a static store for database connection info. There's no reason to duplicate that data into a second place.

   Doing so will only mean that the two stores can get out of sync.

Anyway, I suggest to do #2176621 first, and if deems to be a good idea, we can still come back to this.