Added a cache chain implementation

+++ b/core/lib/Drupal/Core/Cache/CacheChain.phpundefined
@@ -0,0 +1,289 @@
+
+  /**
+   * Implements Drupal\Core\Cache\CacheBackendInterface::get().
+   *
+   * Because the constructor is into the CacheBackendInterface, we cannot
+   * implement our own that would allow cache backends to be set directly.
+   */
+  public function __construct($bin) {
+    $this->bin = $bin;

What about a CacheChainInterface interface that extends CacheBackenInterface with a custom implementation for the constructor that, instead of having a $bin as an argument would accept an array of CacheBackendInterface objects?

Our cache chain for entity caching (or, more generic: "property container / property item caching") should also be aware of PropertyContainers / PropertyItems and therefore accept a list of property items that should be used as tags. So for me it would make sense to have a constructor with 2 arguments: 1) Array of Cache backends and 2) Array of properties that should be used as tags.

+++ b/core/lib/Drupal/Core/Cache/CacheChain.phpundefined
@@ -0,0 +1,289 @@
+  public function get($cid) {
+    if ($this->doPropagateGet) {
+      return $this->propagatedGet($cid);
+    }
+
+    foreach ($this->backends as $backend) {
+      if (FALSE !== ($ret = $backend->get($cid))) {
+        return $ret;
+      }
+    } ¶
+
+    return FALSE;

There is a whitespace after the closing bracket for the foreach loop.

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/CacheChainUnitTest.phpundefined
@@ -0,0 +1,259 @@
+ */
+class CacheChainUnitTest extends UnitTestBase {  ¶
+
+  public static function getInfo() {

Whitespace after the opening brackets.

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/CacheChainUnitTest.phpundefined
@@ -0,0 +1,259 @@
+   *
+   * @var Drupal\Core\Cache\CacheBackendInterface
+   */ ¶

White space after the PHPDocBlock

Good job on this first implementation. It is more or less exactly how I imagined it to look like except for the missing Property-aware implementation with support for tagging and the stuff that I mentioned about the contsructor.

Oops... Didn't mean to change the status... Others should take a look too :)

Yes. Let's meet asap and discuss the architecture. I already had a small discussion about it with catch yesterday. So... When and where? Let's arrange a meeting on twitter.

We should definitely look at this - it's going to need to be its own issue and we might want to remove the bc layer before D8 is released (or possibly not, but it's worth discussing).

I opened #1748022: Make cache()->get() return a classed CacheItem object.

One

+++ b/core/lib/Drupal/Core/Cache/CacheChain.phpundefined
@@ -0,0 +1,289 @@
+
+  /**
+   * Does this instance must propagate get.
+   *
+   * @var bool
+   */
+  protected $doPropagateGet = TRUE;
+
+  /**
+   * Set the propagate mode on or off.
+   *
+   * Propagation is a specific mecanism that will, if the first backends skips
+   * the key on which we are doing a get operation, will call the set method
+   * upon them if a lower backend has a value.
+   *
+   * @param bool $toggle
+   *   TRUE will enable get propagation, FALSE will disable it.
+   *
+   * @return Drupal\Core\Cache\CacheChain
+   *   Self reference for chaining.
+   */
+  public function toggleGetPropagation($toggle) {
+    $this->doPropagateGet = $toggle;
+
+    return $this;
+  }

I think we need to discuss this. Propagation is not really optional in something that I would call a Cache Chain. After all, propagation is one of the key concepts of a cache chain and one of the two things that actually turn this cache backend into an actual chain.

Can you give us an example of a use case in which this would make sense? I am just worried that we are going to push code for something that is never going to happen. I've discussed this with @catch and basically we are both wondering what the use-case for this is. After all we believe that a one of the two essential concepts of a CacheChain is the propagation of stuff to higher level CacheBackends.

+++ b/core/lib/Drupal/Core/Cache/CacheChain.phpundefined
@@ -0,0 +1,289 @@
+
+  /**
+   * Implements Drupal\Core\Cache\CacheBackendInterface::invalidateTags().
+   */
+  public function invalidateTags(array $tags) {
+    foreach ($this->backends as $backend) {
+      $backend->invalidateTags();
+    }
+  }

We are also missing the forwarded $tags array here ($backend->invalidateTags($tags))

+++ b/core/lib/Drupal/Core/Cache/CacheChain.phpundefined
@@ -0,0 +1,289 @@
+        return $ret;
+      }
+    } ¶
+

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/CacheChainUnitTest.phpundefined
@@ -0,0 +1,274 @@
+ */
+class CacheChainUnitTest extends UnitTestBase {  ¶

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/CacheChainUnitTest.phpundefined
@@ -0,0 +1,274 @@
+   * @var Drupal\Core\Cache\CacheBackendInterface
+   */ ¶

+ trailing whitespaces

I have to disagree here, this is an optional feature and it holds a use case: we might use it as an horizontal chain of responsability cache, using multiple specialized backends that will each carry their own cached entries, case in which duplicating cache entries in multiple backends must not be done.

Yeah this really needs a use-case to justify it, and since we don't have one with core I'd much rather leave that to a contrib backend to implement if someone turns out to need it.

This ought to help with #1187726: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects), bumping priority to major.

+++ b/core/lib/Drupal/Core/Cache/CacheChain.phpundefined
@@ -0,0 +1,291 @@
+    } ¶

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/CacheChainUnitTest.phpundefined
@@ -0,0 +1,279 @@
+class CacheChainUnitTest extends UnitTestBase {  ¶

Whitespaces.

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/CacheChainUnitTest.phpundefined
@@ -0,0 +1,279 @@
+   */ ¶

Whitespace here too.

-----

I am still not entirely convinced that we should commit the propagation toggle at all (even if it's already covered in the tests and helps there in some way). I would like to see a usecase first. Sorry for insisting on this one so much :).

Also, the docblocks in general are not that nice yet and could use quite some improvement. I don't know, however, if we might want to postpone that (considering that catch marked this issue as major and it blocks that other issue over at #1187726: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects)).

Okay. That works for me too... I am not opposed to leaving it as it is for now and unblocking that other issue first. The tests are green now. If you want I can work on the documentation now... I have two hours of time right now.

Yes, I will keep that of course. I like that too :). Ok, I will give it a go now. Brb :)

