KeyValueFactory hard-codes DatabaseStorage

It doesn't need to be fancy.

The factory itself is defined in the DIC, if you want to use a completely different approach, you can replace the class. IMHO, we don't need to go further than the approach in your patch.

This is by design. It's a service, just override it.

Yes, that is correct. There aren't going to be many of them I think.

DatabaseStorage requires the connection (not yet, but will soon and is required to get rid of these db_query() calls in the implementation) as a constructor argument. MemoryStorage does not. We've been discussing this a while in the cache DIC issue, It's just not possible for a factory to know what it has to pass to the constructor.

@sun: Hm, that is exactly what a factory does? Wrapping the knowledge of how a specific implementation of an interface needs to be created.

Sure, we could easily write a factory that knows about the implementations in core and can decide based on a argument or some internal logic if it should create a MemoryBackend or a DatabaseBackend, because it knows these two implementations and knows which constructor arguments they expect.

But it can't know how it would have to create a MongoDbBackend, or a MemcacheBackend, which might e.g. require a Reference('mongodb') as it's second constructor argument, instead of Reference('database').

Making a factory ContainerAware doesn't change anything. Just because it has access to the container doesn't mean that it knows which service it needs?

See the PHP code example on Wikipedia: http://en.wikipedia.org/wiki/Factory_method_pattern#PHP. The factory implementation is a "SedanFactory", that knows that it needs to return new Sedan().

I wasn't tracking the KVStore issue that closely so I don't know the exact code we're dealing with. However, based on my discussion with lsmith and ircmaxell yesterday, we concluded that you don't need a factory IF your logic can be as simple as the cache backend issue: #1764474: Make Cache interface and backends use the DIC. If you cannot reduce your factory to a 1:1 mapping from string to service ID, then you use a ContainerAware factory to map from more than a string to a service ID.

The gist we used for discussion yesterday: https://gist.github.com/3880382

That way, you regiseter:

$container->register('kvstore.thing1', 'KeyValueMongoBased')
  ->addArgument(new Reference('mongodb'));
$container->register('kvstore.thing2', 'KeyValueDbBased')
  ->addArgument(new Reference('database'));
