[FrameworkBundle] integrate the Cache component #17868

xabbuh · 2016-02-20T23:34:50Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#17537
License	MIT
Doc PR	TODO

dosten · 2016-02-21T03:42:15Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/CacheAdapterPass.php

+            return $argument;
+        }
+
+        $name = substr((string) $argument, 14);


You can use $adapterId here.

dosten · 2016-02-21T04:02:12Z

With this approach how can I use a custom adapter? Maybe we can add a service option, so the user must use type or service but not these two together.

Using the type option means create an abstract definition using one of the built-in adapters
Using the service options means that we should check if the definition already exists, if the class implements the correct interface and if is abstract.

~~The problem is that with this implementation we replace the first argument with the prefix, but how we can be sure that a custom implementation follow this "rule"?~~

xabbuh · 2016-02-21T07:15:04Z

This solution is precisely about being able to easily configure the built-in adapters. If you want to use your own adapter, you can simply create a service for it and inject it where needed.

sstok · 2016-02-21T10:08:49Z

So when you use a service-id that starts with cache.adapter. it will use the configuration of that adapter (as decorated service). And as long as the service-id used for the parameter is configurable you can also use a custom adapter. Right?

I like the idea 👍

To bad this <argument id="%service-id%" type="service"> doesn't work, now you need create eg. a private alias with the configured value or replace the argument in the service.

dunglas · 2016-02-22T07:57:19Z

👍

javiereguiluz · 2016-02-22T08:07:57Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/CacheAdapterPass.php