Still working on this. I found some other issues that need to be addressed too... Sorry. Will post the patch asap but not later than tomorrow morning.

Oki, so this is what I did:

• I thought that ripping out the propagation stuff for now was actually not that hard and so I ripped it out.
• There was a performance issue with the propagation of cached values to lower level backends when doing getMultiple(). With the previous version of the patch it kept iterating over all previously fetched results for all backends whenever it reached a new backend causing multiple identical sets all the time. However, we just need to propagate values that we could not retrieve from the previous backends in the chain. That's what it does now.
• I refactored the tests based on the generic cache backend test class that we already have in core. I also duplicated the tests from that base test class in our CacheChain testing class to cover propagation in every single use-case. So we got all normal functional tests in the base test class and then again the same tests, but with focus on propagation, in our extended test class.
• Added support and testing for tags and tag invalidation.
• Removed / emptied CacheChain::__construct() (was redundant).
• The tests are now using the memory backend for performance reasons (database backend before)
• Emptied CacheChain::__construct() (was redundant).
• Codestyle and Documentation fixes. My english is not perfect so we might need some help from a native speaker here.

Whitespace fix.

Pure nitpick, but I like 0 < $index instead of $index > 0 (in both get and getMultiple().

Sure, we can do that... I don't really care which way we do it. I just thought that it makes more sense to iterate over the backends in reverse (last backend up to first backend).

Why did you remove the @todo about cache tags spec? Right now, we have no way to guarantee that propagating tags will work because we have specification of the $cache->tags property. We should leave this todo until cache interface has been improved.

The generic base test class for cache backends has a test for invalidating tags.

Please don't factorise tests, functions such as checkCacheExistsNowhere doesn't make sense IMO, each test is a unit test and I don't think it worth factorizing here.

That one is a really simple method comparable to checkCacheExists in the base class. It makes the tests much more readable and shorter.

I don't understand why you'd remove the setUp() method. It's the right and only place where the chain and three testing backends should be created in. setUp() method must create environment and objects that will be used during the test, it's an arbitrary general rule of testing. Creating stuff dynamically (lazy instanciation) does not have any sense here, because we will use the objects in every test method.

The generic test backend exposes that method. All other backend tests work the same way too (Memory backend, Database backend, etc. all use that method for setting up the cache backend object).

Please don't attach the first, second and third backends references to the chain object. I know that right now there is no property conflict, but it could. As the chain backend is an interface, we should not attach undocumented random properties to it.

We have to do that or something similar if we want to use the generic base test class. We are testing with multiple bins sometimes (e.g. in the invalidateTags test) and if we don't do that we end up with the same objects for the backends in the cache chain. We don't want that. So either we put them into properties on the cache chain object (I understand that that sucks somehow) or we put them in an Array keyed by the $bin or something like that.

We are reusing the $cache->tags property, which is undocumented. I don't care weither or not other tests are using it, as it is non documented and non specified, we need to keep this todo until it has been desambiguated.

By that definition we would not be able to use the $cache->data property either. It's undocumented as well ;). The tags property is just as documented/undocumented as any of the other properties on a cached item.

This is not a backend test, other backends have been made for being extendable for contrib, this one is meant to test a specific implementation.

We still need to provide tests for the basic functionality that is expected for a cache backend. That's what the generic base test class does. And that's why we should make use of it. Even though this is a really abstract implementation for a backend it still is a backend. And so we should test it as a backend too.

I'd expect this to be extended or re-implemented in contrib to provide verification of cache items in 'local' cache storage. i.e. if I chain an APC and Database backend together, then I might want to check a timestamp vs. the APC and database storage to determine whether the stuff in the APC storage is valid or not (or there are other ways to handle this, but still there are use cases for extending it).

@pounard the problem with 'local storage' is that even if you successfully clear stuff on one server, it doesn't clear on another. We absolutely don't need to fix that here, it's just a reason that this might get extended in contrib.

Let's see if the testbot likes this...

Note: I renamed the CacheChain class to ChainBackend so it is in-line with the existing backend namings... Namingconventions, yai! Consequently I also renamed the test classes of course.

The tags test is back in as well.

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/ChainBackendImplementationUnitTest.phpundefined
@@ -0,0 +1,320 @@
+ * Tests cache clearing methods.

I just noticed that this docblock is utterly wrong :). Apart from that I don't think that I have anything to add to this. For me it is ready to go now.

