In #3278493: Make it easier for theme builders to enable Twig debugging and disable render cache I discovered that state is broken when using a memory backend. That's because the service is initialized and the persisted service is not set.
#2368263: Remove the persist service tag tries to remove the persist tag completely. However, a good bug fix exists by replacing how KernelTestBase ensures the memory backend for state persists.
Fix how KernelTestBase persists the key value store
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 3358048-13.patch | 1.58 KB | dww |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !3920
Comments
Comment #3
mglamanThe KernelTestBase fix was first introduced in #2368263-35: Remove the persist service tag by amateescu
Comment #5
dwwNice, thanks! Per #3 crediting @amateescu.
The MR is small, simple, and well-documented. Curious to see if the bot has any problems with this. So far, looks RTBC to me.
Thanks again,
-Derek
Comment #6
dwwNothing to object to in the code. The issue title and summary are clear. This doesn't need a change record, since no one needs to know or care that this implementation detail is different. Ditto a release note. Bot is happy, so am I. 😉 Ship it!
Thanks!
-Derek
Comment #7
dwwI don't think we need explicit test coverage of this change. I asked about it in #bugsmash and @catch replied:
So I tried a combined patch over there. We went from 27 fails with the patch at comment #3278493-95: Make it easier for theme builders to enable Twig debugging and disable render cache to only 1 fail by also applying this patch at comment #98. That seems like sufficient improvement to know this is what we need. 😅🙏
Cheers,
-Derek
Comment #8
catchCommitted e129b1e and pushed to 10.1.x. Thanks!
Comment #10
catchComment #11
dwwThanks!
Is this back-port-able?
I’m having trouble imagining how the new approach could cause any disruption. It seems obviously more reliable and better for making sure the key value survives. Anyone relying on key value getting clobbered on a container build seems like they have buggy tests and should fix them. 😉
Comment #12
longwaveIf we get a green test run on 10.0.x/9.5.x I don't see an issue with backporting it, given this is tests-only and only likely to fix problems rather than cause them.
Will need new MRs or a patch uploading to test on the other branches.
Comment #13
dwwHere's exactly the MR diff in patch form, which I just checked applies cleanly to both 9.5.x and 10.0.x branches.
Comment #14
dwwBot is green on both branches. Cherry-pick away. 😅
Comment #17
longwaveCherry-picked and pushed to 10.0.x and 9.5.x, thanks!
Comment #18
andypostbackport to 9.5 needs revert and new patch without type for property https://www.drupal.org/pift-ci-job/2663367
Comment #19
andypostSorry for noise, valid issue is #3358328: Improve how KernelTestBase manages its persistent key value storage