Fix MemoryCache discovery and DX

See #2973286-54: [PP-1] Clean up MemoryCache and MemoryBackend naming issues → for more information if needed. Will post a POC soon.

PoC in MR now

One small comment on the MR but overall looks like a good improvement to me.

What about using 'memory' instead of 'static' in the various places this is wired up. There's potential confusion with APCu or even memcache, but less confusion with PHP statics.

Sure, I'm not married to any name here. Will look at the test fails and change the name.

Fixed two causes of test failures and renamed everything to "memory", will update IS. We do get the ugly cache.backend.memory.memory service name that way, though.

All green now :)

Did a quick look to the best of my ability with caching.

Believe we may have to do the backward compatibility dance now right?

Also may need a change record for the new parameter.

Not sure if there is concern for contrib that could be using Drupal\Core\Cache\MemoryCache\MemoryCache and not the interface?

I've addressed the feedback. Can add the typehint if you want, I'm fine either way but curious about what the policy is for adding methods on an existing class that doesn't have typehints on its other methods.

Thanks for following up on my thread.

So don't need a change record for the new parameter but imagine for the new MemoryCacheFactory? For other contrib modules can take advantage.

Added CR here: https://www.drupal.org/node/3409455 →

I'm interpreting catch's latest comment on the MR as if we should add the typehint here as it's new code. I could be mistaken in case he meant a completely new class with "new code", but can always easily revert. So will add the typehint.

Also noticed that the service book.memory_cache is actually poorly named as it gets the bin name "memory_cache" which is very ambiguous. We should consider renaming this to cache.book_memory when we move Book to contrib in D11. Will find the right issue for that and add that comment.

CR reads well.

In regards to the book rename, think you can open a ticket for the book module and put into postponed. Then when book is deprecated that ticket will be brought over.

I'm interpreting catch's latest comment on the MR as if we should add the typehint here as it's new code

Yes sorry that's what I meant - even though it's inconsistent within the class, it is much easier to add when it's new, than trying to add retrospectively, which we are only just figuring out how to do at all.

I found one more small issue in the MR (which I was planning to commit until I spotted it) - I think it is just removing that part of the comment so should be straightforward to resolve - nearly did it on commit but wasn't sure exactly the best place to snip it from.

Re #13 I added a comment on 📌 [11.x] [Meta] Tasks to remove Book Active

Re #14 I think I cut it off at the right spot if I understood your reservations regarding the example correctly. It's still valid to state that it can be overwritten in settings.php (which it can), but I dropped the copy-pasted example from the original method as that doesn't use the memory cache.

Yes I think that's good!

Committed/pushed to 11.x, thanks!

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

I'm a bit confused by this change.

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

  cache.jsonapi_resource_types:
    class: Drupal\Core\Cache\BackendChain
    calls:
      - [appendBackend, ['@cache.jsonapi_memory']]
      - [appendBackend, ['@cache.default']]
    tags: [{ name: cache.bin.memory }]

BackendChain passes cache tag invalidations through to the inner bins. That means any cache tag invalidations are now processed twice by cache.jsonapi_memory.

I can see that \Drupal\Core\Test\RefreshVariablesTrait::refreshVariables() calls a special reset() method that is not on the interface and only some memory implementations have that, but couldn't we have "just" implemented that on BackendChain and pass it through as well?

We can still tag that cache as private again, I suppose.

That tags in the memory cache get invalidated twice is a bit unfortunate, I agree. But wasn't that always the case to begin with for the regular cache? Not saying one wrong justifies the other, just trying to point out that BackendChain is easily misused because it can take any backend regardless of how it was declared. Most caches you'd feed into it are persistent/static pairs where the persistent is tagged as cache.bin, also leading to double invalidation.

Perhaps we should open a follow-up to discuss these findings.