Improve how KernelTestBase manages its persistent key value storage

Words:

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.

I 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.

This makes sense. Swapping the definition and being more direct. +1 from me.

My only stylistic nits would be that I prefer explicit null checks

if (!isset($this->keyValue)) {

That's it! Otherwise RTBC to me (pending test results)

Moving to RTBC per #4. Will let the committer decide to make that small change mentioned (didn't seem like a blocker).

Yay, 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 === NULL vs. !isset().

Fwiw 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.

Thanks @alexpott. ignore me in #4. I forgot it was a typed property where you need to use isset.

I'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...

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!

backport to 9.5 needs revert and new patch without type for property https://www.drupal.org/pift-ci-job/2663367 →

Valid patch, interdiff vs #3

Thank you!

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