Clean up MemoryCache and MemoryBackend naming issues

X-Posting my comment from #3016690-3: Audit usages of cache.static here:

#2973286: Clean up MemoryCache and MemoryBackend naming issues proposes to rename the classes to make their usage constraints clearer, but I think the more onerous problem is actually that there isn't a general service for MemoryCache and that core.services.yml doesn't have a good comment for cache.static. It just says "A special cache bin that does not persist beyond the length of the request." but it really should be more like its class docblock: "Should be used for unit tests and specialist use-cases only, does not store cached items between requests."

Could we not name it MemoryTestingBackend, though? People might have a valid use case for using the current static cache backend.

One example is the new permission layer in the latest Group dev release: I store a bunch of value objects that represent one's permissions in the static cache. For me, the behavior that everyone gets different (throwaway) copies of said object rather than the same reference could arguably be a boon for security.

If one module decides to alter the returned object, then that would/could break all other implementations or even lead to vulnerabilities. The same can be said for entities, though: If people alter an entity without saving it, then all other code will get said alteration up until the point the static cache is cleared for some reason (e.g.: loadUnchanged()).

So I'm on the fence here. On the one hand, I love that no-one can mess with my precious permission layer value objects and open the floodgates. On the other hand, the entity layer is also vulnerable to this and it seems to be fine with the attitude of "if people do something that stupid, the code should break".

Given how you mention a huge performance by switching to the entity.memory_cache, I might consider doing the switch myself. But if I do, then the service name 'entity.memory_cache' makes no sense at all because I'm not storing entities in memory, but actually value objects.

Per #8, I think this has become actionable.

I'm at DrupalCon Amsterdam and I'll have a look at this.
I'm being mentored by mmbk.

Where are you sitting? I could help out a little with the approach. You can contact me through the event app if you want to. I'm currently sitting at the media desk.

+++ b/core/lib/Drupal/Core/Cache/MemoryBackendFactory.php
@@ -2,23 +2,10 @@
+ * @deprecated in Drupal 8.9.x. Will be removed before Drupal 9.0.0.

Will we have 8.9.0? Might need to state 8.8.x, not sure. This is something that could go into 8.8.1 or so just fine I believe...

Everything else looks great so far (with the recent services file updates).

I've changed it to Drupal 8.8.0 in the latest patch. I'll keep working on it.

First patch coming up :-)

Don't commit code on an empty stomach... Last one for today.

Can someone please review this?

Thank you for working through this issue @larskhansen and submitting a working patch. I found a few non-technical issues as I was reviewing.

I don't have time to do anything further, but I think the next review could apply the patch and manually search for the changed class.s

