MemoryCache backend does not validate the passed cids to invalidateMultiple().

Consequences:
1. It causes a PHP warning: Warning: Creating default object from empty value in Drupal\Core\Cache\MemoryBackend->invalidateMultiple().
2. It creates malformed cache item entry. If invalidateTags() gets called after invalidateMultiple() on a malformed cache entry it causes a PHP notice: Notice: Undefined property: stdClass::$tags in Drupal\Core\Cache\MemoryBackend->invalidateTags()

CommentFileSizeAuthor
#6 drupal-memorycachebackend-invalidatemultiple-ignore-invalid-cids-3017753-6.patch1.29 KBmxr576
#2 drupal-memorycachebackend-invalidatemultiple-ignore-invalid-cids-3017753-2.patch595 bytesmxr576
MemoryBackend_php.png298.32 KBmxr576
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mxr576 created an issue. See original summary.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
CreditAttribution: mxr576 at Pronovix commented
Status: Active » Needs review
FileSize
drupal-memorycachebackend-invalidatemultiple-ignore-invalid-cids-3017753-2.patch595 bytes
tamasd’s picture

tamasd CreditAttribution: tamasd at Pronovix commented
Status: Needs review » Reviewed & tested by the community

LGTM

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

We should add a test for this. Let's add

    // Invalidating a non-existent item should not cause an error.
    $this->assertFalse($backend->invalidate('test5'));
    $this->assertFalse($backend->invalidateMultiple(['test5']));

to \Drupal\KernelTests\Core\Cache\GenericCacheBackendUnitTestBase::testInvalidate()

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented 
+++ b/core/lib/Drupal/Core/Cache/MemoryBackend.php
@@ -156,7 +156,8 @@ public function invalidate($cid) {
-    foreach ($cids as $cid) {
+    $items = array_intersect_key($this->cache, array_flip($cids));
+    foreach ($items as $cid => $item) {
       $this->cache[$cid]->expire = $this->getRequestTime() - 1;
     }

This could be reduced to array_walk($cids, [$this, 'invalidate']); for a bit less complexity since the invalidate method does an isset() check.

There'll be less array options i.e. not both array_flip() and array_interset_key() plus less duplicate logic.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
CreditAttribution: mxr576 at Pronovix commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
drupal-memorycachebackend-invalidatemultiple-ignore-invalid-cids-3017753-6.patch1.29 KB

Thanks for the suggestions @alexpott! Good ones! I saw the implementations in this class are fairly simple and I wanted to keep it that way but the array_walk($cids, [$this, 'invalidate']); idea is the best solution.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer and at Calibrate commented
Status: Needs review » Reviewed & tested by the community

Awesome. This now has testcoverage and all of @alexpott's remarks have been fixed. Setting this to RTBC>

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 76bb511174 to 8.7.x and 2047ded936 to 8.6.x. Thanks!

  • alexpott committed 76bb511 on 8.7.x 
    Issue #3017753 by mxr576, alexpott: MemoryBackend should validate the...

  • alexpott committed 2047ded on 8.6.x 
    Issue #3017753 by mxr576, alexpott: MemoryBackend should validate the...

Status: Fixed » Closed (fixed)

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