State cache collector uses a fixed cache and collection ID

Created on 1 April 2024, 12 days ago
Updated 3 April 2024, 10 days ago

Problem/Motivation

State::__construct() calls the cache collector constructor with a fixed cache ID, and the key-value factory with the same fixed ID:

    parent::__construct('state', $cache, $lock);
    $this->keyValueStore = $key_value_factory->get('state');

We could make this easier for subclasses by providing a constant that can be overridden.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

📌 Task
Status

Active

Version

11.0 🔥

Component
Cache 

Last updated about 18 hours ago

Created by

🇬🇧United Kingdom longwave UK

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @longwave
  • 🇨🇭Switzerland Berdir Switzerland

    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.

  • 🇩🇪Germany jurgenhaas Gottmadingen

    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.

  • 🇨🇭Switzerland Berdir Switzerland

    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.

  • 🇩🇪Germany jurgenhaas Gottmadingen

    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?

  • 🇨🇭Switzerland Berdir Switzerland

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

  • 🇩🇪Germany jurgenhaas Gottmadingen

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

  • 🇬🇧United Kingdom longwave UK

    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)

  • 🇩🇪Germany jurgenhaas Gottmadingen

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

  • 🇨🇭Switzerland Berdir Switzerland

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

  • 🇬🇧United Kingdom catch

    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.

  • 🇩🇪Germany jurgenhaas Gottmadingen

    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.

Production build https://api.contrib.social 0.62.1