Add a PHP array cache backend

Do we have a full and official test suite for any cache backend we could reuse for both database and array implementation? (and that could be used by contrib modules too!)

Ok, if it happens I have an free hour this weekend I'll try to do this. EDIT: I'm already working on it.

Here is a preliminary test case, testing get/set/delete/getMultiple other advanced features are to be done. It should pass (minor edge case are commented in the code, still need to be fixed).

The idea is to provide a full class for those tests, with a getTestBin() and createBackendInstance() which contrib module should implement to provide their own implementations. Everything happening inside is not revelant to the contrib implementor. Internally, a getBackend() method will statically cache the instance the createBackendInstance() created and use it all along. This static cache is wiped out in every setUp() and tearDown() (and flushed too) to ensure that persistent backends won't keep data and each test will have a clean instance and empty bin.

All the others tests are backend agnostic using only the interface, the only edge case is that I wanted a unit test case and not a web test case, so I have to install the system module schema for database backend in setUp() which is not a proper use, but it ensures a really fast run compared to web test case.

Moreover, this should definitely be backported to D7 once done (and the LRU might be too a good candidate, as the array implementation).

Any suggestions, ideas?

EDIT: Using this implementation as base, incomplete backends can also override test they cannot success by just overriding using empty methods.

I am separating the database default implementation and the generic test case by abstracting the class carrying the tests, it allows me to decouple the setUp() and tearDown() specifics into the database implementation. Non database related implementation won't carry the system module schema install this way. This makes the generic abstract class clean.

Here is it is.

Now next step, I'll include @catch's raw code, and add their own test. Next patch in 20 minutes!

Here it is, fixed some stuff in ArrayBackend, but it needs more clean ups. Right now I fixed only the non passing tests (without focusing on code readability or performance -- this would be the phase next to completing the tests).

Patch attached.

Added deleteMultiple(), deletePrefix() and isEmpty() tests. Fixed the ArrayBackend::deleteMultiple() non working implementation.

Side note: once all this is done, we can get rid off a few legacy test classes. Already spotted at least two that we could get rid off: GetMultipleTest and ClearTest.

EDIT: CacheTestBase and IsEmptyTest will be candidates for removal too.

Only the special backends such as InstallTest and NullBackendTest shall remain.

This also raise a new question: the non native tags emulation as implemented in the database cache backend could probably be abstracted to an abstract base class for code sharing with other backends (including ArrayBackend in a first pass) for easier tags handling.

I think that, after done that and working nice, the ArrayBackend should be modified and handle tags natively with a denormalized tag array instead, for being faster (avoids a lot of strings operations).

I think we should also drop expiration support in ArrayBackend implementation, and make it silently always be peristent. The cache will die as soon as the page hit dies anyway.

Then we'd have just to add an empty stub for the ArrayBackendUnitTestCase::testExpires() method and live with it.

I'm not sure that we should be adding generic cache backends in the same patch as implementing an ArrayBackend for a couple of reasons - it makes the patch unnecessary large and harder to review - and it's two different concerns a arraybackend and generic testing.

Generic testing actually allows to ensure ArrayBackend consistency while fixing it. If we separate both, we'll need to commit the one containing the tests first. EDIT: Plus, I don't think this patch is that big. The ArrayBackend class is 200 lines and the test class is 300 hundred lines, and those are the only files it brings. Plus, the tests are basically self validating by the fact that the pass green with the actual database backend.

The patch attached adds a test for arraybackend and no longer attempts to generalise cache backend testing.

Additionally the patch adds working tag invalidations with tests :)

The interdiff attached is a diff between the ArrayBackend class from #10 and this patch (i.e. it doesn't include the tests)

This is a bad idea, we should proceed with generic testing first, it ensures that, even in case of wrong implementation(s) all backends behave exactly the same and avoid test code duplication. Which means that if we find bugs thanks to the ArrayBackend, we can fix the test in one shot and see how the DatabaseBackend behave directly in the generic test results.

I think we should continue with the dual patch, it's an excellent use case for it: it makes a lot of sense not only just testing the array backend, but also ensuring both backends works exactly the same. It's far more secure and robust, and we will gain a lot of time in the end.

EDIT: I think in this patch, we can assume the ArrayBackend is a mock up cache for testing purposes, and continue this way, and once the patch fully tests and passes with both database and array backend we can then declare the array backend safe to use for static caches.

Actually, reading this

the dual patch, it's an excellent use case for it: it makes a lot of sense not only just testing the array backend, but also ensuring both backends works exactly the same.