+
+            $calls = $definition->getMethodCalls();
+
+            foreach ($calls as $c => $call) {


Single letter variables (like $c) are a bit unreadable. In other parts of Symfony (for example here) we do this:

foreach ($definition->getMethodCalls() as $name => $arguments) { // ... }

xabbuh · 2016-02-22T11:06:53Z

@sstok Currently, abstract service definitions are only supported for the built-in adapters (which makes sure that several services don't use the same cache namespace). To have the same feature for custom adapters, we would need a dedicated tag that you can use to tag your own adapters I think.

xabbuh · 2016-03-02T18:58:58Z

I updated the code to use a new cache.adapter tag to create cache adapters. This way it is easy to register custom cache adapters too.

ping @symfony/deciders

nicolas-grekas · 2016-03-04T08:55:18Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/CacheAdapterPass.php

+        $adapter = new DefinitionDecorator($adapters[$name]['definition_id']);
+
+        if (null !== $adapters[$name]['namespace_argument_index']) {
+            $adapter->replaceArgument($adapters[$name]['namespace_argument_index'], sha1($serviceId));


For the prefix, sha1 is 40 chars long. Since PSR-6 says the minimally supported length is 64 (utf8) chars, I think we should reduce this. What about using substr(str_replace('/', '-', base64_encode(md5($serviceId, true)), 0, 10)) instead? We only need a unique prefix, and 10 chars in base64 look unique enough to me. WDYT?

Also, since this namespace splits cache pools, should't we have one pools per $name?
Or put an other way, how should we allow sharing one single cache pool for several services?

I am fine with changing the strategy to generate the namespace here. However, how likely is it to get collisions with the solution you proposed?

When it comes to sharing the same pool in several services, I don't have a good idea how we can do that right now.

nicolas-grekas · 2016-03-04T10:12:36Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/CacheAdapterPass.php

+
+    private function createCacheAdapter(array $adapters, $serviceId, $argument)
+    {
+        if (!$argument instanceof Reference) {


I think this check should be a few lines above in the foreach of resolveArguments and this method should typehint $argument as Reference, it would save many function calls and looks more aligned to the responsibility of each method, wdyt?

… tags

xabbuh · 2016-03-11T17:38:07Z

@nicolas-grekas proposed some nice changes that are now part of this pull request.

Now you do not configure cache adapters anymore, but you define your cache pools like this:

framework:
    cache:
        pools:
            foo:
                type: apcu
                default_lifetime: 30
            bar:
                type: doctrine
                default_lifetime: 5
                cache_provider_service: app.doctrine_cache_provider
            baz:
                type: filesystem
                default_lifetime: 7
                directory: app/cache/psr
            foobar:
                type: psr6
                default_lifetime: 10
                cache_provider_service: app.cache_pool

The result of this configuration will be four services (cache.pool.foo, cache.pool.bar, cache.pool.baz, and cache.pool.foobar).

One thing that currently is not possible is to create a kind of pool template. This means that you need to configure more than one pool of the same type if you need to cache to the same destination but with different namespaces. The question is: Does it matter? Should we add something like a prototype option which would determine whether or not we needed to actually inject pools that share configuration but that should use different namespaces?

nicolas-grekas · 2016-03-12T09:27:18Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

+            ->children()
+                ->arrayNode('cache')
+                    ->info('Cache configuration')
+                    ->fixXmlConfig('pool')


fixXmlConfig() takes the singular form as the first argument

nicolas-grekas · 2016-03-12T09:34:47Z

The meaning of prototyping would be to auto create a namespaced pool on each use of the service. Auto-generating a namespace and auto-detecting each use to inject this namespace: this look too magic to be good to me. What do others think ?

On an other side, I think we should allow some kind of configuration "inheritance", so that one cache pool config could be reused as the basis for an other one. The use case I see is pre-configuration for bundles: the FrameworkBundle could provide a default template cache pool of type "filesystem", which would be reused by bundles/specialized cache pools. Then the user could easily change the type of all of them just by reconfiguring the default template to e.g. apcu. Or on a case by case basis for fine grained cache pool config. I don't know yet how to do that exactly, but does the use case make sense ?

xabbuh · 2016-03-13T11:29:02Z

@nicolas-grekas The configuration template pretty much sounds like the abstract adapters we already define.

xabbuh · 2016-03-31T14:33:50Z

closing in favour of #18371

…h, nicolas-grekas) This PR was merged into the 3.1-dev branch. Discussion ---------- [FrameworkBundle] integrate the Cache component | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17537 | License | MIT | Doc PR | - Last commit is the diff with #17868. Commits ------- 4152634 [FrameworkBundle] Add default pool & system adapter 714b916 [FrameworkBundle] Add & use Psr6CacheClearer 4740c5c [FrameworkBundle] use abstract cache.pool decoration and aliases 92b1a20 [FrameworkBundle] Fix and add tests for cache pool wiring e44bfdc [FrameworkBundle] Add cache-pool tag and wiring 281eafa [FrameworkBundle] Integrate the Cache component bc51fde [Cache] Normalize constructor arguments order

xabbuh added FrameworkBundle Cache labels Feb 20, 2016

carsonbot added the Status: Needs Review label Feb 20, 2016

xabbuh mentioned this pull request Feb 20, 2016

[RFC] The new Cache component should be a centerpiece feature of Symfony #17537

Closed

dosten reviewed Feb 21, 2016
View reviewed changes

xabbuh force-pushed the issue-17537 branch from 57a521b to 1bc4306 Compare February 21, 2016 07:13

xabbuh force-pushed the issue-17537 branch from 1bc4306 to 49329c8 Compare February 21, 2016 07:33

javiereguiluz reviewed Feb 22, 2016
View reviewed changes

xabbuh force-pushed the issue-17537 branch 2 times, most recently from e1d67af to a9f217c Compare February 22, 2016 20:13

xabbuh force-pushed the issue-17537 branch from a9f217c to 6f7e113 Compare March 2, 2016 18:57

xabbuh changed the title ~~[WIP][FrameworkBundle] integrate the Cache component~~ [FrameworkBundle] integrate the Cache component Mar 2, 2016

xabbuh force-pushed the issue-17537 branch 2 times, most recently from 20528c3 to 82ae7a7 Compare March 3, 2016 08:35

nicolas-grekas reviewed Mar 4, 2016
View reviewed changes

javiereguiluz added the Feature label Mar 4, 2016

nicolas-grekas reviewed Mar 4, 2016
View reviewed changes

xabbuh force-pushed the issue-17537 branch from 82ae7a7 to 7bdb403 Compare March 4, 2016 11:28

dunglas mentioned this pull request Mar 6, 2016

[PropertyAccess] Allow custom methods on property accesses #18016

Closed

4 tasks

[FrameworkBundle] integrate the Cache component

e9f5e71

xabbuh force-pushed the issue-17537 branch from 7bdb403 to e9f5e71 Compare March 10, 2016 14:23

nicolas-grekas and others added 4 commits March 10, 2016 18:15

[FrameworkBundle] Add wiring for cache-pool and cache-adapter service…

0bfd3f5

… tags

[FrameworkBundle] Add PSR-6 cache pool proxying

6bf21a4

fix cache pool wiring

fba8f64

fix FrameworkExtension tests

aecb007

xabbuh force-pushed the issue-17537 branch from 2ea922c to aecb007 Compare March 11, 2016 11:51

add PSR-6 type tests

5e927b4

nicolas-grekas reviewed Mar 12, 2016
View reviewed changes

dunglas mentioned this pull request Mar 18, 2016

Add metadata caching api-platform/core#469

Merged

nicolas-grekas mentioned this pull request Mar 30, 2016

[FrameworkBundle] integrate the Cache component #18371

Merged

xabbuh closed this Mar 31, 2016

xabbuh deleted the issue-17537 branch September 29, 2016 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle] integrate the Cache component #17868

[FrameworkBundle] integrate the Cache component #17868

xabbuh commented Feb 20, 2016

dosten Feb 21, 2016

dosten commented Feb 21, 2016

xabbuh commented Feb 21, 2016

sstok commented Feb 21, 2016

dunglas commented Feb 22, 2016

javiereguiluz Feb 22, 2016

xabbuh commented Feb 22, 2016

xabbuh commented Mar 2, 2016

nicolas-grekas Mar 4, 2016

nicolas-grekas Mar 4, 2016

xabbuh Mar 4, 2016

xabbuh Mar 4, 2016

nicolas-grekas Mar 4, 2016

xabbuh Mar 4, 2016

xabbuh commented Mar 11, 2016

nicolas-grekas Mar 12, 2016

xabbuh Mar 13, 2016

nicolas-grekas commented Mar 12, 2016

xabbuh commented Mar 13, 2016

xabbuh commented Mar 31, 2016


		$calls = $definition->getMethodCalls();

		foreach ($calls as $c => $call) {

[FrameworkBundle] integrate the Cache component #17868

[FrameworkBundle] integrate the Cache component #17868

Conversation

xabbuh commented Feb 20, 2016

Choose a reason for hiding this comment

dosten commented Feb 21, 2016

xabbuh commented Feb 21, 2016

sstok commented Feb 21, 2016

dunglas commented Feb 22, 2016

Choose a reason for hiding this comment

xabbuh commented Feb 22, 2016

xabbuh commented Mar 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Mar 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Mar 12, 2016

xabbuh commented Mar 13, 2016

xabbuh commented Mar 31, 2016