Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.
Problem/Motivation
Per @alexpott in #3278493-112: Make it easier for theme builders to enable Twig debugging and disable render cache:
I wish we'd added a bigger comment in #3358048: Do not use persist tag for keyvalue.memory in KernelTestBase
When I reviewed #3358048 it all seemed pretty clear and self-documenting to me. But it's a very important change and a somewhat tricky part of Drupal (Kernel tests are always a bit weird). So probably we should be a bit more verbose.
Steps to reproduce
Proposed resolution
Flesh out the docs on both the $keyValue protected member itself, and the spot we manipulate it in ::register().
Remaining tasks
- Decide what more to say.
- Update the comments.
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 3358328-15-9.5.x.patch | 1.71 KB | andypost |
| #16 | interdiff.txt | 532 bytes | andypost |
| #3 | 3358328.patch | 1.98 KB | alexpott |
Comments
Comment #2
mglamanWords:
We cannot trust the container to persist properly, which we have found buggy. If the service doesn't persist, then data is lost. Instead of requiring all kernel tests to install the key_value schema, we use the storage backend and manually replace it to persist state.
Which if `persist` tag is removed, maybe we need better words. Or we use that wording and update the "remove persist tag" to include adjusting this comment.
Comment #3
alexpottI think we shoudl move things about a bit - and make the kernel test environment a bit more real. There's no reason to add a keyvalue.memory service and use aliases as far as I can - it's all unnecessary redirection. Also the comment should be less about persistence and more about why we're even bothering doing this.
Comment #4
mglamanThis makes sense. Swapping the definition and being more direct. +1 from me.
My only stylistic nits would be that I prefer explicit
nullchecksThat's it! Otherwise RTBC to me (pending test results)
Comment #5
smustgrave commentedMoving to RTBC per #4. Will let the committer decide to make that small change mentioned (didn't seem like a blocker).
Comment #6
dwwYay, this is cleaner still, and involves even less magic / indirection / obfuscation. +1 to #3.
Here's a more accurate title for the scope of this issue. Also, since it's no longer just adding more verbose docs, bumping priority a bit.
I'm also not NW'ing for #4, but would be happy to upload a new patch if committers prefer
=== NULLvs.!isset().Comment #7
alexpottFwiw isset() is more accurate that the NULL check and does not access the value so the difference becomes very important for read only values. The type of the variable does not actually allow for NULL values.
Comment #8
mglamanThanks @alexpott. ignore me in #4. I forgot it was a typed property where you need to use isset.
Comment #9
alexpottI've checked that tests like https://git.drupalcode.org/project/pathauto/-/blob/8.x-1.x/tests/src/Ker... continue to use a database. Not sure why that test makes the change it does but it still works - and I've checked that it is actually using the database (FWIW it passes when using the memory storage too). Here's the search I did to ensure that removing the keyvalue.memory service from KTB is safe - https://git.drupalcode.org/search?group_id=2&repository_ref=&scope=blobs...
Comment #13
catchCommitted/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!
Comment #14
andypostbackport to 9.5 needs revert and new patch without type for property https://www.drupal.org/pift-ci-job/2663367
Comment #16
andypostValid patch, interdiff vs #3
Comment #17
andypostThank you!