Ok, this is it I think...

Renamed once more to: BackendChain - Makes more sense per agreement of catch, pounard and me.

Considering #55 this is RTBC now :)

Probably makes sense to have catch look at this.

Overall this looks great. I'm fine with not handling the local caching issue here, that's probably better left for the APC issue.

I mainly found comment/style issues but there's quite a few of them, and some dead code:

+++ b/core/lib/Drupal/Core/Cache/BackendChain.phpundefined
@@ -0,0 +1,208 @@
+/**
+ * The cache chain backend allows you to combine multiple cache backends. It
+ * behaves as a chain of command for write operations (every modification will
+ * be propagated to every backend in the chain) while it's a chain of
+ * responsability for read operations (the first backend in the chain that holds
+ * the queried value "wins" and returns the result while subsequent cache
+ * backends are left alone). This can make it slower for write-, but a lot
+ * faster for read operations.
+ *
+ * A common use-case would be to chain a database cache backend (persistent)
+ * behind a memory cache backend (static). Now, when querying the BackendChain
+ * for a particular value it would first try to fetch that value from the memory
+ * cache backend up front. If the queried entry does not exist in the memory
+ * cache backend it would attempt to retrieve said value from the database cache
+ * backend and, if successful, propagate that value back to the memory cache
+ * cackend. In case of a subsequent request the same value would now be fetched
+ * from the memory cache backend without having to query the database again.
+ */