I agree - makes sense.

#16: 1637478-16-array_backend.patch queued for re-testing.

Patch attached implements a full generic test suite for cache backends. It provides testing for:

• Setting and getting
• Variable type preservation
• Deletion
• GetMulitple
• isEmpty
• Clearing using wildcards (Flush and DeletePrefix)
• Clearing using an array (DeleteMultiple)
• Clearing using tags (invalidateTags)

Also because it now uses UnitTestBase instead of WebTestBase it's three times as quick than the previous cache tests even though it's testing an additional backend.

I still have concerns about the size of this patch. I haven't provided an interdiff as the arrayBackend class is precisely the same as #13 and the tests are a rework of #10, #16 and the existing tests.

A big advantage of this patch is that it makes it very simple to thoroughly test different cache backends. For example:

use CustomCacheBackend;

class CustomCacheBackendUnitTestCase extends GenericCacheBackendUnitTestBase {

  public static function getInfo() {
    return array(
      'name' => 'Custom cache backend',
      'description' => 'Unit test of the custom cache backend using the generic cache unit test case.',
      'group' => 'Cache',
    );
  }

  protected function createCacheBackend($bin) {
    return new CustomCacheBackend($bin);
  }

  public function setUpCacheBackend() {

  }

  public function tearDownCacheBackend() {

  }
}

Removed some redundant tests.
Improved tests.

Please remove

+  public function setUpCacheBackend() {
+    drupal_install_schema('system');
+  }
+
+  public function tearDownCacheBackend() {
+    drupal_uninstall_schema('system');
+  }

From the ArrayBackendUnitTestCase class.

I don't like abstracting all the checkVariable() and checkCacheExists(), my tests was a bit verbose, but each test was encapsuled and readable in one block, which I think is easier for debugging.

All the test int, double, string etc could be done in a single testCachedValueTyping() method, it would avoid the test to setUp() and tearDown() for one value only. Plus it would test the typing in the context were multiple values are set in the bin (it could detect leakage).

Small coding convention I don't like to do this kind of review but this one makes my eyes blled: protected $test_bin; should be protected $testBin;. Same for $default_value, should be $defaultValue. the original tests didn't respect the coding standard either.

You refactored the testGetMultiple() method: you replaced all my tests by a series of $this->assertTrue(isset($ret['beatle1']) && $ret['beatle1']->data == $test_data['beatle1'], each one of your line is actually two of mine: one assert with the key existence, and one other with the key value. Your way will produce less precise reports (I voluntarely separated key existence from value tests, because it's a method that is not trivial to implement, so the developers needs a specific error here), plus you loose precision with the value typing. I think we should revert with mine.

Aside of those notes thanks for continuing this issue, this looks way better now that you added the missing tests, it's a good step forward, thanks!

Why have you removed my code and copied the testClearWildcard() and testClearArray() methods?

I had rewritten a full pragmatic test case in testDeletePrefix(), testDeleteMutliple() and testDelete() which are actually a bit more complete and test more use cases of deletion and try to detect more potential wildcard problems and leakage than the actual core tests.

They give more precise and detailed reports, and are more verbose. Please use what I done in #10 instead of erasing it with legacy core core. I didn't wrote this just for fun on scratch, I did it because I maintain cache backends in contrib projects, and what I wrote was built for precisely pinpointing the exact problems I could encounter when maintaining those: this is whole point of having a generic test case for cache backends.

Same notes for the getMultiple() methods, the reason why my version was extremely verbose and more strict is because it's way better when you do test driven development, and for things as cricitical as cache backends, it's not luxury, it's a must have. I wrote testGetMultiple() test from my experience developing various cache backends.

Actually I'm thinking that drupal_install_schema('system'); and drupal_uninstall_schema('system'); should go in the generic class as in order to test a backend properly we need the variables table (at the moment) since we need to variable_set('cache_clear_threshold', 2); to test that a cache backend obeys it. What is really interesting is that the only place in the codebase where cache_clear_threshold appears is in ./core/modules/system/lib/Drupal/system/Tests/Cache/ClearTest.php so this should probably be removed.

One reason I replaced your tests with the test already in core was to make it easier to get this patch in. We can improve the tests in follow up patches.

I will fix the code styling issues that make your eyes bleed :)

Ok thanks for the explaination. We should probably remove all base system hardcoded dependency from any backend (except the database one of course). The array backend itself doesn't need the variables, and the problem by adding this dependency in the generic test case is that it's not a simple unit test case anymore, it partly installs a Drupal. I'd prefer not to taint the generic test case and leave the database implementation as an example of how to add the system module if needed for others.

The issue is also about making those tests right, we shouldn't be shy about rewritting them, I don't see the point of doing follow-ups, we are adding one new big class, it's one class only, so review of the patch is easy and straightfoward (and moreover it's self testing). We should do it properly in one shot, follow-up will add a lot of administrative like work, a lot more that we could avoid by doing it directly. EDIT: And even worse, we don't write the complete test suite right now, we could find bugs later and be forced to fix the implementation in another issues: this would mean that other people may have already started working with the bugguy implementation.

