Make Cache interface and backends use the DIC

Thanks! :)

To address a point catch made in the prior issue, yes, passing in a DB object or calling Database::getConnection() once in the constructor means that you will be unaffected by db_set_active(). However, unlike catch I do not see that as a problem. Rather, I see it as one of the main reasons we SHOULD switch to injected connection objects, so that the database you're writing to in a cache object or CMI object does not change out from under you unexpectedly. That is, it's not a bug, but a feature, and all the more reason we should be making this switch.

For a taste of things to come: http://privatepaste.com/3c917bb5ea. That patch has a small problem: simpletest when batching messes up the parent site :P otherwise, tests pass.

Reassinging

Burn bot, burn! katbailey pointed out (thanks!) that it'd be (much) nicer to use tagged services instead of this weird string magick I am doing for cache_backends(). Next version. I am sure there will be a next version.

This patches the installer too. Most of the hocus-pocus in the installer cache backend is just gone. When we have a database we switch to the database backend. That's easy because cache() no longer static caches anything. Why would it? drupal_container() does. In drupal_container() we trust.

Any chance to introduce a proper CacheFactory here?

(coming from #1702080-37: Introduce canonical FileStorage + (regular) cache)

@sun What do you mean by CacheFactory?

Cross-linking #1759152: Add a database service to the DIC - once that's done there's no reason to worry about having a db connection class property any longer.

msonnabaum brought up a good point in another issue that if something like the database backend only exists to call the database, then we don't have to inject the database connection, so we could presumably use the Database factory directly with no functional change and it's not ever going to affect unit testing in practice (if we unit test the database backend, we won't want to mock the Database class). That seems reasonable to me and might make this simpler.

As the database maintainer I 100% oppose putting the db_() functions or Database:: static class directly into injectable objects. They were written as utility wrappers only for a reason. They rely on the "active connection" concept, which we are already talking about removing as it doesn't make sense in an injected world. I'd like to see the static Database class go away entirely, in fact, and become a DB manager object in the DIC.

It saves a few function calls, too. And avoids "is the function available yet?" questions entirely. And doesn't make people who do follow a properly injected design laugh at Drupal. And we can make the table name injectable very easily.

See the new routing code for an example. The PathMatcher doesn't work on anything but the database, yet it's an injected object, as it should be.

Moving the database service down to bootstrap.inc means that I can also solve the problems I'm having in #1547008-32: Replace Update's cache system with the (expirable) key value store and #512026: Move $form_state storage from cache to new state/key-value system. Yay!

+++ b/core/includes/cache.incundefined
@@ -23,20 +23,14 @@
+  $container = drupal_container();
+  $service = $container->has("cache.$bin") ? "cache.$bin" : 'cache';
+  return $container->get($service)->bin($bin);

So this means that provide a different cache implementation, you have to do smething like this in settings.php?

drupal_classloader_register('apc', 'modules/apc');
drupal_container()->register('cache', 'Drupal\apc\ApcBackend');
// Or

drupal_container()->register('cache.bootstrap', 'Drupal\apc\ApcBackend');

Adding an example for that to default.settings.php might not be a bad idea? or do we leave that to document to the modules providing cache backends as we currently do?

Will try to re-roll this one, might get too late today, though.

Re-rolled against latest HEAD.

There is now a CacheFactory but we can't really use it. The problem is that we can't put the dynamic $container->has() code inside CacheFactory because that results in an endless loop, obviously.

So what I did right now is define a hardcoded cache.config service and am ignoring CacheFactory for the moment. The problem is that you would need to explicitly override that cache backend, it's not enough to just register('cache').

We could directly register the cache factory in the DIC only and call it like this: drupal_container()->get('cache')->bin($bin). cache() would wrap that and we could also pass it as a reference. A service that needs cache then would just need to know that he receives a factory and not an actual cache. That essentially does mean that we're back at the use of $conf['cache_classes'] and can't do the fancy $container->has('cache.$bin') anymore, though.

Wondering where we're at with the tests and if I messed this up.

Removed the duplicated Reference use statement.

#20: update-dic-1764474-20.patch queued for re-testing.

Berdir: What is wrong with just having cache.$bin service IDs, and injecting that from the DIC? For something as simple as cache that should be fine.

@Crell: i don't know the DIC that well, but wouldn't that mean that we explicitly have to specificy all bins? Right now, we have a fallback mechanism that checks if cache.bootstrap is defined explicitly if you request cache('bootstrap') and if not, falls back to cache.

If we can't do that, then somone who wants to use memcache by default would have to specify every single cache bin, and there are a lot. (13 currently, and entitycache in 7.x uses a bin per entity type).

Is there a way to bake that fallback logic directly into the DIC? Because it only works when you call cache(), you can't use a new Reference() with the current code.

Ok, let's see if this fixes the installer.

I found an interesting installer bug, when the installer is able to create the settings.php then he doesn't explicitly check for readability but then displays an error if it's not TRUE. So on the first request, we always get a requirement error and it works after a refresh.

Doesn't have to do with this issue as far as I can see, so will open a separate issue for that, if there isn't one already.

A) It should be possible to extend ContainerBuilder to support wildcard service registrations. Whether this is a good idea I have no idea. As an idea, here's Container::get extended to wildcards http://privatepaste.com/4abdc2c8ec . Who knows, it might actually work as an extension of ContainerBuilder too.

B). I am surprised #1167144: Make cache backends responsible for their own storage is not linked. That seems relevant / necessary if we dont want A).

Discussed with chx in IRC, one possible solution might be a CacheFactory that works with a cache bin -> service map, which should be relatively easy to extend and doesn't need to know what it needs to invoke a certain backend. The main requirement there is to ensure that backends can switch the bin at runtime, without getting confused about tags and stuff.

Anyway, first step is getting the tests working. Let's see what the current patch does. Fixed some things I messed up when merging and also updated some conversions. Had to add a try/catch to delete() because that was invoked during install time within a test where maintance mode isn't defined.

I can no*w* run tests locally both with run-tests.php and in the UI.

Edit: I can now run test, not I can not run tests...

Fixed the tests and broken the installer. This time it's probably the other way round :p

Had to restore the original InstallBackend, I'm not sure if it's actually required but there is a test that does explicitly test how it's supposed to work. Can probably be simplified a bit, by e.g. testing if $this->dbConnection is set but I didn't wanted to change too much.

Also completely messed up cache validation, copy pasted into the wrong function, that's why the other tests failed.

And... fixing the installer!

More installer fixing. This seems to work manually, not sure why it's necessary.

This is as far as I could get before having to switch away from this task. These are mostly code readibility notes. shouldn't change status of issue.