$container->regiseter('kvstore.factory', 'KeyValueFactory')
  ->addArgument(new Reference('service_container');

And then KeyValueFactory does the black magic of figuring out whether to use kvstore.thing1 or kvstore.thing2. That kvstore.thing2 needs a database connection is handled by the Container already, and KeyValueFactory doesn't have to care.

I don't know which of these applies to the KVStore system, as I said, but that's the general distinction. If you have KeyValueFactory, all it needs to know about is service IDs.

+ ->addArgument('%bin%') // ???

is not something possible. Once you compile , there's just no way to pass in an argument. Check the issue linked in #5 on how to achieve this by using two factories.

This pattern exist, it is called abstract factory.

I am trying to get in this issue, but first I have to read the 200+ comments of #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config to understand where the code come from :D

Ok so my opinion on this would be to pre-create the backend object, that is putting them in the pool of objects managed by the container. Then inject them pre-configured where you need them.

For me it makes sense, I am trying to understand your patch and playing a bit with that problem. Tell me if I am wrong but the current philosophy is to put factories in the container, then letting them instantiates objects ?

That's for me very weird (I used to do DI in Java and Flex/AS3 with Parsley/Robotlegs). In these framework the DI has the possibility to use parametric as you can see here in the chapter called Dependency Injection (by ID).

Anyway I don't know the DI component of Symfony but it should be possible, but involve a philosophical change in the way object are registered in the container. I am working on a patch so you'll be able to see what I mean by code.

A simple solution, that I personally don't like is to make the Factory ContainerAware.

Restoring tag

ContainerAware factories are an acceptable thing: #1810912: [meta] Decide on pluggability

This is now critical since comment #31 on #1798732: Convert install_task, install_time and install_current_batch to use the state system

There is no patch here. Working on one.

Note that the fully proper solution of this depends on #1811730: Refactor Database to rely on the DIC solely.

How does this look? Yes, there is a Database::getConnection() with a @TODO to nuke it but otherwise it's a really simple patch that allows for plugging in whatever keyvalue service you might added to drupal_container(). Once Database is DIC-only we can remove the ugly -- and it's a very small, contained ugly.

I temporarily reverted #1798732: Convert install_task, install_time and install_current_batch to use the state system, demoting to major.

Took me a moment to grasp but makes sense to me. We need to figure out what do with the global $conf stuff, whether we want to keep it like that even after variable_get() is gone or if it should be replaced with something else. This is a general question, this just follows existing patterns.

Here's a re-roll that updates the expirable database storage constructor and the tests to make them pass. Made connection a required argument, not sure why that was optional.

This seems like a reasonable approach, and easy enough to fix up after the DB-DIC issue lands.

Forgot to define $connection.

#23: 1809206_23.patch queued for re-testing.

When reviewing the patch in #23 there was just one line that stuck in my craw - in the createStorage() method of StorageTestBase:
$stores[$i] = new $this->storageClass($collection, Database::getConnection());
Passing a db connection to the constructor regardless of class - ugh. I was determined to fix it, but it certainly was not a trivial task. This new patch succeeds in replacing that line, with this:
$stores[$i] = $this->container->get('keyvalue')->get($collection);
You can see from the interdiff what I had to do to get there but in a nutshell:

• I added the missing factory classes for the other two implementations.
• I made StorageTestBase create its own container, to which it registers just the 'keyvalue' service
• Each individual test class then registers the service for its own factory (plus the database service where needed, so that takes care of dependencies) and sets the $conf['keyvalue_default'] var to the name of the service it registered.

I also didn't understand why the existing patch didn't make use of the 'database' service that we have already - it just needed to be moved to the "bootstrap container" because of the current double-container nonsense (whose demise is in the foreseeable future).

I think those changes make a lot of sense. We have multiple issues that are partially moving the Database service directly into the drupal_container() more or less consequently and even more that could benefit from this. Time to get one of them in to get things unblocked.

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueDatabaseExpirableFactory.phpundefined
@@ -0,0 +1,42 @@
+/**
+ * Defines the key/value store factory for the database backend.
+ */
+class KeyValueDatabaseExpirableFactory {
+

Note that to apply this pattern consistently, there will need to be a separate KeyValueExpirable factory and a database.expirable.database as argument for that but that doesn't need to happen here. In fact, this class alone isn't useful and not really necessary here.

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueDatabaseFactory.phpundefined
@@ -0,0 +1,44 @@
+   * @param \Drupal\Core\Database\Connection $connection
+   *   The Connection object containing the key-value tables.
+   * @TODO: remove the NULL case once http://drupal.org/node/1811730 is in.
+   */
+  function __construct(Connection $connection = NULL) {
+    // @TODO: remove the ?: Database::getConnection() once
+    // http://drupal.org/node/1811730 is in.
+    $this->connection = $connection ?: Database::getConnection();

These todo's can be removed as well. We could also make those factories extend each other to only require an override of the get() for expirable.

Ah yes, missed that one - fixed, and made the Expirable factory extend the other as suggested.

+++ b/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/DatabaseStorageExpirableTest.phpundefined
@@ -32,9 +27,23 @@ protected function setUp() {
+    $this->container
+      ->register('keyvalue.database_expirable', 'Drupal\Core\KeyValueStore\KeyValueDatabaseExpirableFactory')
+      ->addArgument(new Reference('database'));
+    global $conf;
+    $conf['keyvalue_default'] = 'keyvalue.database_expirable';

It's just for tests but this is conceptually still wrong IMHO. The KeyValueFactory shouldn't be used to access the expirable keyvalue store. Because that one is documented as returning a non-expirable one, $container()->get('keyvalue')->setWithExpire() doesn't work in normal circumstances.

We might be able to use the same implementation but need to make those setting names dynamically then.

Other than that, I think this is RTBC. I'm also fine with getting this in and adding a keyvalue.expirable factory once we need it, which means my cache_update/form_state issues if that's with others.

Re #29 - yes, I totally missed that, I didn't realise we were dealing with two different interfaces so this needs to be reworked.
Re #30, the problem with using container parameters is that settings.php won't have access to the container - I realise it does now because of the way we have this bootstrap container defined inside the drupal_container() function, but that is a horrible hack that is going away soon. Once that happens, the kernel alone will manage the container and seeing as we're not using Symfony's Config component I don't think there's a lot we can do with configurable parameters. Anyway, this is an important issue to figure out - so I've put down my thoughts on it over at #1810912-16: [meta] Decide on pluggability

Will roll a new patch with the other fixes in the next couple of days unless someone else wants to...

So, my fix for #29 was to add Yet Another Factory Class :-P
In the interests of getting this patch in I'm not dealing with the issue of using $conf for service names as that is a bigger issue that needs to be settled elsewhere. As for the rest of #30:

I thought we fixed that already?

How? I mean, we have to ensure the destructor is run before the table is deleted, otherwise we get a spectacular fail on that test.

Any reason for why can't we use the existing $this->container?

Yes, because it's the bootstrap container, which we're trying to get rid of, and I think it makes sense to have complete control over the contents of the container for these tests.

All of the test fails in sun's patch are due to "The service definition "service_container" does not exist.", as far as I could see, not sure what that is?

I'm not sure if the explicit set() is necessary, it might be enough to do a $container = NULL? In my database in DIC tests, that usually resulted in all services being destructed.

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueExpirableFactory.phpundefined
@@ -0,0 +1,43 @@
+    if (!isset($this->stores[$collection])) {
+      if (isset($conf['keyvalue_exp_service_' . $collection])) {
+        $service_name = $conf['keyvalue_exp_service_' . $collection];
+      }
+      elseif (isset($conf['keyvalue_exp_default'])) {
+        $service_name = $conf['keyvalue_exp_default'];
+      }
+      else {
+        $service_name = 'keyvalue_exp.database';

Not sure if we should use expirable here or not. We usually don't shorten identifier like this I think.

Also not sure about how we use and mix _ and . Is there a specific reason for the current names?

+++ b/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/StorageTestBase.phpundefined
@@ -31,13 +30,34 @@
     $this->container
+      ->register('service_container', 'Symfony\Component\DependencyInjection\ContainerBuilder')
+      ->setSynthetic(TRUE);

Oh, so service_container is a reference to the container itself, why could this cause the failures reported above?

+++ b/core/modules/system/lib/Drupal/system/Tests/KeyValueStore/StorageTestBase.phpundefined
@@ -186,8 +207,9 @@ public function testSetIfNotExists() {
     $stores = array();
+    $factory = $this->expirable ? 'keyvalue_expirable' : 'keyvalue';
     foreach ($this->collections as $i => $collection) {
-      $stores[$i] = $this->container->get('keyvalue')->get($collection);
+      $stores[$i] = $this->container->get($factory)->get($collection);

Could we just add a property for $factory instead of the indirection through expirable?

Addresses #36.

Yes, this looks much better to me.

@sun: Can we handle the whole container handling in setUp()/tearDown() in a separate issue and get this in? While the tearDown() is a bit more elaborate than maybe necessary, this works and allows us to move forward with the cache_update/form removal issues and your unit tests remastered issue I think. That we no longer have to explicitly remove unit test tables affects some other tests to and deserves it's own issue too I think.

Given that, I think this is RTBC. As mentioned above, the not-so-nice global $conf stuff is being hashed out in #1810912: [meta] Decide on pluggability and this can easily be replaced once we have an agreement there.

#37 looks good to me.

Looks good to me too. Committed/pushed to 8.x.