Document what cache clearing from ContentEntityStorageBase::resetCache() actually clears

I know the current API is unfortunate, but it's how it always worked, also with entitycache in 7.x. This is IMHO an API change that we can not do in 8.x. All we can do is add a new method that only does the first.

There are valid cases where external code has to invalidate the persistent cache.

What about adding a second parameter which is just added to the actual implementation instead of the interface? With that practically everything can use it, but it doesn't break APIs.

Well, as I said, this is older than cache tags and a direct port from entity_cache in 7.x. It always worked this there. We don't use entity cache tags for the entity storage, we had an issue once but it didn't make sense.

Changing this IMHO is an API break for existing code that calls this function. There *are* cases that expect that it clears the persistent cache, comment related stuff for example, it's not just for tests. Tests actually don't need that, they only need the static cache clear part.

Changing this IMHO is an API break for existing code that calls this function. There *are* cases that expect that it clears the persistent cache, comment related stuff for example, it's not just for tests.

I think that's the kind of expectation it's OK to break in a minor release, given the documentation completely contradicts it and there are documented ways to do it properly.

I think that's the kind of expectation it's OK to break in a minor release,
given the documentation completely contradicts it and there are documented
ways to do it properly.

What documented ways? This is the only way to invalidate the entity storage cache. We don't use cache tags here. On purpose, since it is a 1:1 relationship between tags and cache ids, not dependencies.

Saving the entity resets the cache and should be the only time it needs to be reset.

Can also use the cache API directly since the cid is predictable if you're doing something unusual.

We have hook_entity_storage_load() and we tried to use it as much as possible (e.g. comment module). That allows you to attach stuff to entities when they're loaded and have it cached. Obviously, if that data changes, you have to invalidate the cache.

So the storage cache is *not* an internal, transparent thing. It's part of our API. And we need IMHO a way to invalidate it through the API.

Migration is also a major use case for clearing the static cache: #2558857: Migrations invalidate entity caches when trying to reclaim memory, should flush.

+1 for a solution to this. My use case is a long running CLI process where the static cache essentially causes a memory leak, but invaliding tags and clearing the persistent cache is both resource intensive and undesirable.

My work around is to use my own storage class extending SqlContentEntityStorage adding a resetStaticCache() method which only deals with the static cache. Perhaps this is something we can consider adding to ContentEntityStorageBase as a means to make this possible? Happy to post a patch if people feel that would be a reasonable solution?

#38: I like resetStaticCache() a lot.

How would backward compatibility affect adding this to an interface?

Ideally it would go into EntityStorageInterface. I Otherwise I presume we'd need to add a new interface that implementing code can check for before using..?

I'm thinking it would be best added to EntityStorageInterface and can simply call self::resetCache, or even make resetCache call the new method so the interface documentation can be updated to be more accurate.

tl;dr: I'm very happy to work on this, but I think the decision of which base class and interface to add this to is beyond my level of understanding of the BC policy.

I'll get a patch up today which simply adds resetStaticCache() to ContentEntityStorageBase with no addition to/of interfaces - that can the be expanded/adjusted once the wider decisions are made...

Methods

1) Deprecating resetCache() (for removal in 9) and having 2 new methods, one for static and one for everything/persistent (don't think you ever want to clear persistent without static, but maybe there is a use of that..?). Then we can update resetCache's documentation to match what it actually does. Everything in code is then switch to the appropriate new method and contrib can do the same.

2) Add a method for static cache and simply update the documentation for resetCache() and take the hit that people who were only expecting static caches to be reset will remain unaware unless they read the updated documentation.

3) Add a method for persistent cache and fix resetCache() with a change record and immediately update core to use the new method. This breaks BC, but only because it's fixing a mis-match between documentation and what it actually does.

What level to add it at

Once that's decided, we can decide if we do it for all entities or just content entities. I would lean towards all as non content entities may also have persistent even though core config doesn't.

How to deal with interfaces

1) We could add a new interface for just the new methods which the core base classes can implement and any calls to the methods need to check for this in case contrib has directly implemented the interface.

2) We could add the method to the interface breaking BC if we think that EntityStorageInterface is actually an internal and contrib should be extending the base classes (in which we will implement it) rather than the interfaces directly. Similar has been discussed elsewhere, but I don't know if it applies here.

3) Add an EntityStorageInterfaceFixed (with a better name) which extends EntityStorageInterface and becomes the new defacto interface with EntityStorageInterface kept as a backward compatibility layer to be deprecated/removed in 9.

4) Not add it to any interface and expect code to do a method exists/know that the entity they are dealing with implements it.

Just a note I'm working on #1596472: Replace hard coded static cache of entities with cache backends which provides a workaround for this (you can clear the cache.static backend directly), but also doesn't conflict with this afaik.

Afais, resetCache() is used as the way to clear the cache of an entity API, i.e., whatever cache is there. So I'd have said it's the docs that need to be fixed. Alternatively, I could see us adding an $excludePermanentCache = FALSE parameter to it?

@fago increasingly I think we should do #1596472: Replace hard coded static cache of entities with cache backends and mark this as duplicate. For now I'm going to postpone it.

@catch I think that #1596472: Replace hard coded static cache of entities with cache backends is probably the best solution to the original issue of not being able to clear the static, but at the moment there is an outstanding bug, whether it be documentation or API. I'm not sure this should be postponed, rather the plan of cache backends should influence how far we go in solving the current issue.

The change requested here we will handle as a duplicate of #1596472: Replace hard coded static cache of entities with cache backends. We should rescope this issue to simply correct documentation to explain what is cleared.

This not to be committed to core.
Just a workaround until #1596472: Replace hard coded static cache of entities with cache backends provides a solution.

Here's a first take at a docs update that clarifies that resetCache is any cache, not just static caches, and forks the docs for content entities to explain how to reset the static cache.

Applied patch #55 on 9.2.x and it works fine.

The new docs look good. Thanks!

(BTW @Abhijith S thanks for taking the time to check this, but the automated review process already shows that a patch is applying ok. And you don't need to upload a screenshot just to prove it; that wastes space on d.org's servers.)

Re-uploading 2635440.53-document-static-entity-cache.patch so it gets retested every 2 days against 9.2.x-dev.

Currently it's tested every 2 days against 8.8.x-dev.

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!