State cache collector uses a fixed cache and collection ID

This is implies that we want to support that use case, but I'm not convinced at all about that.

I'm often against making implementations final/abstract, but this is a case where it might make sense, see also my comment in 📌 Add support for cache collector for state in core Active .

This isn't CachedKeyValueStore, it's State and it has one specific use case.

What's wrong with extending State. Sorry, I', not getting it, maybe I also misunderstood your comment in the linked issue.

This isn't CachedKeyValueStore, it's State and it has one specific use case.

So, where is State without CachedKeyValueStore then? The latest change to State in 11.x and 10.3.x seems to have changed State in that direction. Sorry, when I'm missing the point here, I'm not deep into the architecture, I must admit.

My point is that the State *class* is not an API, it's the implementation of StateInterface and its implementation and behavior should be considered internal. It's not a base class that's designed to be reused. If you'd override and change the get/set implementations then it would now no longer work correctly.

CachedKeyValueStore doesn't exist, that's my point, it's not something we offer as a reusable base class.

If you want your own state-like API then I'd suggest to copy the State class, and not extend it. Possibly the old implementation in 10.2 if you're not interested in caching. I didn't investigate ECA enough whether or not caching is desired or useful in your case.

My point is that the State *class* is not an API, it's the implementation of StateInterface and its implementation and behavior should be considered internal. It's not a base class that's designed to be reused.

Why is that? It hasn't been marked internal. Is there any other way we should have known that this is not to be overriden?

> Why is that? It hasn't been marked internal. Is there any other way we should have known that this is not to be overriden?

Yes, it wasn't. One hint I guess is the fact that you had to override the constructor completely, which is always problematic as our common approaches to constructor BC handling then doesn't work, breaking your module in a case like this.

What I'm saying is that we *should* consider to make it @internal instead of trying to make it reusable.

> What I'm saying is that we *should* consider to make it @internal instead of trying to make it reusable.

Why? Is there any harm is leaving it resuable? I would certainly vote for it, and with the overrideable key proposed by @longwave, it would become even easier for our use case to construct the subclass object.

I don't know which way we should go here, but I think either we should be allowing multiple state-type things to exist by making it configurable (we could even inject the cache key via services.yml to truly make the core service reusable), or we should tag it as @internal and consider making it final. If we weren't expecting multiple implementations why do we have an interface? (though we could say that about a lot of core)

From what I can tell, there is at least one other contrib module that extends State: webprofiler. Another fairly popular module, I'd say.

> Why? Is there any harm is leaving it resuable?

The harm is breaking modules, like we did with ECA. It's a specific implementation, designed for a specific purpose (optimized for a few, frequently accessed keys with a shared single cache entry). We just made *considerable* changes to the implementation. I didn't look into EcaState and why it exists, but there's a chance that this caching is either not necessary or even harmful in your case (for example if there's a chance to have many different keys).

If it fits EcaState as well (a few, small-ish, frequently access items), then I'm not sure why it's even a separate service and not just using state to begin with, because then you benefit from the shared cache.

> From what I can tell, there is at least one other contrib module that extends State: webprofiler. Another fairly popular module, I'd say.

That's a very different use case. webprofiler is a developer tool that specifically wraps *the* state service to track how it's used and how often it's called. That makes sense, although after the implementation change, there's a very good chance that it will need considerable changes as well, because it should now likely track completely different things (cache misses/key lookups and maybe cache size. Because state get() calls are now basically free as long as you don't end up with a problematic cache size).

> If we weren't expecting multiple implementations why do we have an interface? (though we could say that about a lot of core)

We certainly have plenty of things where having an interface or not is questionable (e.g. still not sure if entity types should have interfaces or not ;)) but that's IMHO a completely different question. Different implementations of State are absolutely possible, the change we just did could have lived in a cached_state contrib module as an alternative implementation for example. Or there could be an implementation that uses a similar pattern to FastChained cache backends, with an apcu-backed local cache.

You could even go as far as saying having an interface is what would actually allow the specific State class/implementation to be internal/final, IMHO those things don't contradict each other at all.

PS: The irony isn't lost on me that I'm usually the one arguing against making implementations final/internal and allowing to extend/reuse code. Partially that may be that I'm starting to appreciate the benefits but also I also think that we shouldn't do that by default/everywhere like some people argue, just in specific cases where we agree on it making sense.

If we weren't expecting multiple implementations why do we have an interface? (though we could say that about a lot of core)

That's because some of the application of OOP patterns during the 7-8 cycle were dogmatic and didn't really stand up to scrutiny, and the kind of questions like 'why do we even need an interface for this' got drowned out, plus a lot of us were learning as we went along. I think we can probably just deprecate interfaces like this for some services altogether, or at least empty them out, the main problem is type hinting.

Phew, this seems to be a fundamental question far beyond the original scope of this issue. For my own use case, I'm OK either way, as long as we can make a fairly fast decision so that we can get prepared and make adjustments in contrib asap.

What seems to be difficult is to come up with an objective pattern, so that similar discussions won't be necessary for each current and future case.