Problem/Motivation

15:52:45 Wim Leers: I've found a bug in KernelTestBase…
15:53:00 Wim Leers: It puts all caches in memory cache back-ends
15:53:08 Wim Leers: Unfortunately that keeps the object references
15:53:32 Wim Leers: So modifications to whatever you cached after it's been cached, are also applied to the cached data…

Proposed resolution

serialize() the cached data to prevent this insanity.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

msonnabaum’s picture

Status: Needs review » Reviewed & tested by the community

Looks totally sane to me.

Wim Leers’s picture

#1 will fail in the getMultiple() test coverage. This fixes that.

The last submitted patch, 1: memory_cache_serialize-2293163-tests-only-FAIL.patch, failed testing.

The last submitted patch, 1: memory_cache_serialize-2293163-1.patch, failed testing.

msonnabaum’s picture

Good catch. Still RTBC.

chx’s picture

Why are we cloning and serializing at the same time? Is that necessary really?

catch’s picture

Status: Reviewed & tested by the community » Needs review

Yeah I wondered the same thing, looks like leftover from previous iteration of the fix?

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

If I don't do this:

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.php
@@ -54,7 +54,7 @@ public function getMultiple(&$cids, $allow_invalid = FALSE) {
+      $item = $this->prepareItem(clone $item, $allow_invalid);

Then this:

+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.php
@@ -83,6 +83,8 @@ protected function prepareItem($cache, $allow_invalid) {
+    $cache->data = unserialize($cache->data);

will effectively cause $this->cache[$cid]->data to be unserialized. I.e. without the cloneing of the "cache" object in memory, we would be unserializing the data in the object in memory, thereby changing what's stored in the cache to not be serialized, but unserialized.

Go ahead and try to run the test coverage for the memory back-end without the clone statements. You'll see failures.

I know, rather confusing!

pwolanin’s picture

I had some small improvements I'll roll in.

catch’s picture

Status: Reviewed & tested by the community » Needs work

The information in #9 should be in an inline comment to explain the clone.

pwolanin’s picture

I'm updating it - I think the clone call should also be centralized in the prepare function.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
3.75 KB
1.53 KB
3.44 KB

Here's the updated patch, test-only patch, and incremental diff. I also added another test case condition that fails with current HEAD and show further the bugs this patch fixes. I also moved the clone call to a central location and added documentation.

@chx - I think it is necessary to both serialize and clone. clone is *not* a deep operation - it won't clone objects that are referenced as properties of another object. The serialize and unserialize makes a full-depth "clone" of every object.
Even if the serialize step wasn't strictly necessary, it also makes this a better mimic of the real cache back-end.

The last submitted patch, 13: 2293163-13-test-only.patch, failed testing.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

  • catch committed 6383bb2 on 8.x
    Issue #2293163 by Wim Leers, pwolanin: Fixed MemoryBackend's cache...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.