Skip to content

[FrameworkBundle] missing integration of PSR-16 simple cache #28918

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Tobion opened this issue Oct 18, 2018 · 19 comments
Closed

[FrameworkBundle] missing integration of PSR-16 simple cache #28918

Tobion opened this issue Oct 18, 2018 · 19 comments

Comments

@Tobion
Copy link
Contributor

Tobion commented Oct 18, 2018

Description
Cache pools can easily be configured in the framework bundle with

framework:
    cache:
        pools:
            cache.pool1:
                adapter: cache.adapter.redis
            cache.pool2:
                adapter: cache.adapter.redis

But all the cache services created only implement PSR-6 Psr\Cache\CacheItemPoolInterface. We would like to use PSR-16 Psr\SimpleCache\CacheInterface instead. Currently the only solution seems to be to wrap all the pools in an adapter like

services:
    cache.simple.pool1:
        class: Symfony\Component\Cache\Simple\Psr6Cache
        arguments: ['@cache.pool1']

    cache.simple.pool2
        class: Symfony\Component\Cache\Simple\Psr6Cache
        arguments: ['@cache.pool2']

This is quite cumbersome as we have alot of pools. It would be nice if the framework bundle provides a way to use SimpleCache directly. Maybe something like the following would create PSR-16 caches instead:

framework:
    cache:
        pools:
            cache.pool1:
                adapter: cache.adapter.redis
                simple: true
            cache.pool2:
                adapter: cache.adapter.redis
                simple: true
@Tobion
Copy link
Contributor Author

Tobion commented Oct 18, 2018

Another idea would be create a the simple cache adapter automatically for each pool since it's already done for the predefined cache.app

<service id="cache.app.simple" class="Symfony\Component\Cache\Simple\Psr6Cache">
<argument type="service" id="cache.app" />
</service>

@Tobion Tobion added the Feature label Oct 18, 2018
@ogizanagi
Copy link
Contributor

ogizanagi commented Oct 18, 2018

I tend to agree with this second solution, but that would create the simple cache adapter service id based on a convention, which you tend to avoid (that's why cache pools names are exactly the service id to use).
Given registering the adapter isn't that much overhead for the developer, it may be rather a documentation issue rather than something to fix through configuration ?

@Tobion
Copy link
Contributor Author

Tobion commented Oct 18, 2018

simple cache adapter service id based on a convention, which you tend to avoid

Agree it's not optimal.

rather a documentation issue rather than something to fix through configuration ?

Don't agree as you can easily have a typo in all the adapters you need to configured like injecting the wrong pool into an adapter which is hard to spot since it does not fail.
Also I wonder why the cache component provides all the SimpleCache implementations like Symfony\Component\Cache\Simple\RedisCache if they are not beeing used by symfony at all. You could just use the Psr6Cache adapter for everything then.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 18, 2018

The fact that this doesn't exist is on purpose: the more choices we provide, the more confused people are. Here, PSR 16 doesn't provide anything over PSR 6. With the new CacheInterface, PSR 16 is not the simplest alternatives if one thinks that's where it fits.
With this reasoning, I think we shouldn't have a configuration helper for it.

@nicolas-grekas
Copy link
Member

I'm in favor of deprecating the cache.app.simple service and updating the doc to not tell about the PSR 16 interface so prominently. That's already on my 4.3 list :)

@weaverryan
Copy link
Member

And we can / should do that in 4.2, because that’s when the new CacheInterface will be available (which is both easy to use and powerful - best of both worlds)

@lyrixx
Copy link
Member

lyrixx commented Oct 19, 2018

For what it worth I like the PSR-6. It's not as simple as PSR-16 but actually PSR-16 has many drawbacks (the key is duplicated in the code, and it's impossible to know if the item was hit). I always use PSR-6 and I'm teaching only this way.

The new CacheInterface is nice too and I like it too :) But it's not a PSR :/ I'm not sure it's an issue.*

So for me : We should not talk about PSR-16 at all and only promote PSR-6 or the CacheInterface

@nicolas-grekas
Copy link
Member

I'd go one step further and deprecate all PSR 16 implementations except a PSR 6 to 16 bridge! (and remove psr/simple-cache from "require")

@Tobion
Copy link
Contributor Author

Tobion commented Oct 23, 2018

It seems like symfonys PSR-6 implementation does not allow to set cache items without fetching the data first? With PSR-16 this should be possible: With $cache->setMultiple() I can just set alot of stuff without having to read it first. But PSR-6 $cache->getItem()->set() will always read it first. CacheItem::isHit and CacheItem::get could be lazy to solve the problem? But it does not seem to be the case.

