The constructor for the config storage classes is currently part of the StorageInterface and it takes an array of options. However, looking at the DatabaseStorage class it really ought to just take a db connection, which should be injected into it. We now have the database connection as a service in the DIC, so this seems like a perfect use case for it. Patch forthcoming...

Files: 
CommentFileSizeAuthor
#18 1762388_18.patch9.15 KBchx
PASSED: [[SimpleTest]]: [MySQL] 40,684 pass(es). View
#16 1762388_16.patch6.39 KBchx
FAILED: [[SimpleTest]]: [MySQL] 40,628 pass(es), 0 fail(s), and 8 exception(s). View
#14 1762388_14.patch9.24 KBchx
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#13 1762388_13.patch10.2 KBchx
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#11 1762388_11.patch12.93 KBchx
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#1 1762388.config-use-di.patch16.17 KBkatbailey
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Comments

katbailey’s picture

Status: Active » Needs review
FileSize
16.17 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

This patch removes the constructor from StorageInterface so that each implementation can decide for itself what it needs, and in the case of the DatabaseStorage class, it takes a db connection.

I've only tested the patch against the Configuration group of tests and I'm still getting 19 fails that I can't figure out, but before I spend more time on it I wanted to see what the CMI people think of this - I haven't been involved in this initiative at all and could be missing a very good reason why you need to have the constructor in the interface that takes an anonymous array of options (I realise there's the issue of configurability and being able to register the settings as a parameter to the DIC, but presumably each individual setting could be equally configurable as a DIC param).

Status: Needs review » Needs work

The last submitted patch, 1762388.config-use-di.patch, failed testing.

Crell’s picture

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

katbailey’s picture

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

And

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.

Yes indeed - and that is how I had done things first time around and things exploded much more spectacularly - basically it broke the installer. It all boils down to this comment in the read() method of the DatabaseStorage class:

    // There are situations, like in the installer, where we may attempt a
    // read without actually having the database available. In this case,
    // catch the exception and just return an empty array so the caller can
    // handle it if need be.
    // @todo Remove this and use appropriate config.storage service definition
    //   in the installer instead.

Basically, this system relies on exceptions being thrown when there is no db in place. So perhaps that @todo should be taken care of in this patch as well.

heyrocker’s picture

Issue tags: +Configuration system

Tagging

sun’s picture

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

Crell’s picture

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.

katbailey’s picture

OK then, as a compromise I'll roll a version of the patch that keeps the constructor in the interface with its anonymous options array and passes the db service from the DIC as the connection option for DatabaseStorage. Will try and get that done this evening.

katbailey’s picture

On second thoughts, I think we should keep this issue for making the proper change, i.e. removing the constructor and using dependency injection, given the discussion that has already taken place here.
Once I get around to rolling the less ambitious version of the patch, i.e. where we don't change the interface but just pass the db connection as the 'connection' param in the options array, I'll create a separate issue for it.

chx’s picture

Assigned: Unassigned » chx

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.

chx’s picture

Status: Needs work » Needs review
FileSize
12.93 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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.

Status: Needs review » Needs work

The last submitted patch, 1762388_11.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
10.2 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Bleh, installer.

chx’s picture

FileSize
9.24 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

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

sun’s picture

Status: Needs review » Needs work
+++ 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?

chx’s picture

Title: Use Dependency Injection for config storage classes » Use Dependency Injection for FileStorage
Status: Needs work » Needs review
FileSize
6.39 KB
FAILED: [[SimpleTest]]: [MySQL] 40,628 pass(es), 0 fail(s), and 8 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 1762388_16.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
9.15 KB
PASSED: [[SimpleTest]]: [MySQL] 40,684 pass(es). View

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

chx’s picture

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.

sun’s picture

Status: Needs review » Postponed

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

mtift’s picture

Assigned: chx » Unassigned
Issue summary: View changes
Status: Postponed » Closed (duplicate)

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.