Use Dependency Injection for FileStorage

+++ b/core/lib/Drupal/Core/Config/DatabaseStorage.php
@@ -26,21 +27,25 @@ class DatabaseStorage implements StorageInterface {
+  public function __construct(Connection $connection = NULL) {
+    $this->connection = $connection;
   }

Why is the connection optional? If this backend won't work without it, just require it.

+++ b/core/lib/Drupal/Core/Config/DatabaseStorage.php
@@ -26,21 +27,25 @@ class DatabaseStorage implements StorageInterface {
   /**
    * Returns the database connection to use.
    */
   protected function getConnection() {
-    return Database::getConnection($this->options['target'], $this->options['connection']);
+    if (!$this->connection) {
+      throw new ConfigException(t('No database connection'));
+    }
+    return $this->connection;
   }

Once the connection is a property, we can just access it directly. No need to go through this method every time and spend the stack call.

+++ b/core/lib/Drupal/Core/Config/DatabaseStorage.php
@@ -93,8 +97,7 @@ class DatabaseStorage implements StorageInterface {
-    return (bool) $this->getConnection()->delete('config', $options)
+    return (bool) $this->getConnection()->delete('config', array('return' => Database::RETURN_AFFECTED))

Isn't that what a delete query does anyway? No need to set a return directive. Really, that's an internal property.

+++ b/core/lib/Drupal/Core/Config/DatabaseStorage.php
@@ -106,8 +109,7 @@ class DatabaseStorage implements StorageInterface {
-    $options = array('return' => Database::RETURN_AFFECTED) + $this->options;
-    return (bool) $this->getConnection()->update('config', $options)
+    return (bool) $this->getConnection()->update('config', array('return' => Database::RETURN_AFFECTED))

Isn't that what an update query does anyway? No need to set a return directive. Really, that's an internal property.

That said, I am +1 on using proper DI here and eliminating the hard dependency on the Database class.

Tagging

Hm. In general, this is different from #1730774: Untangle Cache\DatabaseBackend from procedural database.inc functions to make it available in early bootstrap, because that one is about Cache\DatabaseBackend, while this is touching Config\*Storage

Injecting dependent services explicitly is quite a change.

The problem with this approach is that some storage controllers require (or rather: allow for) additional options to be set.

For example:

- DatabaseStorage: connection/target, table
- FileStorage: directory
- CacheStorage: backend + backend-specific options
- CachedFileStorage: all the CacheStorage options + FileStorage options

The architectural goal really is to keep the config storage controllers as pluggable as possible. Essentially hinting at #1202336: Add a key/value store API to some extent already (a secondary goal is to rebase/wrap the config storage controllers onto the k/v API as soon as it's available).

Re #6, I think this is an issue that is going to hit most plugins, too. It's good practice to not put the constructor in the interface, for exactly this reason; a Database backend only works with a DB connection object and table name, but a Memcache backend wants neither of those but a memcache object instead, while a File backend wants nothing but a path, etc. The dependencies differ. On the other hand, we want to have a common factory point. I'm not entirely sure how to resolve that, but we do need to. (This is another reason why I don't think a universal k/v API is a good idea, but that's technically OT here.)

Making things pluggable (which is good) should not force anonymous array constructors (which are bad).

At bare minimum, though, even if we keep the array constructor we should be passing in an actual connection object, which we can then use internally rather than always calling to Database:: every time. That's the bug we're trying to eliminate here.

Let me see.

Edit: note that we will not inject a database connection in this patch, that's #1764474: Make Cache interface and backends use the DIC and diamond hard. I will just get rid of the options array.

So here is one just to make the tests pass. I hope they do. As we are about to rip the database storage out, it's not worth fretting over whether this is the correct architecture (it's not) -- this is just enough to rip that horrific config options array out and then in the next patch when database storage is thrown out and cachestorage is introdiuced we can properly inject a filestorage into it.

Bleh, installer.

Oh, that still contains the remnants of bumping the database service from corebundle to bootstrap. This is really becoming an absolute small patch.

+++ b/core/includes/bootstrap.inc
@@ -2436,16 +2436,7 @@ function drupal_container(Container $reset = NULL) {
-    // @todo The active store and its options need to be configurable.
-    //   Use either global $conf (recursion warning) or global $config, or a
-    //   bootstrap configuration *file* to allow to set/override this very
-    //   lowest of low level configuration.

Looks like this patch makes that situation even worse?

Sure. We can leave DatabaseStorage alone, it's not for long.

Here."Import configuration 45 passes, 0 fails, and 0 exceptions"

Note that the patch has been enrolled into #1702080: Introduce canonical FileStorage + (regular) cache so if that's accepted this can be closed as duplicate.

Not sure who had the idea to remove DatabaseStorage, but that's not on the table.

Marking this issue postponed, as it looks like we're going to perform this change in #1702080: Introduce canonical FileStorage + (regular) cache

Looks like this is quite a bit out-of-date, that we are already using dependency injection for FileStorage, and that we added the concept of collections in #2262861: Add concept of collections to config storages.

Feel free to re-open if I'm mis-understanding.