@nicolas-grekas
Copy link
Member

That's correct, that's consistent with wordings in PSR-6. But actually there's a way: using the NullAdapter as a factory for items. Implementing lazy loading at the item level would make everything more complex, better not to me, especially also because saving without fetching first is a very unusual pattern. Only cache warmers may need it.

@Tobion
Copy link
Contributor Author

Tobion commented Oct 26, 2018

I would not call it unusual pattern. We have data importers that basically refresh the cache. Those do not need to read the data first. And it makes a big difference if you do this for alot of items. So basically PSR-6 is unusable for this.

@nicolas-grekas
Copy link
Member

It is, using the NullAdapter as a factory.

@Tobion
Copy link
Contributor Author

Tobion commented Oct 26, 2018

I don't know how that should work. If I use the NullAdapter, then it also cannot save data. What do you mean?

@nicolas-grekas
Copy link
Member

You get the item from the NullAdapter, and you save it in the not-null one. Provided both are Symfony adapters, that works.

@Tobion
Copy link
Contributor Author

Tobion commented Oct 26, 2018

I see. But then I need to inject two caches everywhere to workaround a bad API. This is not practical and is asking for bugs.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 26, 2018

I understand, but I'm not sure that's a strong enough reason to keep PSR 16. The null adapter factory is a legit one, and if it allows reducing the maintenance burden, I'm willing to call it a good enough solution :)

@Tobion
Copy link
Contributor Author

Tobion commented Oct 26, 2018

I'm closing this one as it seems PSR-16 integration in core in not wanted which is fine to me. For anyone interested, we just use a psr6 decorator using

<?php

use Symfony\Component\Cache\Simple\Psr6Cache;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\Reference;

/**
 * Creates a PSR-16 simple cache adapter for each Symfony cache pool with a ".simple" suffix.
 */
class SimpleCacheAdapterPass implements CompilerPassInterface
{
    public function process(ContainerBuilder $container)
    {
        foreach ($container->findTaggedServiceIds('cache.pool') as $id => $tags) {
            $pool = $container->getDefinition($id);
            if ($pool->isAbstract()) {
                continue;
            }

            $definition = new Definition(Psr6Cache::class, [
                new Reference($id),
            ]);

            $container->setDefinition($id.'.simple', $definition);
        }
    }
}

@Tobion Tobion closed this as completed Oct 26, 2018
nicolas-grekas added a commit that referenced this issue Nov 20, 2018
…s (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[Cache] fix optimizing Psr6Cache for AdapterInterface pools

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -.

As described by @Tobion in https://github.com/symfony/symfony/pull/29236/files#r234324045:
> The problem I have experienced is that in dev mode the cache is decorated with a TraceableCache. This means it loses this optimization and introduces #28918 (comment) again

Commits
-------

b8100a9 [Cache] fix optimizing Psr6Cache for AdapterInterface pools
nicolas-grekas added a commit that referenced this issue Jan 26, 2019
…che instead (nicolas-grekas)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Cache] deprecate all PSR-16 adapters, provide Psr16Cache instead

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

As discussed in #28918, PSR-16 implementations are now mostly useless: either PSR-6 or contracts' CacheInterface is always a better fit.

Let's deprecate them all but keep only a `Psr16Cache` to turn any PSR-6 implementation to a PSR-16 one.

From the changelog:
  * removed `psr/simple-cache` dependency, run `composer require psr/simple-cache` if you need it
  * deprecated all PSR-16 adapters, use `Psr16Cache` or `Symfony\Contracts\Cache\CacheInterface` implementations instead
 * deprecated `SimpleCacheAdapter`, use `Psr16Adapter` instead
 * deprecated the "Psr\SimpleCache\CacheInterface" / "cache.app.simple" service, use "Symfony\Contracts\Cache\CacheInterface" / "cache.app" instead

Commits
-------

5006be6 [Cache] deprecate all PSR-16 adapters, provide Psr16Cache instead
@harsh16021992
Copy link

image
facing error after update silverstripe 5.2

@derrabus
Copy link
Member

derrabus commented Aug 7, 2024

Hello and thank you for reaching out.

This tracker is meant for tracking bugs and feature requests. If you are looking for support, you'll get faster responses by using one of our official support channels:

See https://symfony.com/support for more support options.

@symfony symfony locked and limited conversation to collaborators Aug 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants