Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#13 | 2293163-13.patch | 3.44 KB | pwolanin |
#13 | 2293163-13-test-only.patch | 1.53 KB | pwolanin |
#13 | increment.txt | 3.75 KB | pwolanin |
#3 | memory_cache_serialize-2293163-3.patch | 2.7 KB | Wim Leers |
Comments
Comment #1
Wim LeersComment #2
msonnabaum CreditAttribution: msonnabaum commentedLooks totally sane to me.
Comment #3
Wim Leers#1 will fail in the
getMultiple()
test coverage. This fixes that.Comment #6
msonnabaum CreditAttribution: msonnabaum commentedGood catch. Still RTBC.
Comment #7
chx CreditAttribution: chx commentedWhy are we cloning and serializing at the same time? Is that necessary really?
Comment #8
catchYeah I wondered the same thing, looks like leftover from previous iteration of the fix?
Comment #9
Wim LeersIf I don't do this:
Then this:
will effectively cause
$this->cache[$cid]->data
to be unserialized. I.e. without theclone
ing 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!
Comment #10
pwolanin CreditAttribution: pwolanin commentedI had some small improvements I'll roll in.
Comment #11
catchThe information in #9 should be in an inline comment to explain the clone.
Comment #12
pwolanin CreditAttribution: pwolanin commentedI'm updating it - I think the clone call should also be centralized in the prepare function.
Comment #13
pwolanin CreditAttribution: pwolanin commentedHere'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.
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedLooks good to me.
Comment #17
catchCommitted/pushed to 8.x, thanks!