Allow configuring cache key prefix

[rvanlaak]

We'd like to have a namespace with our keys, the GenerationalCacheStrategy and LifetimeCacheStrategy already use some sort of prefix (e.g. __GCS__ and __LCS__). I'm aware that we easily can add it to the key in Twig itself, but that seems wrong.

What about refactoring these "prefixes" to the interface and making it an optional constructor argument?

interface CacheStrategyInterface
{
    private $keyPrefix;

    // ...
}

class LifetimeCacheStrategy implements CacheStrategyInterface
{
    private $keyPrefix = '__LCS__';

    // ...

    public function __construct(CacheProviderInterface $cache, $keyPrefix = null)
    {
        // ...
        if (!is_null($prefix)) {
            $this->keyPrefix = $keyPrefix;
        }
        // ...
    }
}

Our use-case is quite easy, we want to include the Symfony environment in our cache key. Basically I'd like to eventually have the prefix as a configuration parameter in the TwigCacheBundle.

[asm89]

@rvanlaak We could make it an optional constructor argument for the the providers that have a hardcoded prefix (+ default value for BC), but the prefix is mostly meant to separate the key space for the providers implemented in this library.

It seems you want to change some part of the key to have some separation of keyspaces based on your environment? I think doing that in the configuration of your cache is a better place (e.g. namespaces in doctrine cache: http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/caching.html#namespaces). If that's not possible you could always implement the CacheProviderInterface and add something to the key globally yourself?

Closing this issue for now, feel free to re-open if you want to discuss further. :)

[rvanlaak]

Making it a constructor argument in my opinion is a quick win, as it allows us to easily override it via the service definition. I'd like to make you a PR if you'd like.

Next to that, I've now added my own strategy that adds the environment as prefix. As this is tied to Symfony, I think it would be best to make a PR for that PrefixCacheStrategy in the TwigCacheBundle?

[asm89]

Making it a constructor argument in my opinion is a quick win, as it allows us to easily override it via the service definition. I'd like to make you a PR if you'd like.

I'd rather keep it as is. We extract it to a constant and add a comment noting that it is for internal key namespacing in the library?

Next to that, I've now added my own strategy that adds the environment as prefix. As this is tied to Symfony, I think it would be best to make a PR for that PrefixCacheStrategy in the TwigCacheBundle?

Yes, that sounds like a good place. 👍

[rvanlaak]

Making it a constant doesn't allow us to override it. What's against making the prefix both a constant and set it via a constructor as I proposed in my first post? Right now the strategy is not properly extendable at all. Introducing the PrefixCacheStrategy in the bundle actually is a workaround for making it configurable via the DIC.

[asm89]

@rvanlaak What was your use case for needing to change it in the first place?

[rvanlaak]

Our use-case is quite easy, we want to include the Symfony environment in our cache key.

But next to that, I can think of another use case, grouping cache entries by domain logic (e.g. prefixing using a bundle name or component).

Think this can be implemented while maintaining BC.

[asm89]

Ah, Symfony environment being dev/prod/etc? That's the part that wasn't clear to me. If that's the case I strongly recommend doing that at the cache provider layer instead. See my example about how to do it with Doctrine cache.

But next to that, I can think of another use case, grouping cache entries by domain logic (e.g. prefixing using a bundle name or component).

That'd probably be its own strategy regardless?

[rvanlaak]

Changing the prefix would be changing the key generator's behavior and should stay related to the strategy I think? Especially as we use a PSR-6 cache chain with 3 pools modifying the provider will be hard. Exposing the prefix via the strategy's constructor would already allow us to do this way more easy than writing custom classes for it.

Right now I wrote my own class and extend the related strategy. Having less code (replace custom class with one line of code in the service definition constructor arguments) in my opinion is better.