An easy middle ground path is to let live the older tests aside of the new one, to ensure everything passes for database backend, and once this patch is ready for commit, do a proper follow up patch in the exact same issue to get rid of older tests.

The array backend is a really easy implementation, the real challenge in this issue is making the test right. Let's make them complete and ready to fly right now, and leave any follow up issue for real business purpose later.

Just as a side note: for performance reasons we could implement the tags as a multi-dimensional array instead of using the flatten/checksum method. This checksum/flatten based method will explode/implode a lot of strings all the time, and we don't have the restrictions the database backend has. What do you think?

It took me a while to understand you actually removed the tearDownCacheBackend() and setUpCacheBackend() empty stub from the generic implementation, which caused me some troubles once I removed them from the array implementation: never call a non existing method in a class, if the method doesn't exists, keep it abstract or empty or part of an interface it partially implements.

@alexpott Thanks a lot for what you've done once again, I took your fixed ArrayBackend implementation, started from your additional tests in the GenericCacheBackendUnitTestBase class, and modified this:

• Reverted all the changes of my original implementation of testDelete*() testing
• Added the testFlush() implementation
• Removed duplicated tests from core files, but kept those (some are real web test cases and probably ensure non regression due to older bugs)
• Reverted my testGetMultiple() tests too
• Added a testValueTypeIsKept() test (missing object and array, but kept those in one of the core tests)
• Renamed GenericCacheBackendUnitTestBase to GenericBackendUnitTestBase in order to be consistent with ArrayBackendUnitTestCase and DatabaseBackendUnitTestCase: do you think we should keep the "Cache" segment here? If you think so I'm not against make it back in, but I'd like to rename both backend unit tests to have also if we do.

I think we should succeed in making this patch rock solid, I know you don't like having too big patches, but clearly the cache unit testing needs this, and we shouldn't be picky regarding this refactor. A few people do really seem to be interested by this point, so I guess we won't bother much people by commiting 300 lines of unit testing (and all should pass by the way, so I really don't see why we shouldn't).

Sorry, wrong patch attached, this one had a typo error.

Both commented tests needs fixing I think, it's a long time since I posted this and I need to take some time for reviewing this once again. The first one is because we need to remove the ambiguous statement using parenthesis, I don't know why I didn't see it at first sight! Second one is a real bug I can't explain, but with a fresh new look it might be more obvious to find out why it did not pass.

Aside of that, agree with most of your comments, I will fix this as soon as I can.

Rerolled to lastest 8.x

From @catch #34:

1. Fixed serialisation references in PHP Doc.
2. Removed @todo and left comment as to why expire is irrelevant for array backend - it only persists for a single request.
3. Removed object saving test as saving objects in cache is not supported by DatabaseBackend
4. Fixed incomplete sentence in comment
5. Fixed tests in testGetMultiple() (this is the second time I've fixed this - please @pounard include this if you roll it again)

Additionally, this patch is a massive tidy up of the test messages in GenericBackendUnitTestBase and plenty of coding standard fixes in other files.

The attached interdiff between #32 and #36 is difficult to interpret due to the reroll.

We should keep the $expire parameter in, because we can use this implementation chained with persisting ones if #1651206: Added a cache chain implementation gets in, which means that the propagated parameters must kept for those to inherit from the right parameter. This actually was a mistake of mine of setting CACHE_PERMANENT, we need to keep the original given value instead.

@alexpott

• I don't see why you changed the tests in testGetMultiple() that much, just changing the comment place was enough.
• You added a lot of t() and format_string() calls, and sometime nothing: this is inconsistent. There is no need for that, hardcoded strings were fine.
• I don't like things such as foreach ($returned_cids as $cid) { and foreach ($missing_cids as $cid): tests code should not be factorized that much in this case because each test value is a different edge case (or almost all) and having one line per tests allow easier debugging using simpletests messages line numbers. By factorizing when an error happen at line X you cannot tell which value failed because multiple values are being processed in the foreach statement.

Aside of that, thanks for the patch, we are getting it closer from being sane.

@pounard I changed testGetMultiple() to read this way because I think it is more readable and easier to understand what it is actually being tested. If an error happens in testing I think that understanding what is being tested is more important than the line number.

I have not used t() in assertion messages - this is against Drupal coding standards :)

