Extract generic CacheCollector implementation and interface from CacheArray

Yes anything that doesn't need backwards compatibility with array syntax shouldn't be using ArrayAccess really, we should also be able to see a very small performance improvement here by skipping the magic methods.

On top of this there's the potential to have alternative implementations of the base class (like LRU instead of additive, although that's going to be hard to get right without too many writes but someone could try).

Tagging

this looks great! most of them are nitpicks:)

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,276 @@
+   * Implements CacheCollectorInterface::set().

full namespace needed

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,276 @@
+    // Invalidate the cache to make sure that other requests immediately see the
+    // deletion before this request is terminated.
+    $this->cache->invalidate($this->cid);
+    $this->cacheInvalidated = TRUE;
...
+    // Invalidate the cache to make sure that other requests immediately see the
+    // deletion before this request is terminated.
+    $this->cache->invalidate($this->cid);
+    $this->cacheInvalidated = TRUE;

maybe a protected method invalidate to avoid duplication?

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,276 @@
+          // cache entry, do not attempt to overwrite data that might have been

Probably the comma here should be a dot

+++ b/core/lib/Drupal/Core/Cache/CacheCollectorInterface.phpundefined
@@ -0,0 +1,79 @@
+ * The default implementation is CacheCollector.

we should use the full namespace to CacheCollector

+++ b/core/lib/Drupal/Core/Cache/CacheCollectorInterface.phpundefined
@@ -0,0 +1,79 @@
+Interface CacheCollectorInterface {

lowercase first on Interface

+++ b/core/modules/locale/lib/Drupal/locale/LocaleLookup.phpundefined
@@ -39,9 +39,34 @@ class LocaleLookup extends CacheArray implements DestructableInterface {
    * Constructs a LocaleCache object.

uh just noticed we have the wrong object name here. could we fix this since we are here?

this needs some unit tests, probably on AliasWhitelist, tagging

+   * A cid to pass to cache()->set() and cache()->get().
+   *
+   * @var string

Since the cache object is injected should the comment be updated as well?

not very thorough review, but they look great!

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,405 @@
+  function testResolveCacheMiss() {
...
+  function testSetAndGet() {
...
+  function testSetAndGetFalse() {
...
+  function testGetFromCache() {
...
+  function testDelete() {
...
+  function testUpdateCacheNoChanges() {
...
+  function testUpdateCache() {
...
+  function testUpdateCacheLockFail() {
...
+  function testUpdateCacheInvalidatedConflict() {
...
+  function testUpdateCacheMerge() {
...
+  function testUpdateCacheDelete() {
...
+  function testUpdateCacheReset() {
...
+  function testUpdateCacheClear() {

lets add some visibility here

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,405 @@
+    $this->constructCollector();

i see you do this on most tests..why not put it in setUp() ? and still call it when you want to override it later

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,405 @@
+    $key = $this->randomName();
+    $value = NULL;
+
+    $this->constructCollector();
+    $key = $this->randomName();
+    $value = $this->randomName();

you set $key and $value twice

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,380 @@
+ * Tests the cache CacheCollector.
+ *
+ * @group Cache

I really like to have @see \Drupal\Core\Cache\CacheCollector so you can switch between them really easy.

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,380 @@
+class CacheCollectorTest extends UnitTestCase {

This is an amazing test!!

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,380 @@
+   * @var \PHPUnit_Framework_MockObject_MockObject
...
+   * @var \PHPUnit_Framework_MockObject_MockObject

All the other tests actually document on the actual interface/class which is mocked, which gives you a better understanding what is going on.

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,380 @@
+    $this->collector->destruct();
...
+    $this->collector->destruct();

The setup method is called for each test methods, so I don't understand why this i needed.

i agree about the @var with berdir
i think i am not the right person to rtbc this, but imo its ready

Yeah I don't care that much either.

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,307 @@
+   * Implements \Drupal\Core\Cache\CacheCollectorInterface::set().

Oh i guess it should be one @inheritdoc?

lets get it in then

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,307 @@
+    return isset($this->storage[$key]) || array_key_exists($key, $this->storage);

After this->get() the array_key_exists() will always return TRUE, because items that don't exist get stored as NULL at least with the default implementation. Shouldn't this just be an isset()?

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,307 @@
+    // Lock cache writes to help avoid stampedes.
+    // To implement locking for cache misses, override __construct().
+    $lock_name = $this->cid . ':' . __CLASS__;
+    if (!$lock || $this->lock->acquire($lock_name)) {

This comment is just from CacheArray, but it's no longer accurate now that the cache item is loaded in lazyLoadCache.

+++ b/core/lib/Drupal/Core/Cache/CacheCollector.phpundefined
@@ -0,0 +1,307 @@
+          $this->cache->delete($this->cid);

Shouldn't this be deleteTags() if tags were passed in? At the moment it looks like if you have multiple cache items with tags, and a process calls delete on one of them, the rest won't be invalidated.

+++ b/core/lib/Drupal/Core/Path/AliasWhitelist.phpundefined
@@ -7,15 +7,19 @@
-class AliasWhitelist extends CacheArray implements DestructableInterface {

Yes! I was so pleased to get CacheArray into Drupal 8/7, but it's so good we can remove it from Drupal 8.

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,388 @@
+  /**
+   * Tests updating the cache when there is a
+   */

Incomplete comment: there is a?

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheCollectorTest.phpundefined
@@ -0,0 +1,388 @@
+
+    // Set up mock objects for the expected calls, first a lock acquire, then
+    // cache get to look for conflicting cache entries, which does find

Really nice tests except possibly for the multiple cache + tags case.

back to catch then!:)

Hmm. I guess either way is fine for has(), and it's only used in tests in the patch, so as long as we pick one it's up to implementations to figure that out anyway.

Committed/pushed to 8.x, will need a change notice.

Tagging.

#2083415: [META] Write up all outstanding change notices before release

Soooooooooo, I was just about to open an issue about removing CacheArray because I noticed it was deprecated and still used in ModuleInfo.php and SchemaCache.php. Also it was included in code comments in AliasWhitelist.php and TestBase.php. Also the CacheArray class still exists.

So is it better to create a follow up issue for the elimination of CacheArray from Drupal or is it better to piggyback onto here?

Also, I suppose I could try to write a change notice for this one once we clean up.

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release

The patch for this issue was committed on June 21, 2013, meaning that the change record has been missing for more than seven months.