The summary should be one line I think. Not sure if that's true for classes but it probably makes sense here anyway.

There's a few typos 'responsability', 'cackend' (although I like that one) and some of the description here is very clunky. If I have time later I could try to take a look at it but it could probably use looking over by someone not familiar with the patch for clarity.

+++ b/core/lib/Drupal/Core/Cache/BackendChain.phpundefined
@@ -0,0 +1,208 @@
+    foreach ($this->backends as $index => $backend) {
+      if (FALSE !== ($ret = $backend->get($cid))) {
+        // We found a result, propagate it to all missed backends.
+        if (0 < $index) {
+          for ($i = ($index - 1); 0 <= $i; --$i) {
+            $this->backends[$i]->set($cid, $ret->data, $ret->expire, $ret->tags);
+          }

Please reverse the yoda conditions. If we want to start using this (I personally can't stand them though), then it needs a separate issue rather than being snuck in.

Also we don't abbreviate variables, so $return vs. $ret.

This is going to conflict a bit with the invalid vs. deleted patch but I think it'll be fine to adapt once that gets in.

+++ b/core/lib/Drupal/Core/Cache/BackendChain.phpundefined
@@ -0,0 +1,208 @@
+  /**
+   * Implements Drupal\Core\Cache\CacheBackendInterface::deletePrefix().
+   */
+  function deletePrefix($prefix) {
+    foreach ($this->backends as $backend) {
+      $backend->deletePrefix($prefix);
+    }

This has been removed by another patch already.

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/BackendChainImplementationUnitTest.phpundefined
@@ -0,0 +1,320 @@
+    // backend, the values are voluntarely different, this ensures in which
+    // backend we actually fetched the key when doing get calls. This is

Typo 'voluntarely'. Also we don't need comments like "this is important, do not change!", or at least it should be rephrased to "this is essential to avoid false positives".

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/BackendChainImplementationUnitTest.phpundefined
@@ -0,0 +1,320 @@
+    // Set a key present on three of them (for delete).

Should be "all backends", "of them" isn't very clear.

The summary should be one line I think. Not sure if that's true for classes but it probably makes sense here anyway.

Agree. One line summary + native speaker looking over it.

Also we don't abbreviate variables, so $return vs. $ret.

I think this was copy&pasted from the other backends. They use abbrevations in the exact same spot too ($ret). :*(

I will do a re-roll later and will also try to find a native speaker who is unfamiliar with the patch to improve the documentation.

Tried to address @catch's concerns. We will still need someone who didn't work on the patch to review our documentation (especially for the interface).

Good work. RTBC for me too

Trying to help with docs a bit:

/**
 * Defines a chained cache implementation for combining multiple cache backends.
 *
 * Can be used to combine two or more backends together to behave as if they were
 * a single backend.
 *
 * For example a slower, persistent storage engine could be combined
 * with a faster, volatile storage engine. When retrieving items from cache, they will
 * be fetched from the volatile backend first, only falling back to the persistent backend
 * if an item isn't available. An item not present in the volatile backend but found in the
 * persistent one will be propagated back up to ensure fast retrieval on the next request.
 * On cache sets and deletes, both backends will be invoked to ensure consistency.
 */

If that looks OK I can either fix on commit (or a new patch would work). Apart from that no further complaints.

That looks good! Let's go with that :).

Fixed up the docs a bit more with fubhy in irc. Committed/pushed to 8.x.

Follow-up issue: #2541432: Follow-up for #2231595: Document why ChainedFastBackend cannot use BackendChain.