1. +++ b/core/lib/Drupal/Core/Cache/MemoryBackendFactory.php
   @@ -2,23 +2,10 @@
   +/**
   + * @deprecated in Drupal 8.8.x. Will be removed before Drupal 9.0.0.
   + * Use \Drupal\Core\Cache\MemoryTestingBackendFactory instead.
   

   For documentation blocks describing classes, I think there should still be a description of the class. This is for autogenerated api documentation. The description should be distinct from the new class.

   See API documentation and comment standards

2. +++ b/core/lib/Drupal/Core/Cache/MemoryTestingBackend.php
   @@ -118,7 +118,12 @@ public function set($cid, $data, $expire = Cache::PERMANENT, array $tags = []) {
   -      $this->set($cid, $item['data'], isset($item['expire']) ? $item['expire'] : CacheBackendInterface::CACHE_PERMANENT, isset($item['tags']) ? $item['tags'] : []);
   +      $this->set(
   +        $cid,
   +        $item['data'],
   +        isset($item['expire']) ? $item['expire'] : CacheBackendInterface::CACHE_PERMANENT,
   +        isset($item['tags']) ? $item['tags'] : []
   +      );
   

   I think that this is ok per the coding standards. The reason it's most likely the way it was is because there shouldn't be any white space before the closing paragraph.

   You might re-assign the inline conditionals to variables, and then keep the set method call on one line.

3. +++ b/core/lib/Drupal/Core/Cache/MemoryTestingBackendFactory.php
   @@ -0,0 +1,24 @@
   +class MemoryTestingBackendFactory implements CacheFactoryInterface {
   

   We should have a description for this class as well.

4. +++ b/core/modules/simpletest/src/Tests/SimpleTestTest.php
   @@ -92,13 +92,17 @@ function simpletest_test_stub_settings_function() {}
   -      $this->assertNotIdentical(\Drupal::getContainer(), $original_container, 'WebTestBase test creates a new container.');
   +      $this->assertNotIdentical(
   +        \Drupal::getContainer(),
   +        $original_container,
   +        'WebTestBase test creates a new container.'
   +      );
   
   @@ -216,7 +220,10 @@ public function stubTest() {
   -    $this->assertIdentical(get_class($this->container->get('cache.backend.database')), 'Drupal\Core\Cache\MemoryBackendFactory');
   +    $this->assertIdentical(
   +      get_class($this->container->get('cache.backend.database')),
   +      'Drupal\Core\Cache\MemoryTestingBackendFactory'
   +    );
   
   +++ b/core/tests/Drupal/KernelTests/Core/DrupalKernel/DrupalKernelSiteTest.php
   @@ -23,7 +23,10 @@ public function testServicesYml() {
   -    $this->assertIdentical(get_class($this->container->get('cache.backend.database')), 'Drupal\Core\Cache\DatabaseBackendFactory');
   +    $this->assertIdentical(
   +      get_class($this->container->get('cache.backend.database')),
   +      'Drupal\Core\Cache\DatabaseBackendFactory'
   +    );
   
   @@ -41,7 +44,10 @@ class: Drupal\Core\Cache\MemoryBackendFactory
   -    $this->assertIdentical(get_class($this->container->get('cache.backend.database')), 'Drupal\Core\Cache\MemoryBackendFactory');
   +    $this->assertIdentical(
   +      get_class($this->container->get('cache.backend.database')),
   +      'Drupal\Core\Cache\MemoryTestingBackendFactory'
   +    );
   

   I think it's okay to leave the originals as-is on one-line despite it going to over 80 lines.

   This would help make the scope of the change smaller and easier to review.

In the issue summary it says

\Drupal\Core\Cache\MemoryCache\MemoryCache and \Drupal\Core\Cache\MemoryBackend.

MemoryBackend is supposed to be used only for testing, whereas MemoryCache will be used for all entity storage.

+++ b/core/lib/Drupal/Core/Cache/MemoryCache/MemoryCache.php
@@ -12,7 +12,7 @@
-class MemoryCache extends MemoryBackend implements MemoryCacheInterface {
+class MemoryCache extends MemoryTestingBackend implements MemoryCacheInterface {

So... MemoryCache is extending MemoryBackend - so the issue summary isn't quite right - we are using MemoryBackend for non-testing stuff because MemoryCache extends it.

So either this issue needs to decouple them somehow (this is tricky). Or perhaps ... looking deeper...

+++ b/core/core.services.yml
@@ -201,12 +201,16 @@ services:
   cache.static:
+    alias: cache.memorytesting

This is used in user_permissions_hash_generator was is also not test code.

I'm really not sure that we should be making any change here. I'm not sure that the issue summary's assertions are correct.

+++ b/core/lib/Drupal/Core/Installer/InstallerServiceProvider.php
@@ -27,7 +27,7 @@ public function register(ContainerBuilder $container) {
-    $definition->setClass('Drupal\Core\Cache\MemoryBackendFactory');
+    $definition->setClass('Drupal\Core\Cache\MemoryTestingBackendFactory');

More real usages.

cache.memorytesting is more confusing then cache.static_per_request
Also maybe use something related to cacherouter on top of "memory" cache like render cache using a cache redirect...

@alexpott, @andypost: Those systems use the static cache, but really ought to use the "new" EntityMemory cache instead (also poorly named). It's faster because it does not (un)serialize every set/get and it otherwise 100% similar to the static cache, as it extends it.

Ideally, we have someting along the lines of:

1. abstract MemoryCacheBase: Does not serialize
2. MemoryCache (instead of EntityMemoryCache) extends MemoryCacheBase and is used as cache.memory
3. MemoryTestingCache (instead of static cache) extends MemoryCacheBase and adds back the (un)serializing so that it mimics a DB cache backend

Or even drop the base class. Either way, we need to make sure that the current static cache is only used for testing because it really tries to mimic a DB cache backend, which significantly slows it down. Making sure it has "Test" in its name somewhere would go a long way.

@kristiaanvandeneynde totally agree (moreover after re-listening your Amsterdam's session, which were great) Ref https://youtu.be/rAecKDWOPmY?t=1721

In child issues of #1577902: [META] Remove all usages of drupal_static() & drupal_static_reset() we are trying to remove drupal_static so probably this one also related to it
btw MemoryTestingCache still sounds like it supposed to be used for testing purposes (confusing to see in term sof production)

btw MemoryTestingCache still sounds like it supposed to be used for testing purposes (confusing to see in term sof production)

Which is fine, because that's the only place we should be using it. It's an in-memory cache that deliberately adds a performance hit by using serialization so it could functionally mimic a DB cache.

If you need a proper in-memory cache, use #30.2, akak the "entity memory cache" that we really ought to rename.

@kristiaanvandeneynde sure - my main point is that this issue needs to get to a place where code that it is labelling as "test" is not being used in non-test situations.

I agree with an abstract base class, which can then be used in two flavors.

Adding a tag based on the comments from @alexpott, @andypost, @catch, @FabianX and @kristiaanvandeneynde to reflect what needs to be done.

It’s Fabianx (lower case x) - let’s not rename our nick names, please.

On topic:

Agree - unserialize() / serialize() is like a deep clone - so it indeed should be tested first if the Installer can work without the clone.

Worst case it could serialize / unserialize explicitly with the new backend.

But needs separate issue and this one postponed on that.

Note there is #2488350: Switch to a memory cache backend during installation which was trying to switch to memory caches throughout the installer because writing to the DB is very wasteful. That issue hit some interesting breaks in certain PHP configs if I remember rightly. But I think this was with only PHP5.

I’m quite lost about this issue at the moment...

Is it postpone or is it waiting for @alexpott, @andypost, @catch, @FabianX and @kristiaanvandeneynde reflecting?

@larskhansen this is no longer a novice issue I'm afraid :)

Fix IS

Just want to post the summary of a discussion I had with @catch here.

MemoryBackend behaves like a proper cache bin in a sense that it always returns a new copy of the cached data. This opposed to MemoryCache that always returns the same object. For this reason we chose not to introduce a factory for MemoryCache nor to tag implementations of said cache with "cache.bin" as we don't want Cache::getBins() to return cache backends that don't really behave like a cache.

As a result of this, we should also not flag BackendChain implementations using MemoryCache as "cache.bin" but rather as "cache_tags_invalidator". This, as the name implies, ensures that caches still get invalidated based on cache tags, but does not allow the service to make it into the list of cache bins. We currently have 2 implementations in book and jsonapi that need updating as such.

Finally, we should also adjust RefreshVariablesTrait to reset instances of MemoryCache that were tagged as cache_tags_invalidator because the moment we rename their tag from "cache.bin" to "cache_tags_invalidator", some browser tests might start failing. As demonstrated in #3376846-26: Implement the new access policy API

An alternative that was also discussed is to introduce a new tag to flag these "I'm a static cache bin, but not really" type of services, pick those up in ListCacheBinsPass and then also call those from the CacheTagsInvalidator and RefreshVariablesTrait.

With all of the above said, I no longer have any objection to renaming MemoryBackend to something that implies it's to be used in testing only. For all intents and purposes people are encouraged to use MemoryCache, provided we don't let them flag it as a proper cache bin. It's faster in every way and supports cache tags, you just need to treat it as a static in a sense that it always returns the same copy of the cached data (and people might therefore alter it for everyone).

Created #3402850: Fix MemoryCache discovery and DX in light of my latest comment. Once that's cleaned up, it should be easier to move people to MemoryCache and then unblock this issue so we can rename MemoryBackend to something more suitable.

The other issue is fixed.

Yup, to summarize the other issue: We can now use MemoryCache as long as we declare it as cache.bin.memory. Its factory is cache.backend.memory.memory.

Example:

  cache.access_policy_memory:
    class: Drupal\Core\Cache\CacheBackendInterface
    tags:
      - { name: cache.bin.memory, default_backend: cache.backend.memory.memory }
    factory: ['@cache_factory', 'get']
    arguments: [access_policy_memory]

This means we should encourage anyone who wasn't using MemoryBackend because of how it works to switch to MemoryCache. We can then rename MemoryBackend to something that implies we only use it in testing to mimic a real cache.

Oh wait, #3223580: Use MemoryCache (not MemoryBackend) in the installer still exists. So back to [PP-1]?

#58: I don't see the other issue blocking this.

See #48

What about 3rd-party wrongly using MemoryBackend. I know #3503948: [PP-1] MembershipManager using wrong static cache but there could be others out there. I think we'll need to deprecate MemoryBackend not just rename it

MemoryBackend still serves a purpose for tests, though. So deprecating that will cause tests to fail on deprecation warnings.