The patch attached fixes the expire issue raised in #37.

It also makes the use of format_string() consistent. It is only used in assertions made during a foreach... loop.

Oh sorry for the t() calls, I misread + and - you are right.

I'd still prefer the code without foreach() I think it makes the test clearer. Each value is a test case, each line tests a specific test case, we can leave a different message for each one of them and comment them individually, makes more sense to me. Plus you are changing the order of values by separating them into two arrays, this actually tests less cases than my own tests, keeping the order would ensure everything works when various operations are mixed altogether. Please consider that I actually wrote that without letting any details behind, and you just broke one. I did wrote this in the idea of re-using it with other backends, which could benefit of this specific detail.

"Empty backend is empty" was a voluntary typo as a reference to 9gagers. Code without fun comments is not code.

format_string() is a core function, is that wise to use as text formatter in tests? sprintf() would to the trick as well (and would make the code even smaller).

Updated patch, from #39, reverted the foreach() and did minor typo fixes, such as reverted one of your $value to $data variable name change.

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/GenericBackendUnitTestBase.php
@@ -9,7 +9,8 @@ namespace Drupal\system\Tests\Cache;
-use stdClass;
+
+use \stdClass;

The addtional line and \ are unnecessary

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/GenericBackendUnitTestBase.php
@@ -184,7 +185,6 @@ abstract class GenericBackendUnitTestBase extends UnitTestBase {
-      'test6' => array(1,2,3),

Why did you get rid of the array... we save arrays to caches so this should be tested.

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/GenericBackendUnitTestBase.php
@@ -228,78 +227,52 @@ abstract class GenericBackendUnitTestBase extends UnitTestBase {
+    $this->assert(isset($ret['test3']), "Existing key 3 is set");
+    $this->assert(isset($ret['test7']), "Existing key 7 is set");
+    $this->assertFalse(isset($ret['test21']), "Non existing key 21 is not set");
+    $this->assert(isset($ret['test6']), "Existing key 6 is set");
+    $this->assertFalse(isset($ret['test19']), "Non existing key 19 is not set");
+    $this->assert(isset($ret['test2']), "Existing key 2 is set");
+    // Test values
+    $this->assertIdentical($ret['test3']->data, 5, "Existing key 3 has the correct value");
+    $this->assertIdentical($ret['test7']->data, 17, "Existing key 7 has the correct value");
+    $this->assertIdentical($ret['test6']->data, 13, "Existing key 6 has the correct value");
+    $this->assertIdentical($ret['test2']->data, 3, "Existing key 2 has the correct value");
+    // Test $cids array
+    $this->assertFalse(in_array('test3', $cids), "Existing key 3 is not referenced");
+    $this->assertFalse(in_array('test7', $cids), "Existing key 7 is not referenced");
+    $this->assert(in_array('test21', $cids), "Non existing key 21 is referenced");
+    $this->assertFalse(in_array('test6', $cids), "Existing key 6 is not referenced");
+    $this->assert(in_array('test19', $cids), "Non existing key 19 is referenced");
+    $this->assertFalse(in_array('test2', $cids), "Existing key 2 is not referenced");

Please use the actual cache id in the message so, for example, "Non existing cache id test19 is referenced.". All messages should end with a fullstop. Tests should be sorted by type and then by key number.

So for example the above code should be:

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/GenericBackendUnitTestBase.php
@@ -228,78 +227,52 @@ abstract class GenericBackendUnitTestBase extends UnitTestBase {
+    $this->assert(isset($ret['test2']), "Key for existing cache id test2 is set.");
+    $this->assert(isset($ret['test3']), "Key for existing cache id test3 is set.");
+    $this->assert(isset($ret['test6']), "Key for existing cache id test6 is set.");
+    $this->assert(isset($ret['test7']), "Key for existing cache id test7 is set.");
+    $this->assertFalse(isset($ret['test19']), "Key for non existing cache id test19 is not set.");
+    $this->assertFalse(isset($ret['test21']), "Key for non existing cache id test21 is not set");
+    // Test values
+    $this->assertIdentical($ret['test2']->data, 3, "Key for existing cache id test2 has the correct value.");
+    $this->assertIdentical($ret['test3']->data, 5, "Key for existing cache id test3 has the correct value.");
+    $this->assertIdentical($ret['test6']->data, 13, "Key for existing cache id test6 has the correct value.");
+    $this->assertIdentical($ret['test7']->data, 17, "Key for existing cache id test7 has the correct value.");
+    // Test $cids array
+    $this->assertFalse(in_array('test2', $cids), "Existing cache id test2 is not in cid array.");
+    $this->assertFalse(in_array('test3', $cids), "Existing cache id test3 is not in cid array.");
+    $this->assertFalse(in_array('test6', $cids), "Existing cache id test6 is not in cid array.");
+    $this->assertFalse(in_array('test7', $cids), "Existing cache id test7 is not in cid array.");
+    $this->assert(in_array('test19', $cids), "Non existing cache id test19 is in cid array.");
+    $this->assert(in_array('test21', $cids), "Non existing cache id test19 is in cid array.");

Oups, the dropped array is a typo error, you are right and it should be restored.

Ok for messages, they need fixing.

The new line allow to separate imports from Drupal and PHP core/other framworks, it's a reflex I kept from coding with other frameworks, makes more obvious from where are imported various classes, I can remove the empty line if you wish, is that necessary per coding standard?

Removed the trailing \ of \stdClass
Added back the dropped array test in value/type test.
Changed all 'key 3' to 'cache id test3' as asked.

Hopefully this is a final tidy up of the getMultiple() tests.

• I've changed the order of the getMulitple() tests, test messages and added some commentary to make it super clear what's going on.
• I've also removed the use of the words "non existing" as the word "non" does not exist in English!
• I've made the tests reference the CacheBackendInterface method they are testing.

Further clean up to make test message language consistent and more commentary in the testClearTags() method so it's easier to understand what is being tested.

+++ b/core/modules/system/lib/Drupal/system/Tests/Cache/GenericBackendUnitTestBase.phpundefined
@@ -0,0 +1,457 @@
+    // Test a second time after deleting and setting new keys which ensures no
+    // static cache stall.

I think this comment might need further explanation as I'm not sure what is meant by "static cache stall". By this do we mean "ensures that statics in backends do not cause unexpected results"?

By reading the interdiff, all those changes sounds good to me. It's ready to fly and RTBC IMHO.

@beejeebus While I don't think they are necessary I'm OK with those changes, feel free to provide the patch and interdiff!

I'm on it...

Also renaming ArrayBackend to MemoryBackend - after conversations with @catch and @sun

I think it hides the implementation real nature, and that sort of sucks IMO, but if that is per convention and in order to normalize the names within the whole framework, I'm OK with it, but if not, I just don't see the purpose. Change it if you like I will be happy with it, in the end I just want it to be commited some day so that I can continue with the ChainBackend implementation and hopefuly then continue with cache chaining for entity controllers.

Implemented #52 and #50

@alexpott Thanks, let's make this happen :)

Cross posted, I think.

Oups yes, sorry, RTBC too.

One last coding standard violation: protected $invalidated_tags = array(); must be camelcased protected $invalidatedTags = array();, I will do it when I get back home if nobody did it.

Made change for #60- setting back to RTBC

RTBC for me too if test passes. Thanks.

Realised that we've named the test classes wrong. Keeping rtbc since this is just a rename.

Unfortunately the interdiff looks very ugly :(

That's only in tests I see, the interdiff doesn't look ugly, it's small enough to be readable. Still RTBC for me (if tests still pass).

For anyone working on cache, please now switch on #1596472: Replace hard coded static cache of entities with cache backends if you want to keep working on this. Once the CacheChain implementation done, numerous new issues will follow in order to homogenize and simplify cache usage in various parts of core.

Win! \o/

There were 2 minor issues that I ran into while working on the entity cache implementation.

@see patch.

We need to find out why the generic backend test didn't uncover this.

Added a test that catches the exceptions. Interestingly I can't produce an outright failure as the cache tag invalidations still occur because the checksum calculation is works the same for the tags that have been invalidated and the tags on the cache that you are retrieving though cache->get()

#70: 1637478-70-generic-testing+array-backend.patch queued for re-testing.

Bumping this follow-up to critical as this is actually a bug in core.

No longer needs tests as I added them :)

Tests looking good and seem to cover all the previously missing parts. This is currently blocking the entity cache issue.