+++ b/core/includes/bootstrap.incundefined
@@ -2129,14 +2132,14 @@ function drupal_bootstrap($phase = NULL, $new_phase = TRUE) {
-        case DRUPAL_BOOTSTRAP_PAGE_CACHE:
-          _drupal_bootstrap_page_cache();

part 1: is this change for readibility? this combined with part 2 completes an unnecessary change as it doesn't make much of an impact.

Are you trying to make the DRUPAL_BOOTSTRAP_DATABASE condition earlier because it is selected from the switch more often than DRUPAL_BOOTSTRAP_PAGE_CACHE?

+++ b/core/includes/bootstrap.incundefined
@@ -2129,14 +2132,14 @@ function drupal_bootstrap($phase = NULL, $new_phase = TRUE) {
+        case DRUPAL_BOOTSTRAP_PAGE_CACHE:
+          _drupal_bootstrap_page_cache();

part 2

+++ b/core/includes/bootstrap.incundefined
@@ -2320,7 +2322,8 @@ function _drupal_initialize_db_test_prefix() {
+    foreach ($databases['default'] as &$value) {
       // Extract the current default database prefix.

I realize this is a continuation of the syntax that was there before, but to me this reads as there is more than one default database. Just wanted to point out that's difficult to follow unless you know the anatomy of the settings.php file pretty well.

+++ b/core/includes/cache.incundefined
@@ -26,21 +26,13 @@
+  return $container->get($service)->bin($bin);

Perhaps this a personal preference but I think it would be more readible if these were unchained.

$container->get($service) seems to return on an object that has a "bin" method or property. But someone who doesn't know drupal_container intimately may assume that "bin" is a method or property of drupal_container instead.

breaking up the chain of these object calls may go a long way to help folks get ramped up on these functions.

bin() should be exploded into setBin() and getBin() for readability and consistency. Aside of that the patch sounds going into the right direction, I especially like the database connection injected into the database backend. I'm still a bit concern about easyness of overiding backend implementations if they are injected into the DIC.

EDIT: Typo fixes only.

Ok, here is a new patch. Let's see if this will pass the tests. For the record, our installer is a very fragile thing, did cost me some nerves...

Changes:
- Now registers cache.factory and cache.default in the DIC
- This makes it possible to inject the database service into the database backend, without having to do that in the cache factory
- The Cache Factory uses a bin => service mapping that allows to use different services for different backends.
- Renamed bin() to setBin(), not chained anymore
- Removed caching for the moment. It's not much more than a call to the DIC and there are ~25-30 of them on a front page with some content. Feels strange to have caching there when there are hundreds of calls through language() without.

Note: The same service is currently re-used for multiple backends, and is switched on run time. This is nice but I'm not sure if it's possible. Consider this code:

$cache_a = cache('a');
$cache_b = cache('b');
// Both bins are now set to bin b, so this would look in bin b.
$cache_a->get('something');

So we probably can't do this, not sure how to solve it yet.We could re-introduce caching and clone them or something like that.

@cosmicdreams: I'm not sure why that code was moved, was part of the original patches from chx, maybe to have it consistent with the order in which it is called? Likely not relevant to this issue but this is still in stage "figuring out what's possible" and not "getting it ready for being commited", so not worrying about stuff like that too much atm :)

If I'm reading the patch correctly, I think Berdir's sample code at the end of #41 is exactly why it won't work. :-) Having an object you get back from the DIC randomly change out from under you is a show-stopper, especially when you consider that we want to not be using cache() all the time but injecting the cache object as a dependency to other objects.

I really don't see an issue with having a separate instantiated object for each bin, though, so I don't understand why it should be an issue. The number of bins never gets all that large, or if it does you're doing something wrong. Why exactly are you allowing the bin of a cache object to change after it's been instantiated? That is the part that strikes me as "eeek, run away!!"

Yes, yes, that is mostly why the patch failed :) cache.config is now used like that and falls apart.

The problem is, it's not that easy, there are at least three reasons that make "Let's just register every cache bin as a service in the DIC" problematic:
- There are quite a few cache bins, in fact. Core alone has 13 (cache_update and cache_locale don't really con't and will be replaced by KV, so 11). A 7.x site that I'm working on has 25.
- Configurability. If we register every single bin, does that mean that memcache has to override each of them explicitly?
- Even if we wanted to register all of them, we can't. We don't *know* which cache bins exist. Yes, there is an issue that will register them in a hook, but hooks aren't available when we want register them?

One approach that I'm currently trying is using the service as a "template object", clone it for each bin when requested and store these in the factory. We could also store them in the container using set() but I don't know if that will work after it's been compiled to the disk?

25 cache bins sounds to me like "you're doing something wrong". Really, that's a red flag to me.

I'd presume that a module that says "I need a foo cache bin" would register a cache.foo service, and then it knows that service is there and will start calling it. Other modules that don't know about that module would not know about cache.foo, nor should they even care. If they care, that's a bug.

If you're swapping out a given bin for memcache, then you know the bin exists so you know to replace it.

I really don't think cache bins are or should be as fluid as we are trying to make them. :-)

25 cache bins is the obvious result of the current situation, we only have per-module cache bins, with some exceptions. So each contrib module that needs to store more than a few entries currently defines it's own bin because there is no existing one that it could use. #1194136: Re-organise core cache bins tries to change that by introducing more

The thing is that we have a lot of interdependencies between these issues. To get the number of cache bins down, we for example need to move cache_update to KV, but that needs the database changes contained in this issue and so on. So in case we get somehow blocked here, we could consider moving that into a separate issue and get it in, to solve the deadlock :)

I'm really unsure about the configurability of cache bins. Right now, it's easy to specifiy that Memcache should be used by default by setting two variables in settings.php. If we make it explicit, you would need to check after each installed module if there's a new bin provided by it and update the configuration. Also, I'm not sure about the order, given that mymodule would register cache.mymodule in MymoduleBundle. Maybe there could be a default service configuration and some helper code?

Will continue later with this, have some code that I'm still working on.

Hum I have to agree with both Crell and Berdir, in one side I'd prefer to do a direct registration of all bins into the DIC and keep out the factory, but in another I'd like to keep the factory because it could help building some workaround like returning null implementations when database is not active.

Yes it's currently possible to register a backend as the default for all cache bins site-wide, because the cache backend configuration is static, whereas the cache bins themselves are dynamic.

There's no way to get a list of all cache bins without invoking hook_flush_caches(). #1167144: Make cache backends responsible for their own storage would encapsulate that hook a bit but doesn't fix any of the problems here.

If we go in any direction I'd prefer to see the cache bins be less dynamic and the cache configuration stay static, not the other way 'round (because that'd mean writing to cache configuration on module enable/disable automatically which just feels fragile).

Does registering the cache bins separately mean that we'd have to instantiate a real connection with the backends each time? Something like cache_mymodule might not ever be used during a request and if I have a memcache bin set up separately for it for some reason I'd not want that overhead if it's not going to be used during the request.

catch: If we go all the way to a factory-less cache.page, cache.filter, etc. registration straight on the DIC, no. If cache.filter is backed by memcache, and memcache is a service in the DIC, then the first time you request cache.filter the DIC will notice that it depends on the "memcache" service ID and instantiate that, then pass that to the constructor of the class wired into cache.filter. If you never request cache.filter, then it never causes the "memcache" service to initialize. (Unless some other service asks for it, of course.)

Ok. Here is a patch that explicitly registers every bin that we could possibly use. Currently doing this in a loop in bootstrap.inc, non-system.module bins would need to be moved to $moduleBundle.

What's totally not clear to me is how to override this to use Memcache for all bins.

For the ones directly registered in drupal_container(), we could use drupal_container()->register('cache', '...'). But I totally don't get yet how that would work for those defined in moduleBundle because that is invoked after settings.php?

Any pointers are welcome.

Fixed the InstallBackend and the upgrade tests. update.php really calls cache('update') ? Weird, it shouldn't do that IMHO, could lead to more of the ugly fetch task queue duplicates...

It's even weirder to replace it by a direct table truncate, it should remain a cache() call instead. A lot of other cache() calls will happen during various installation tasks.

No, cache_update is not a cache. Among other things, it contains fetch_task entries which are used to prevent duplicate queue items. Deleting them means we create duplicate fetch records. So the truncate is just as wrong, that is correct. I think we might be able to remove it completely, because update.module has a special implementation of hook_cache_flush() that deletes stuff on update.php runs. Not sure why this is duplicated.

I've explicitly not registered cache.update as a cache backend service, that's why it failed, not because it's called on update.php.

I'm converting cache_update to the key value storage in another issue but that issue needs this one to be able to use the database in early bootstrap.

Oh right, forgot that one, it's not a cache... Yes so you're right here sorry for the noise.

Explicit registration++

I suggest, however, in CoreBundle:

  $container->register('cache', 'Drupal\Core\Cache\DatabaseBackend')
    ->addArgument(new Reference('database'))
    ->addArgument('cache');

Adding another $bin:

$id = 'cache.' . $bin;
if (!$container->has($id)) {
  $definition = clone $container->getDefinition('cache');
  $container->setDefinition($id, $definition->replaceArgument(1, $bin));
}

The $container->has() would allow, eventually, for overrides.

What the real benefit of cloning the entry VS. explicitely registering it with the database reference? It makes it less readable, especially with the replaceArgument and all.

We can probably explicitly register as well. We also discussed about using tags (definitions can have tags) to identify cache bins instead of looking through the ids), so the default definition might get a bit bigger, but we could wrap this thing in a helper function and then it doesn't really matter if it's a clone or not for those using it.

Yep that was a minor detail, tags is a good idea indeed, we could get rid of cache backends reference in the factory and use it for retrieval for cache clear all or tags clearing operations.

I like the sound of #57, but then we'd likely want to reverse the parameter order so the bin is the first constructor argument, always. That way if you have a cache implementation that needs 2 or more additional parameters, the position of the bin name argument doesn't change. That's an easy fix.

Tagging cache services sounds sensible, too, and in fact may serve as a replacement for #1167144: Make cache backends responsible for their own storage

I'm liking this.

Agree with #61, we could imagine something like this:
* Register the default cache backend like any other component in the DIC
* Tag it in the kernel
* chx's idea is not that bad even if I'd prefer people to set their own definition: in anyway document that all registered cache bins must be tagged
* Generalize use of the tag for fetching backend list instead of the #1167144: Make cache backends responsible for their own storage
* Document how to override the class used for a specific bin
* Expose onto the factory various methods such as flush, flush tags, and potentially a list helper
* Remove from the factory any other code

Pros:
* Get rid of a yet another hook
* Every feature we need here DIC is already capable of doing it
* Untangle the factory from procedural code (removes the varialbe_get() of #1772682: Remove the variable_get inline in CacheFactory::getBackends
* Everything gets compiled
* Hardcoded list of bins easily retrive-able for various operations (flush, tags flush and UI listings)
* Fix all the procedural wrappers to use that

Cons:
* Less easy to understand maybe, but people generally know how to read documentation
* Uneasy to set a "default" class
* If people don't know how to read documentation, we'd have to find a way to move this out of hardcoded bundle registration and set those into a config file for override (sounds like a "Pros" in the end) used before compile if found
* Not a Drupalism :) (I should have put this in the "Pros" category)

Ok, here is a new patch.

- Changes: Uses an adapted version of chx's code to define the non-bootstrap bins in CoreBundle. Currently all definitions are still created in CoreBundle, we can move them to per-module bundles once we agreed on the exact syntax. Also note that I'm working on generalizing the cache bin names so that we can get rid of module-specific bins in Core (except cache_test :p), so they will in fact go back to CoreBundle, see #1194136: Re-organise core cache bins.
- Using the first argument for the bin, as Crell suggested. To enforce this, we would have to make a separate set method in the interface for this as we don't want to explicitly define the
- Using tags to identify cache services.
- Using the correct query copied from update.authorize.inc to flush update.module caches in update.php. I think this is actually a separate bug and this change should be backported to 7.x as well.
- No helper function to use the clone trick to define the cache backend, Crell almost beheaded me as he heard me adding a function :)

Notes:
- It's not exactly nice, but it's actually possible with this code to properly override cache bins in settings.php. The following example has been tested and works as expected:

// Manual namespace registration.
drupal_classloader_register('apc', 'modules/apc');

// The cache definition is what later bins will use. Those three need to be overwritten explicitly.
drupal_container()->register('cache', 'Drupal\apc\ApcBackend')
  ->addArgument('cache');
drupal_container()->register('cache.bootstrap', 'Drupal\apc\ApcBackend')
  ->addArgument('bootstrap');
drupal_container()->register('cache.config', 'Drupal\apc\ApcBackend')
  ->addArgument('config');

// It is possible to create a specific bin in advance.
drupal_container()->register('cache.form', 'Drupal\Core\Cache\DatabaseBackend')
  ->addArgument('form')
  ->addArgument(new Reference('database'));

- While testing the above configuration, I found an interesting bug. The database is now solely configured through the DIC. If the first request does not go through DatabaseBackend but e.g. db_query() then the Database never receives the necessary connection information and the site explodes. Discussed with Crell and we agreed this can be solved in a separate issue as the Database class should be refactored a bit anyway to make it easier to embed it into the DIC.

- While we could throw out the default/clone stuff and rely on explicit overwrites only if people don't like that, we at least need to keep the !$container->has() check IMHO or it is impossible to overwrite cache bins from settings.php. Having to add a module with a bundle class, making sure that it has a high weight and so on is IMHO the wrong approach, considering that this is environment-specific configuration.

Here is PoC I was working on during the time you wrote yours:
* Took your previous patch as base.
* Added the tags, and added a CacheManager object for iterating over.
* Followed http://symfony.com/doc/2.0/components/dependency_injection/tags.html and added a compiler pass for registering bins in a container parameter.

If you interdiff both you'll see I didn't edit any of your code, just added tags, the compiler pass, the cachemanager object, and their respective registration.

I think tagged services and a compiler pass are definitely the way to go here, and I have a feeling there are several other places this pattern could be made use of. The problem is we can't use compiler passes until #1706064: Compile the Injection Container to disk gets in - I will try to at least get that patch green again tomorrow.

Forgot to switch the arguments in install.core.inc. This should do better.

@pounard: That code looks interesting, but currently dies with "Fatal error: Class 'Symfony\Component\Config\Resource\FileResource' not found", probably because of what katbailey said in #67. I merged it with my latest code and have it in a separate local branch, so I can re-add it once it can work.

@Berdir, I just didn't tested the code, because since a recent commit no D8 will install on my box (even unpatched) and I can't figure out why. It seems it's because of early registration of CMI stuff...

You shouldn't experience this error, I think this is due to a missing use statement, maybe, but written correctly it should work!

@katbailey Compiler passes are supposed to work even if not compiled I thought? I mean, it's slower but behavior shouldn't change with or without compilation since we already are using all the compilation ready tooling, or I am wrong here?

EDIT: Re @Berdir I saw a lot of install stuff being patched, I think we can make it less invasive regarding the DIC by building a DIC with scopes ("config-ready", "database-ready", "system-ready" and "request"): each scope defines components, until the database is not ready cache are null implementations, and after database ready the DIC overrides itself by entering the "database-ready" scope which overrides the previous definitions and makes normal cache available. This allows to have one single big bundle for Core, and may help removing the drupal_container() custom container which sounds heretic IMO.

@pounard The DependencyInjection container relies partially on Symfony's Config component, which we don't use/have. Looks like this compiler pass stuff is one of the things that do rely on it. In a quite hardcoded way (new FileResource()) Looks like the Dependency injection container does not follow the dependency injection rules :p

Still working on this. The cache.field error is because run-tests.sh is not booting a Kernel and then tries to invoke hooks, which rely on stuff that is registered in CoreBundle, then it breaks. That can be easy fixed but after that, I'm getting a very weird behavior in module.inc, config('system.module')->get('enabled') returns NULL instead of the enabled modules. Haven't figured this out yet, any pointers would be appreciated.

Oh, didn't mean to change the status.

@Berdir Ok, I understand, this is bad for we'll have to use the Config component or write a compatible null implementation. Drupal 9 will be SF2 full stack I guess ^^

@Berdir the config('system.module')->get('enabled') must happen either because:
* Site is not fully installed yet, and this shouldn't be called OR
* #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config introduced bugs
You should ask sun for this.

The failure only happens after the test execution, after the (static) caches have been emptied and the function is called again. Something seem to go sideways there, not sure yet if there's something wrong with this patch or this patch just exposes it.

@pounard yes, compilation means two things - running compiler passes and compiling to php. We had the former working without the Config component but it was deemed not performant enough until we get the latter in. #1706064: Compile the Injection Container to disk deals with both of these aspects of "compilation".

@katbailey My question was more: even if not compiled to disk, the behavior should not change and the compiler pass should run, right? As soon as we are building a ContainerBuilder the pass should be runtoo? If not, I don't know how SF developers can debug their own apps since the compilation passes oftenly add business related stuff which are mandatory for the app to run.

@pounard see this line here: http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/lib/D...
No compiler passes are being run - which is why all the event subscribers are being instantiated and added to the dispatcher instead of being added as tagged services and using a compiler pass:
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/lib/D...

Figured it out, I have to move the container above the module_list_reset() call in TestBase::tearDown(). A bit confused by the duplicate reset calls in TestBase and WebTestBase but that's not relevant here.

Let's see if this gets us back on track.

Ok, had to move the page bin to drupal_container() and pretty much all bins that we have to update.php to get the upgrade tests working. On the other side, was able to remove the test bin because that is just used a direct argument to DatabaseBackend()/InstallBackend().

Let's see if any test fails remain.

Adding more bandaids to get the upgrade tests working. UpgradePathTestBase is ugly...

+++ b/core/includes/bootstrap.inc
@@ -2133,14 +2136,14 @@ function drupal_bootstrap($phase = NULL, $new_phase = TRUE) {
-        case DRUPAL_BOOTSTRAP_PAGE_CACHE:
-          _drupal_bootstrap_page_cache();
-          break;
-
         case DRUPAL_BOOTSTRAP_DATABASE:
           _drupal_bootstrap_database();
           break;
 
+        case DRUPAL_BOOTSTRAP_PAGE_CACHE:
+          _drupal_bootstrap_page_cache();
+          break;

Strictly speaking, if we're changing the order of these phases their numbers should change, too.

+++ b/core/includes/cache.inc
@@ -57,7 +36,18 @@ function cache($bin = 'cache') {
+/**
+ * Returns a list of cache backends for this site.
+ *
+ * @return array
+ *   An array of cache backend service names.
+ */
+function cache_backends() {
+  return array_keys(drupal_container()->findTaggedServiceIds('cache'));
+}

Can we put this somewhere other than a function? If the CacheFactory class still exists that might be a good place. Or, it's a one liner, just don't bother with it. Really, I would just not bother with it. :-) cache_invalidate() has to call drupal_container() anyway for now.

+++ b/core/lib/Drupal/Core/Cache/InstallBackend.php
@@ -34,6 +35,22 @@
+  function __construct($bin) {
+    if (class_exists('Drupal\Core\Database\Database') && drupal_container()->hasParameter('database.info')) {
+      $this->dbConnection = Database::getConnection('default', NULL, drupal_container()->getParameter('database.info'));
+    }
+    if ($bin != 'cache') {
+      $bin = 'cache_' . $bin;
+    }
+    $this->bin = $bin;
+  }

I'm unclear why the InstallBackend is doing this, but then I've never really understood why the InstallBackend extends DatabaseBackend rather than just being a null implementation. Why do we need two static calls in what should be a noop implementation?

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -144,15 +144,20 @@
   /**
    * Gets the connection object for the specified database key and target.
    *
-   * @param $target
+   * @param string $target
    *   The database target name.
    * @param $key
    *   The database connection key. Defaults to NULL which means the active key.
+   * @param array $database_info
    *
    * @return Drupal\Core\Database\Connection
    *   The corresponding connection object.
    */

$database_info should have a description. You can probably steal some existing docs from somewhere. Or just reference settings.php.

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -144,15 +144,20 @@
-  final public static function getConnection($target = 'default', $key = NULL) {
+  final public static function getConnection($target = 'default', $key = NULL, $database_info = array()) {
+    if (empty(self::$databaseInfo)) {
+      self::parseConnectionInfo($database_info);
+    }

I should note this is not the final state we want to have for the DB, but I'm OK with this approach for now. We can/should refactor later.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/ModulesDisabledUpgradePathTest.php
@@ -36,6 +38,16 @@ public function setUp() {
+    drupal_container()->register('cache.field', 'Drupal\Core\Cache\DatabaseBackend')
+      ->addArgument('field')
+      ->addArgument(new Reference('database'))
+      ->addTag('cache');
+    drupal_container()->register('cache.menu', 'Drupal\Core\Cache\DatabaseBackend')
+      ->addArgument('menu')
+      ->addArgument(new Reference('database'))
+      ->addTag('cache');
+

There's no need to call drupal_container() multiple times. Just call it once, save the $container variable, and call it a few times.

I'm in nitpicking mode now, I think. The overall approach is IMO solid. Berdir++

Thanks for the review.

- Fixed the remaining exceptions, had to move the container restore above the static reset call. That one seriously results in a system_list() call, through __destruct() functions of the cache array class.
- Changed value of those constants.
- merged cache_backends() into cache_invalidate().
- Not sure what to do with InstallBackend. AFAIK, we can *not* replace it with NullBackend, because it needs to do those cache clears if it can. We could possibly inject the database there conditionally as well, to keep the logic of that outside of the class and avoid functions calls. Will do that when this patch is green again.
- Changed the duplicate calls to drupal_container(). Not happy with the messy situation in the upgrade tests.

The problem with the NullBackend is for a different issue. For me, the NullBackend is an EXTREMELY ugly workaround. It should definitely use NullBackend at some point but due to the current behavior of the installer that's not yet possible. What the installer backend basically does AFAIK is: It clears the database cache entries that were produced as a result of AJAX callbacks that are already using the normal DatabaseBackend during the batch process. If that's not fucked up then I don't know... :P

Yeah, the renumbering actually broke stuff. Reverted that and also the changed order within the switch, that does not have any effects anyway.

@Berdir The findTaggedServiceIds() method is not available on the ContainerInterface, it is only available on the ContainerBuilder specific implementation: this is why the compilation pass is mandatory for this to work. If you use it outside of a compiler pass you are violating the interface contract and making the code fragile.

+++ b/core/includes/bootstrap.incundefined
@@ -725,13 +724,16 @@ function drupal_settings_initialize() {
   // Make conf_path() available as local variable in settings.php.

@@ -2441,12 +2446,31 @@ function drupal_container(Container $new_container = NULL, $rebuild = FALSE) {
+      ->addTag('cache');
+    foreach (array('bootstrap', 'config', 'page') as $bin) {

Is there any reason to make this array hardcoded?
Possibly we should make it configurable via yml ?
Or may be I`d missing something here ;(

CMI has a dependency on the cache API. So we can't use CMI anywhere there.

This is the hardcoded list of cache bins that are required in early bootstrap. Modules can define their own bins in a bundle class.

You can see that the other ones are defined in CoreBundle currently #1194136: Re-organise core cache bins will reduce the amount of bins we have in Core and, instead of specific per-module bins, introduces generic bins that can be re-used by othe rmodules, which should eliminate the need for custom bins in most cases.

if (!isset($container->getParameter('initial_cache_bins')){
$container->setParameter('initial_cache_bins', array('bootstrap', 'config', 'page'));
}

with this can we replace it via settings.php ?

woops

I'm not sure this would make sense: bins are registered by modules that will explicitely use it (hardcoded bin names) and I'm not sure we have any use of having a "dynamic user bin registration" feature: if the site admin adds bins in there, they probably will remain unused forever or conflict with already registered module bins.

There's nothing user facing in this patch nor is it advocated. What is advocated is that overriding the default cache changes everything and for that reason I would to see the bootstrap container to use the getDefinition pattern as well.

I was speaking about settings.php.

The question on settings.php is valid. It's not about registration of cache bins, but about specifying which backend to use for which cache bin.

This patch seems to hard-code the database backend for all cache bins, unless I'm missing something big (unlikely, but possible). So the overly simple question is:

What do I put in my settings.php to make Drupal use APC for all cache bins?

Right now, all I need to do is this:

$conf['cache_classes']['cache'] = 'Drupal\Core\Cache\ApcBackend';

(Test yourself with #1807662: Built-in APCu support in core (PHP 5.5 only) ;))

I definitely agree we need a configuration entry for setting those backends, but the psuedo code written upper doesn't relate this need.

@chx: Using getDefinition() does not help there. getDefinition() only helps if settings.php has a chance to replace the default cache bin definition before those bins are added. That's not the case for those defined in drupal_container().

#63 has an example how this can be configured. Yes, it is more complicated than what we currently have. We might be able to simplify it a bit by looping over the backends but as @pounard said, it's not save to use findTaggedServices() outside of a compiler pass thingy, so as soon as we do the compilation, it won't work anymore.

Yep, the SF2 documentation gives a very good example with tag handling, we should tie to this method.

Ok, here is an update.

- Passing the database to InstallBackend if available, so moving that logic outside of the class. Note that there is an important difference in any patch in this issue (not just this one) for the InstallBackend. It does not act if the database becomes available during request, only if available initially. Not sure if that is a problem, might be?
- Updated the documentation in CacheBackendInterface. Quite outdated stuff in there, e.g. non PSR-0 example class names.
- Added all cache bins to UpgradePathTestBase, removing one-off registrations from subclasses. This is the same behavior as we have in update.php too, so I think this makes sense. Also standardized all cases to use clone getDefinition().
- Some general cleanup and improvements.

Let's see if I introduced any bugs with this.

#103: cache-dic-1764474-103.patch queued for re-testing.

#103: cache-dic-1764474-103.patch queued for re-testing.

something wrong with a bot?

No, the patch is.

The installer is crazy, the actual error is usually hidden below two meaningless errors, because trying to display the error results in a dependency injection exception and then trying to handle that exception results in yet another one.

Forgot to add the use statement. Also removed some debug code that I left in there accidently.

despite of these hardcoded arrays

+++ b/core/includes/bootstrap.incundefined
@@ -2440,12 +2442,29 @@ function drupal_container(Container $new_container = NULL, $rebuild = FALSE) {
+      ->addTag('cache');
+    foreach (array('bootstrap', 'config', 'page') as $bin) {

+foreach (array('block', 'field', 'filter', 'path', 'form', 'menu') as $bin) {

+++ b/core/update.phpundefined
@@ -448,15 +454,16 @@ function update_check_requirements($skip_warnings = FALSE) {
 +foreach (array('block', 'field', 'filter', 'path', 'form', 'menu') as $bin) {

all other looks good for me

The first three are core cache bins, installed by the system module and used for early bootstrap, so there is no other choice. And in the end, modules defining bins will have to hardcode their registration into their respective bundles, so those hardcoded names sounds legit.

can we move those arrays into settings.php ?

@podarok: Once the patch is more or less done they will be moved to the bundle of the module that they belong. So the second loop in bootstrap.inc will go away. Not sure if the first one can or should be moved to CoreBundle. Moving them to settings.php doesn't make sense imo.

#116 Thanks, i got it
Looks like we need update documentation here with @todo for feature follow-ups

No, the second loop in bootstrap.inc will *not* go way. Those are bootstrap caches that are required before the bundles are loaded. No module can be called there, so there is little chance that something could use a different bin than these. And if it would actually be necessary to overwrite anything in there, then that could be done in settings.php, which is about the only case that could execute something at that point. I think.

The loop in CoreBundle is the one that should be distributed to per-module bundles, but the thing is that #1194136: Re-organise core cache bins will move them back because there will only be a fixed set of bins *in core* (modules still have the possibility to define their own, but the ones in core are then actually designed to be generic instead of added to the module that needs them).

The only ones that are a problem IMHO are those in update.php and UpgradePathTestBase. Because other modules might not have the chance to register their bin. This might get solved if update.php is converted to use a Kernel.

The problem there is that the cache bins are flushed according to hook_cache_flush(). As a follow-up of this issue, we might actually want to kill that hook (or rename it and keep it for special flush stuff like the thing in update.module) and flush all caches that are registered in the service.

Didn't mean to remove the tag.

@#118 Having two containers is heretic, "there's an issue for that" (TM) somewhere to reduce the pre bootstrap stuff to the maximum and move them into the core bundle where they belong. EDIT: The only blocker is the module list, it's the bare minimum to be able to build the full container using module bundles. The problem is that since now the module list uses both config and k/v, which themselves use cache and database, it forces us to continue this pre-bootstrap standalone non compiled container, which is very bad...

Sorry. I didn't look at the patch again and so I thought that the second loop listed in @podaroks comment was in bootstrap.inc too. So I was refering to the same loop that you were talking about @berdir. I just was wrong with the file that it is in :). So.. Yes, I meant the loop on the modules -> That one should be removed in favor of per-module bundles.

#118 updated summary with hook_cache_flush() feature pointed to Your comment for pointing this in @todo`s

Not marking RTBC yet in case someone else has comments, but I think I've run out of things to object to in #112.

For sure settings.php can set up cache as it wishes so I have wrapped that in a has() much like the rest of the patch does and added plenty of comments. It's a minute change and for the rest everyone agree so I mark this RTBC.

I really don't understand the need for these has() calls there. By calling drupal_container() in settings.php, which you need to do first, this code is run and those services are registered. The container class is created with new ContainerBuilder() right above, there is exactly zero chance someone could have added a cache service between new DrupalContainer() and register('cache')?

And if settings.php would actually create a completely new Container and pass it into drupal_container(), then the whole code block would never be executed.

Given that, exactly how could the cache service already exist at that point? Those 4 services/bins need to be overwritten using the example code that I've added to the documentation of CacheBackendInterface.

That sounds like a needs review.

Ooops... totally right, of course. #112 is the RTBC one then, reuploading here to avoid confusion.

This looks like it has catch written all over it. :)

+++ b/core/includes/bootstrap.inc
@@ -725,13 +724,16 @@ function drupal_settings_initialize() {
     include_once DRUPAL_ROOT . '/' . $conf_path . '/settings.php';
+    if (!empty($databases)) {
+      drupal_container()->setParameter('database.info', $databases);
+    }

@@ -2440,12 +2442,29 @@ function drupal_container(Container $new_container = NULL, $rebuild = FALSE) {
+    // This will be overridden by settings.php in drupal_settings_initialize().
...
+    $container->register('cache', 'Drupal\Core\Cache\DatabaseBackend')
+      ->addArgument('cache')
+      ->addArgument(new Reference('database'))
+      ->addTag('cache');
+    foreach (array('bootstrap', 'config', 'page') as $bin) {
+      $definition = clone $container->getDefinition('cache');
+      $container->setDefinition('cache.' . $bin, $definition->replaceArgument(0, $bin));
+    }

It still does not seem to be possible to define the cache storage to use (for all cache bins) in settings.php? This is a critical regression from D7, and I'm afraid, we don't have any critical follow-up issue slots available. :-/

+++ b/core/includes/install.core.inc
@@ -372,6 +387,15 @@ function install_begin_request(&$install_state) {
+    $container->register('cache.form', 'Drupal\Core\Cache\DatabaseBackend')

Let's add a @todo + @see here for the issue that replaces this with a persistent non-cache storage.

+++ b/core/includes/install.core.inc
@@ -496,7 +520,14 @@ function install_run_task($task, &$install_state) {
+          // drupal_retrieve_form() tries to load the current menu item
+          // if this is not set.
+          'files' => array(
+            'menu' => FALSE,
+          )

Why does this suddenly become an issue? Such changes generally indicate that something else must be wrong.

+++ b/core/includes/install.core.inc
@@ -939,8 +970,9 @@ function install_verify_completed_task() {
 function install_verify_database_settings() {
-  global $databases;

@@ -963,13 +995,16 @@ function install_verify_database_settings() {
 function install_settings_form($form, &$form_state, &$install_state) {
-  global $databases;

@@ -1047,7 +1083,6 @@ function install_settings_form_validate($form, &$form_state) {
 function install_database_errors($database, $settings_file) {
-  global $databases;

TestBase and WebTestBase contain various @todos on the fact that the installer does not actually use the connection info in Database. By removing global $databases from the installer, those need to be resolved with this change.

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -144,15 +144,21 @@
-  final public static function getConnection($target = 'default', $key = NULL) {
+  final public static function getConnection($target = 'default', $key = NULL, $database_info = array()) {
+    if (empty(self::$databaseInfo)) {
+      self::parseConnectionInfo($database_info);
+    }

This looks like a pretty bad hack - the getConnection() method is cleanly separated from connection info management methods currently. I do not think we want to hi-jack that clean design and separation. This change effectively means a ::setInfo() in a ::getConnection() method.

+++ b/core/lib/Drupal/Core/Database/Database.php
@@ -359,9 +362,9 @@ public static function addConnectionInfo($key, $target, $info) {
-  final protected static function openConnection($key, $target) {
+  final protected static function openConnection($key, $target, $database_info = array()) {

Same as above, but much worse.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -896,6 +896,9 @@ protected function tearDown() {
+    // Restore the original container.
+    drupal_container($this->originalContainer);

@@ -907,7 +910,6 @@ protected function tearDown() {
     // Restore original statics and globals.
-    drupal_container($this->originalContainer);

These changes should be done in the dedicated issue instead:
#1810990: Teared down test environment leaks into parent site/test runner

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -613,6 +613,8 @@ protected function setUp() {
+    // drupal_container() does not use drupal_static().
+    drupal_container(NULL, TRUE);

TestBase sets up a new container already, so why is this needed?

It still does not seem to be possible to define the cache storage to use (for all cache bins) in settings.php? This is a critical regression from D7, and I'm afraid, we don't have any critical follow-up issue slots available. :-/

It is possible, as described above. Not in a single line, but not *that* many. It is documented in the patch and relies on the fact that later cache bins will clone the definition of the default cache bin, so if that was changed in settings.php to APC, they will use APC as well.

 *  $container = drupal_container();
 *  $container->register('cache', 'Drupal\mymodule\MyCustomCache')
 *    ->addArgument('cache')
 *    ->addTag('cache');
 *  foreach (array('bootstrap', 'config', 'page') as $bin) {
 *    $definition = clone $container->getDefinition('cache');
 *    $container->setDefinition($id, $definition->replaceArgument(0, $bin));
 *  }

Yes, it's not as nice as what we can currently do. On the other side, this likely makes the suggested hook_cache_bin_info() unecessary and should also be able to eliminate hook_cache_flush() in the way it exists now. We know which bins are around, so we can just loop over them and delete them. @pounard has posted a nice cache manager object above, but that can't happen until we compile the DIC, so let's open a follow-up: #1811724: Introduce cache manager to invalidate tags and flush all bins. We could also use this to provide an easy method for adding new cache bins using that method I guess.

+++ b/core/includes/install.core.inc
@@ -372,6 +387,15 @@ function install_begin_request(&$install_state) {
+    $container->register('cache.form', 'Drupal\Core\Cache\DatabaseBackend')

Let's add a @todo + @see here for the issue that replaces this with a persistent non-cache storage.

#512026: Move $form_state storage from cache to new state/key-value system. There is a already a patch, just waiting for this and #1547008: Replace Update's cache system with the (expirable) key value store to happen so that we can use the expirable keyvalue storage in maintenance/install mode. *However*, that will not make the cache.form bin unecessary, it only moves the parts out that are *not* a cache. #1194136: Re-organise core cache bins is also waiting for all these issues to happen to get the number of our bins down to a handful generic ones.

Why does this suddenly become an issue? Such changes generally indicate that something else must be wrong.

Not sure, will check if it's still necessary. It certainly was at some point.

This looks like a pretty bad hack - the getConnection() method is cleanly separated from connection info management methods currently. I do not think we want to hi-jack that clean design and separation. This change effectively means a ::setInfo() in a ::getConnection() method.

Yeah, it is. The correct way to fix this is to refactor the static Database class into one that lives within DIC and receives the necessary information in the constructor. Quoting Crell from #85: "I should note this is not the final state we want to have for the DB, but I'm OK with this approach for now. We can/should refactor later.". Follow-up created here: #1811730: Refactor Database to rely on the DIC solely. This issue is blocking a number of improvements and cleanup's for the cache system and I really hope we can get it in asap.

Ok, here we go.

- I did not add a @todo about cache.form because we don't want to replace it, we want to make it a normal cache. And there's already a major issue which is about doing that, with a patch, blocked by this issue :)
- I was able to remove the change in install_run_task(), seems to work fine now, must have been related to another issue.
- I was also enable to remove the drupal_container() call in WebTestBase::setUp(), tests seem to work.
- Added a @todo to Database::getConnection(). I actually realized that by moving the database correctly into the DIC, this will make the setUp()/tearDown() much much nicer. setUp() just needs to create a new container and set the database.info parameter. Done. tearDown() will just need to restore the original container. Done. Awesome :)
- Found and removed an now unecesary code snippet that restored global $databases, this is now restored as part of the container restore.
- Also, the drupal_container() move in tearDown() might be wrong, but is 100% in line with the current code. Here's why: Right above that, we restore the database to the default, then call those resets. So we are already executing them against the default database right now. The difference is that the cache backends now have an instantiated connection object instead of calling the current global connection. Moving it above means that the behavior is not changed. I think I know how to solve this properly, will comment in #1810990: Teared down test environment leaks into parent site/test runner. IMHO, the change in this patch makes sense given the current behavior.
- The remaining @todo: in TestBase about $databases is I think still true, so I left it there:

    // Additionally set database.info, since the installer does not use
    // the Database connection info.
    // @see install_verify_database_settings()
    // @see install_database_errors()
    // @todo Fix installer to use Database connection info.
    $databases['default']['default'] = $connection_info['default'];
    drupal_container()->setParameter('database.info', $databases);

It is not a global anymore, but we still have to set it in two different ways. As mentioned above, once the Database is in the DIC, this will in fact be the only thing that we will have to do.

Thanks for the review, made a few nice cleanup's possible as one can see in the interdiff. Let's see if the testbot is happy.

EDIT: I'm actually not sure if we will replace the form cache bin completely or not, not sure what's correct, I'm going by catch's comment in the form_state issue, but looking at the code, I'm not sure if it makes sense to keep it. There is a major issue, so IMHO, not really necessary to add a todo that might or might not be true.

Strange. Something in drupal_container(NULL, TRUE) must depend on something in drupal_static(). It works when drupal_container() is below drupal_static_reset() but not otherwise. And it's even strange that only those two tests break.

I am deeply troubled by one aspect of this patch - if I am understanding it correctly, it is completely at odds with the effort to get rid of the "bootstrap container". See #1784312: Stop doing so much pre-kernel bootstrapping for background.

The bulk of the work for solving that problem is in #1331486: Move module_invoke_*() and friends to an Extensions class and that, combined with #1706064: Compile the Injection Container to disk, will result in the following set-up: on a normal page request, there is no container at all until the kernel creates one, and what it creates is a Container, not a ContainerBuilder. This means that nothing can affect the contents of the container without going via the kernel.

Obviously the ability to allow settings.php to affect how services are built is important, but I feel like this patch is going way off in one direction to solve that problem, and the work in the above two patches is going in the opposite direction (granted, not with that particular goal in mind, but with equally important ones IMO).

I know we have to solve one problem at a time and I don't want to hold this one up, but if it goes in like this I'm afraid it will be a huge setback for the above two patches.

I think sun is the one who first mentioned having a "SiteBundle" of some sort, where a site can register new services, or override services. I think that's the better approach to take. Then we simply add that bundle (if defined?) to the list of bundles to process when rebuilding the container.

See also: How to Set External Parameters in the Service Container

OK, based on #138 and #139 I am less deeply troubled about this ;-)
I still think it will be a ton of work to get both the DIC compilation and ExtensionHandler patches back on track after this patch, but at least it's fairly clear what needs to be done and, more importantly, that it can be done.

Kat: So what patch order would result in the least rewriting between these three? Let's coordinate that since we want all three to get in soon. :-)

Heh, that is a difficult question for me to answer objectively ;-)
I *think* that the order involving the least amount of rewriting would be:

• #1706064: Compile the Injection Container to disk
• #1331486: Move module_invoke_*() and friends to an Extensions class
• #1764474: Make Cache interface and backends use the DIC

... because I don't think either of those first two patches has side-effects that are at odds with the goal of this patch, whereas the converse is not true in either case. But of course it's easy for me to say that, seeing as I've worked on the other two and not on this one...

@chx has worked on all three, so maybe he's the best placed to give an objective opinion about this.

Ok, let's focus on the other ones then first. That will hopefully also make the performance decrease of this issue

Wha I'm going to do is extract the database changes form this issue and continue with that in #1811730: Refactor Database to rely on the DIC solely. Because it's only the database changes which are required to unblock my keyvalue issues, which would in turn help to simplify this patch.

Ok, re-starting this. See proof of concept patch.

This is the same approach as the one we're using for KeyValue. Database changes are no longer required, as the key value factory went the easy way to expose it for now. Applying the patch seems to work fine in an existing site, install will obviously not work yet.

This looks great, @Berdir.

I assume that the following steps should get us to green here:

- adding a factory for cache.memory, cache.null, and cache.install
- registering the cache.install service in install_begin_request()
- registering the cache.memory service in DrupalUnitTestBase::setUp()

@sun: Almost. The thing you forgot is CacheFactory::getBackends(). I'm not sure how to implement that, because I'm not sure what it's actually supposed to return.

Right now, it returns all backends that are explicitly registered. Let's say you use memcache by default, memory for A, database for B and apc for C then it returns 4 backends. Each is returned once. But if you use memcache by default and database for A and B, it will return three backends, 1x memcache and 2x database.

The implementation of the cache tag invalidation is currently based on the first case. It then invokes a method on each backend which it assumes is bin-agnostic ( I haven't found this being documented anywhere). There are already some issues about this and how it could be solved. The easiest would be to make tag invalidation a per-bin operation, but I'm not sure if that's actually possible.

If not, then the question is if that method should return bins or (distinct) backends. If backends, then the problem is that we currently have no official way to execute bin-agnostic methods. We could invoke it with CACHE_BIN_ALL or something like that, but that's a bit ... weird too.

Ok, I restored the current getBackends for the moment, that should allow to ignore it for the moment and get the other things working.

Added an install backend factory and made install work. Due to some crazy __destruct() stuff I had the problem that some cases still tried to access the install.backend service when it was no longer available. Fixed that by adding a fallback for now.

+++ b/core/includes/install.core.inc
@@ -346,7 +346,8 @@ function install_begin_request(&$install_state) {
-  $conf['cache_classes'] = array('cache' => 'Drupal\Core\Cache\InstallBackend');
+  drupal_container()->register('cache.install', 'Drupal\Core\Cache\InstallBackendFactory');
+  $conf['cache_bin'] = 'cache.install';

Shortly after this location, we're setting up a specialized container for the installer, and I think this should be moved in there.

I also think that we no longer need an install backend. Instead, the memory backend should be sufficient and suitable for the installer container, and once system tables have been installed, that container is replaced with the regular container, which can use the database backend as usual.

Simplification++ :)

@sun: I'm not sure, I think the tricky part are those ajax requests that might lead to cache sets but still need to be deleted when requested to do so. Other than that, InstallBackend is essentially a NullBackend. Agree that it might be possible to improve this, but it's not really relevant for the complexity of this issue We still need to overwrite the default with *something* on install.

Not sure what you mean with the specialized container part, there is one, but that is only used conditionally when there is no settings.php yet. That one was missing a definition for the cache factory, which I added now, this should make the installer work, I was testing with an existing settings.php. The cache.install definition is also needed when we use the default bootstrap container.

Installer backend is #1792536: Remove the install backend and stop catching exceptions in the default database cache backend let's discuss that issue there but yes it can't just be removed as it is without a regression still afaik.

@Berdir, we could loop over hook_cache_bin_info() and invalidate tags for each bin that way, then it's up to something like the database backend to avoid writing the tags over and over again itself. That would be cleaner than the assumption we have now probably. This would mean that tag invalidation would eventually depend on the extension manager but that's not too bad.

This would mean bringing in just the hook_cache_flush()/hook_cache_bin_info() change from #1167144: Make cache backends responsible for their own storage into here though. That issue now depends on this one (because it needs update module to not be hard-coding database queries to its cache table).

Added memcache factory and using that in DrupalUnitTestBase.

Somehow, running tests within a test is currently broken, looks like the tables don't exist. Haven't figured that out yet.

Ok, re-rolled this works fine through the installer for me. Testbot, be nice?

Ok, the cache decorator tests are easy to fix, but I'm really strugglying with the other two, both have the same problem, they fail to run tests within a test.

The reason seems to be that the state query in drupal_get_installed_schema_version() connects to the parent simpletest site, where those modules are already enabled. Obviously. So it doesn't install the schema of user.module and then breaks. But it only happens within a test, not for normal test and I can't figure out what's different.

Someone has an idea what this could be? :(

#156: cache-dic-re-revival-1764474-155.patch queued for re-testing.

Ok, here is a re-roll. Those weird schema issues might have been fixed in the meantime in other issues, let's see what happens!

+++ b/core/lib/Drupal/Core/Cache/CacheFactory.phpundefined
@@ -7,19 +7,41 @@
 class CacheFactory {

Just a small point, shouldn't all classes which depends on the container implement the ContainerAwareInterface?

+++ b/core/lib/Drupal/Core/Cache/CacheFactory.phpundefined
@@ -42,7 +77,7 @@ public static function get($bin) {
+  public function getBackends() {

You talked about getting rid of that earlier in the issue, but couldn't pull that information from the container using tags on the individual cache factories?

+++ b/core/lib/Drupal/Core/Cache/InstallBackend.phpundefined
@@ -34,6 +35,19 @@
+  function __construct($bin, Connection $connection = NULL) {

+++ b/core/lib/Drupal/Core/Cache/InstallBackendFactory.phpundefined
@@ -0,0 +1,27 @@
+    return new InstallBackend($bin);

The InstallBackendFactory seems to require to inject the database connection?

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -49,6 +49,18 @@ public function build(ContainerBuilder $container) {
+      ->register('cache', 'Drupal\Core\Cache\CacheFactory')

Wouldn't it help a bit if the service would be called cache.factory, so the name explains what is behind? On the other hand $container->get('cache')
is simplier to read.

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -49,6 +49,18 @@ public function build(ContainerBuilder $container) {
+    $container->register('dispatcher', 'Symfony\Component\EventDispatcher\EventDispatcher')
+      ->addMethodCall('addSubscriber', array(new Reference('config.subscriber.globalconf')));

So additional to the event_dispatcher we register a dispatcher, which is not container aware, so we can't rely on container tags. I'm wondering whether this is really cleaner the rely on the container.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -736,7 +736,7 @@ protected function setUp() {
+    unset($conf['cache.bin']);

Shouldn't this be cache_bin?

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/DatabaseBackendUnitTest.phpundefined
@@ -29,7 +30,7 @@ public static function getInfo() {
+    return new DatabaseBackend($bin, Database::getConnection());

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/InstallTest.phpundefined
@@ -49,8 +50,8 @@ public static function getInfo() {
+    $database_cache = new DatabaseBackend('test', Database::getConnection());
+    $install_cache = new InstallBackend('test', Database::getConnection());

Is there a reason why not to use the database connection from the container?

Thanks for the review!

Re-rolled, updated service definitions and hopefully fixed the installer, also added the ContainerAware part and fixed the cache.bin unset. install backend is gone, so we don't have to worry about that anymore :)

To-Do:
- Consider to rename the services to cache.factory(.*)
- Change to use settings in the factory, possibly injected. Will need to change that a bit I think, we'll see.

Re-roll after the lock patch went in. Could not have worked on two other issues that were overlapping that much ;)

#168: cache-dic-1764474-168.patch queued for re-testing.

This patch has been written from ground up. I have seen Drupal install and ClearTest pass with it.

Oh. I tried minimal. Here's one that installs with standard :)

Oh DUTB.

Great work so far, here are just small comments.

+++ b/core/includes/cache.incundefined
@@ -57,9 +47,9 @@ function cache($bin = 'cache') {
 function cache_delete_tags(array $tags) {

@@ -76,7 +66,21 @@ function cache_delete_tags(array $tags) {
 function cache_invalidate_tags(array $tags) {
...
+function cache_get_bins() {

I'm wondering whether it would make sense to have these methods on the global Cache backend factory.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -23,19 +23,28 @@ class DatabaseBackend implements CacheBackendInterface {
    *
    * @param string $bin
    *   The cache bin for which the object is created.
    */
-  public function __construct($bin) {
+  public function __construct($connection, $bin) {

I guess we could typehint the connection object here?

+++ b/core/lib/Drupal/Core/Cache/ListCacheBins.phpundefined
@@ -0,0 +1,30 @@
+ * Adds services tagged 'access_check' to the access_manager service.
...
+   * Adds services tagged 'access_check' to the access_manager service.

Just to note: This contains code by another class :)

+++ b/core/lib/Drupal/Core/Cache/ListCacheBins.phpundefined
@@ -0,0 +1,30 @@
+      $cache_bins[$id] = substr($id, 6);

Why not split up after the first dot instead? This could be easier to read.

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -37,17 +37,30 @@ class CoreBundle extends Bundle {
+    $container->addCompilerPass(new \Drupal\Core\Cache\ListCacheBins());

Does it make sense to use this class instead of using the full path here?

+++ b/core/modules/filter/lib/Drupal/filter/FilterBundle.phpundefined
@@ -0,0 +1,30 @@
+ * Filter dependency injection container.

Just in general: I prefer the variant by local module: "Registers locale module's services to the container."
because it's simply more correct.

Thanks for the review, these have been addressed. The helper methods does not belong to CacheFactory -- but I already wanted a helper class for bin registration so Cache was added. I hopefully addressed most (all?) test failures as well.

+++ b/core/includes/cache.incundefined
@@ -26,40 +28,10 @@
   // Temporary backwards compatibiltiy layer, allow old style prefixed cache
   // bin names to be passed as arguments.
   $bin = str_replace('cache_', '', $bin);

Do we have an issue to remove this backport compatibility layer?

+++ b/core/includes/common.incundefined
@@ -6364,8 +6365,18 @@ function drupal_flush_all_caches() {
+  /*
+  array_map(function (CacheBackendInterface $cache_backend) {
+    $cache_backend->deleteAll();
+  }, CacheHelper::getBins());
+  */

Even if it's commented code, shouldn't we better use array_walk, as we don't map a function, and apply the value but rather iterate over the values. If we use array_walk, we can also exclude the bins in that already.

+++ b/core/includes/install.core.incundefined
@@ -350,6 +350,11 @@ function install_begin_request(&$install_state) {
+    foreach (array('bootstrap', 'config', 'cache', 'form', 'menu', 'page', 'path') as $bin) {

Idea for a follow up: We could put these bins in a constant, so it's documented why we exactly have these kind of bins.

+++ b/core/includes/update.incundefined
@@ -97,7 +98,9 @@ function update_prepare_d8_bootstrap() {
+  new Settings($settings);

Could we actually test settings() with that?

+++ b/core/lib/Drupal/Core/Cache/Cache.phpundefined
@@ -0,0 +1,84 @@
+ * Contains Drupal\Core\Cache\Cache.

Nitpick: Missing "\", I will not comment further nitpicks.

+++ b/core/lib/Drupal/Core/Cache/Cache.phpundefined
@@ -0,0 +1,84 @@
+class Cache {

If we add a new file we should also document what this class allows us to do.

+++ b/core/lib/Drupal/Core/Cache/Cache.phpundefined
@@ -0,0 +1,84 @@
+  static function registerBin(ContainerBuilder $container, $bin) {
...
+  static function deleteTags(array $tags) {
...
+  static function invalidateTags(array $tags) {
...
+  static function getBins() {

I guess these functions should be marked as public?

+++ b/core/lib/Drupal/Core/Cache/Cache.phpundefined
@@ -0,0 +1,84 @@
+    array_map(function (CacheBackendInterface $cache_backend) use ($tags) {
+      $cache_backend->deleteTags($tags);
+    }, static::getBins());
...
+    array_map(function (CacheBackendInterface $cache_backend) use ($tags) {
+      $cache_backend->invalidateTags($tags);
+    }, static::getBins());

Seems to be a good candidate for array_walk?

+++ b/core/lib/Drupal/Core/Cache/Cache.phpundefined
@@ -0,0 +1,84 @@
+    $container = drupal_container();
...
+      $bins[$bin] = $container->get($service_id);

Should we better use Drupal::service() now, as drupal_container() is deprecated?

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/ClearTest.phpundefined
@@ -32,19 +34,26 @@ function setUp() {
+    $bins = Cache::getBins();
+    // @todo remove after http://drupal.org/node/512026
+    unset($bins['form']);
+    foreach ($bins as $bin => $service) {
+      if (!$service instanceof \Drupal\Core\Cache\CacheBackendInterface) {
+        $this->fail(format_string('@bin is not a valid cache backend', array('@in' => $bin)));
+      }
+    }
+    $this->assertTrue($bins, 'cache_get_bins() returned bins to flush.');

It's odd to have a test for that in there, isn't that test cout clearing the cache, not getting the bins?

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/DatabaseBackendUnitTest.phpundefined
@@ -29,7 +30,7 @@ public static function getInfo() {
+    return new DatabaseBackend(Database::getConnection(), $bin);

Don't we have the container available at that time? There is at least a $this->container there.

+++ b/core/modules/system/system.moduleundefined
@@ -3521,10 +3523,10 @@ function system_cron() {
+  array_map(function (CacheBackendInterface $cache_backend) {
+    $cache_backend->deleteExpired();
+  }, Cache::getBins());

Another array_walk

+++ b/core/modules/views/lib/Drupal/views/ViewsBundle.phpundefined
@@ -42,6 +37,8 @@ public function build(ContainerBuilder $container) {
+    Cache::registerBin($container, 'views_results');

Nice catch, that we forgot the views_results in there.

Related to "$bin = str_replace('cache_', '', $bin);", I think my earlier patches already removed that as I couldn't find any remaining usage of that.

I'm working on throwing out 50% of the current cache bins in #1194136: Re-organise core cache bins (In short, group cache bins by type/usage instead of module that uses it), so let's not spend too much on documenting/defining them as constants for now :)

+++ b/core/lib/Drupal/Core/Cache/Cache.php
@@ -0,0 +1,87 @@
+ */
+class Cache {
+
+  /**

This class is not needed. See below.

+++ b/core/lib/Drupal/Core/Cache/Cache.php
@@ -0,0 +1,87 @@
+  public static function registerBin(ContainerBuilder $container, $bin) {
+    $container
+      ->register("cache.$bin", 'Drupal\Core\Cache\CacheBackendInterface')
+      ->setFactoryService('cache_factory')
+      ->setFactoryMethod('get')
+      ->addArgument($bin)
+      ->addTag('cache.bin');

I don't see how this is needed. It's one call in a bundle method.

+++ b/core/lib/Drupal/Core/Cache/Cache.php
@@ -0,0 +1,87 @@
+  public static function deleteTags(array $tags) {
+    array_walk(static::getBins(), function (CacheBackendInterface $cache_backend) use ($tags) {
+      $cache_backend->deleteTags($tags);

This should be a method on the CacheFactory object itself.

+++ b/core/lib/Drupal/Core/Cache/Cache.php
@@ -0,0 +1,87 @@
+  public static function invalidateTags(array $tags) {
+    array_walk(static::getBins(), function (CacheBackendInterface $cache_backend) use ($tags) {
+      $cache_backend->invalidateTags($tags);
+    });

This should be a method on the CacheFactory object itself.

+++ b/core/lib/Drupal/Core/Cache/Cache.php
@@ -0,0 +1,87 @@
+  public static function getBins() {
+    $bins = array();
+    foreach (Drupal::parameter('cache_bins') as $service_id => $bin) {
+      $bins[$bin] = Drupal::service($service_id);
+    }
+    return $bins;

This should be a method on the CacheFactory object itself.

+++ b/core/lib/Drupal/Core/CoreBundle.php
@@ -37,17 +39,27 @@ class CoreBundle extends Bundle {
+    // Register cache services.
+    foreach (array('bootstrap', 'config', 'cache', 'form', 'menu', 'page', 'path') as $bin) {
+      Cache::registerBin($container, $bin);
+    }
+
+    $container
+      ->register('cache_factory', 'Drupal\Core\Cache\CacheFactory')
+      ->addArgument(new Reference('settings'))
+      ->addMethodCall('setContainer', array(new Reference('service_container')));
+
+    $container
+      ->register('cache.backend.database', 'Drupal\Core\Cache\DatabaseBackendFactory')
+      ->addArgument(new Reference('database'));
+    $container
+      ->register('cache.backend.memory', 'Drupal\Core\Cache\MemoryBackendFactory');
+    $container->addCompilerPass(new ListCacheBinsPass());

We've been moving larger blocks like this into utility methods within CoreBundle for readability. I think this qualifies.

+++ b/core/modules/jsonld/lib/Drupal/jsonld/Tests/JsonldTestSetupHelper.php
@@ -46,10 +46,13 @@ class JsonldTestSetupHelper {
-    $this->siteSchemaManager = new SiteSchemaManager(new DatabaseBackend('cache'));
+    $this->siteSchemaManager = new SiteSchemaManager($container->get('cache.cache'));

Wait, why are we using the container in a test?

> I don't see how this is needed. It's one call in a bundle method.

We are not going to copypaste 5 lines of code 7 times. Not to mention it's not quite copypaste, it has an argument. Do we really need to argue on why functions are a good idea...?

> This should be a method on the CacheFactory object itself.

That's a followup. It involves refactoring _menu_clear_page_cache to use the new terminate event and so on. The issue is about the cache interface and the backends using the DIC, not converting every single helper to full injection. It's already 50kb and complicated.

> We've been moving larger blocks like this into utility methods within CoreBundle for readability. I think this qualifies.

Sure. I added comments too.

Unfollowing. Have fun!

Removed Drupal::parameter. The name is way too generic for what it does, and doesn't really provide a useful abstraction in this patch.

Changed the array_walk's to foreach. The type hinted callback was less readable and didn't buy us anything.

Also moved registerBins to CacheFactory. It's unfortunate that registering bins is already tied to the the container, but let's not make Cache dependent on it.

Thanks for continuing this issue!

+++ b/core/lib/Drupal/Core/Cache/CacheFactory.phpundefined
@@ -57,2 +58,18 @@
+   *   The container

We could fix this small comment in a follow up.

WD php: Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "cache.cache_block" does not exist. in        [error]
Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 799 of
/Users/alex/dev/sites/drupal8alt.dev/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).
Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "cache.cache_block" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 799 of /Users/alex/dev/sites/drupal8alt.dev/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).

Due to...

$this->discovery = new CacheDecorator($this->discovery, 'block_plugins:' . language(LANGUAGE_TYPE_INTERFACE)->langcode, 'cache_block', CacheBackendInterface::CACHE_PERMANENT, array('block'));

Should have fixed that comment at the same time...

Okay-dokey... the issue was this...

Strict warning: Only variables should be passed by reference in system_cron() (line 3535 of core/modules/system/system.module).                        [error]

+++ b/core/modules/system/system.moduleundefined
@@ -3532,7 +3532,8 @@
+  $bins = Cache::getBins();
+  array_walk($bins, function (CacheBackendInterface $cache_backend) {

In one of the other patches array_map/array_walk got changed to foreach, so using array_walk seems to be a small inconsistency, but I don't consider this as a problem which blocks the RTBC.

Okay had to make GenericCacheBackendUnitTestBase use DrupalUnitTestBase as Drupal\system\Tests\Cache\DatabaseBackendUnitTest was using the database and this shouldn't really happen in a UnitTestBase test.

Also incorporated feedback from #199 to make system_cron() consistent.

Hopefully green...

Looks awesome, some comment nitpicking but should be ready after that.

+++ b/core/lib/Drupal.phpundefined
@@ -122,6 +122,18 @@ public static function service($id) {
+  public static function parameter($name) {
+    return static::$container->getParameter($name);

I think Crell said at some point that this isn't really necessary. Also, I don't seem to find any usage of it, so not sure why it was introduced?

+++ b/core/lib/Drupal/Core/Cache/Cache.phpundefined
@@ -0,0 +1,71 @@
+ * Contains Drupal\Core\Cache\Cache.

In case this needs a re-roll, should be Contains \Drupal\...

+++ b/core/lib/Drupal/Core/Cache/Cache.phpundefined
@@ -0,0 +1,71 @@
+    $container = Drupal::getContainer();
+    foreach ($container->getParameter('cache_bins') as $service_id => $bin) {

Ah, I guess it was used here.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackendFactory.phpundefined
@@ -0,0 +1,29 @@
+ * Contains Drupal\Core\Cache\DatabaseBackendFactory.

+++ b/core/lib/Drupal/Core/Cache/ListCacheBinsPass.phpundefined
@@ -0,0 +1,30 @@
+ * Contains Drupal\Core\Cache\ListCacheBinsPass.

+++ b/core/lib/Drupal/Core/Cache/MemoryBackendFactory.phpundefined
@@ -0,0 +1,16 @@
+ * Contains Drupal\Core\Cache\MemoryBackendFactory.

Same here, should have a \ in front of the class name.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackendFactory.phpundefined
@@ -0,0 +1,29 @@
+class DatabaseBackendFactory {

I am wondering if we can use the same pattern here that we are nowing using for controllers and might be using for plugins as well. That is, a static create() method on the implementation that can get the stuff it needs from the container. Not as clean as this but also doesn't require this class.

Certainly something to discuss in a follow-up, we're just applying a pattern here that's already beeing used for queue and key/value.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackendFactory.phpundefined
@@ -0,0 +1,29 @@
+  function get($bin) {
+    return new DatabaseBackend($this->connection, $bin);

+++ b/core/lib/Drupal/Core/Cache/MemoryBackendFactory.phpundefined
@@ -0,0 +1,16 @@
+  function get($bin) {
+    return new MemoryBackend($bin);

Hm. A simple docblock with @param and @return wouldn't hurt. That was already in my previous patches I think...

+++ b/core/lib/Drupal/Core/Cache/ListCacheBinsPass.phpundefined
@@ -0,0 +1,30 @@
+/**
+ * Adds services tagged 'access_check' to the access_manager service.

Left-over docblock from copy & paste.

+++ b/core/modules/filter/lib/Drupal/filter/FilterBundle.phpundefined
@@ -0,0 +1,26 @@
+ * Definition of Drupal\filter\FilterBundle.

Contains \... :)

+++ b/core/modules/jsonld/lib/Drupal/jsonld/Tests/JsonldTestSetupHelper.phpundefined
@@ -46,10 +46,13 @@ class JsonldTestSetupHelper {
-    $this->siteSchemaManager = new SiteSchemaManager(new DatabaseBackend('cache'));
+    $this->siteSchemaManager = new SiteSchemaManager($container->get('cache.cache'));

It's arguable if this should use the container service or not. We often do that but it doesn't really matter, if we don't use that then we still need the database connection, so I think this is fine.

+++ b/core/modules/system/lib/Drupal/system/Tests/Plugin/CacheDecoratorTest.phpundefined
@@ -45,7 +47,8 @@ public function setUp() {
+    // @todo switch to injecting the MemoryBackend http://drupal.org/node/1903346
+    drupal_container()->set("cache.$this->cacheBin", new MemoryBackend($this->cacheBin));

Hah, I was going point to the same issue as being related but then noticed that you already did pick that one ;)

+++ b/core/modules/toolbar/lib/Drupal/toolbar/ToolbarBundle.phpundefined
@@ -0,0 +1,26 @@
+ * @file
+ * Definition of Drupal\toolbar\ToolbarBundle.

Another one of those.

+++ b/core/modules/toolbar/lib/Drupal/toolbar/ToolbarBundle.phpundefined
@@ -0,0 +1,26 @@
+   * Overrides Symfony\Component\HttpKernel\Bundle\Bundle::build().

Another \ ;)