[PropertyInfo] Cache support #16917

dunglas · 2015-12-09T07:18:01Z

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

Replace #16893 with several advantages:

Less complex patch
Work for all usages of PropertyInfo (even outside of the Serializer)
Avoid a circular reference between Serializer's CallMetadataFactory and PropertyInfo's SerializerExtractor
Allow @mihai-stancu's Recursive denormalize using PropertyInfo #16143 to work as-is

aitboudad · 2015-12-09T08:45:32Z

src/Symfony/Component/PropertyInfo/PropertyInfoExtractor.php

    {
        $this->listExtractors = $listExtractors;
        $this->typeExtractors = $typeExtractors;
        $this->descriptionExtractors = $descriptionExtractors;
        $this->accessExtractors = $accessExtractors;
+        $this->cache = $cache;


instead of passing the cache here I would prefer using composition instead which decorate the default PropertyInfoExtractor something like https://gist.github.com/aitboudad/12419c2db6d19320c5fa

same thing for #16838

I was thinking the same thing initially, but finally I'm not sure that it's a good idea:

Performance (and cache) is not an "optional" feature. It should be enable for all implementations in prod and IMO it's why it belongs to this class.

In the context of the Standard Edition it adds a lot of complexity and an overhead: it requires to use the service decorator feature to switch the class depending of the environment (the cache must be enabled in prod, but not in dev)

In the context of the component, it makes it harder to enable the cache (one more class to instantiate)

all other components using cache doesn't use composition for that (it is considered as a builtin feature)

When the PSR-6 will be ready, the argument can even be made mandatory (it's better to always use a cache) and a NullCache or something like that can be added for the dev environment (like with the Logger).

What do you think @symfony/deciders?

👍 Composition is much cleaner and should be used unless it is not possible, like with the property accessor cache.

dunglas · 2015-12-09T21:21:21Z

Switched to an implementation to a Decorator as requested. Should the decorator belong to the Doctrine Bridge? IMO no because when PSR-6 will be ready we will switch to this interface instead of Doctrine Cache but I'm open to suggestions. Same if you find a better name for the decorator.

dunglas · 2015-12-14T21:26:23Z

ping @symfony/deciders

xabbuh · 2015-12-15T16:51:47Z

src/Symfony/Component/PropertyInfo/PropertyInfoExtractorCacheDecorator.php

+    }
+
+    /**
+     * Retrieves the cached data if applicable or delegates to the decorated extractor..


double dots

xabbuh · 2015-12-15T16:52:26Z

Looks like you need to rebase here (the namespace changes have been done in another PR).

dunglas · 2015-12-16T07:36:40Z

Rebased and typo fixed.

nicolas-grekas · 2015-12-18T10:21:51Z

src/Symfony/Component/PropertyInfo/PropertyInfoExtractorCacheDecorator.php

+     */
+    private function extract($method, array $arguments)
+    {
+        $key = $method.serialize($arguments);


just wondering: $arguments contains $context. But what does $context contain? Is it a big or small array of scalars? Something potentially non-serializable there?

For now it is only used in Symfony by the serializer extractor (a small array of strings). But it's a good concern because the context can be used by any custom extractor and can contain anything.

Do you know a way to easily retrieve an unique identifier from an array or should we try something like recursively replacing instances of objects by the return of spl_object_hash()?

Maybe can we add a note in the doc of $context in interfaces that it should only contain scalar and serializable objects or it will create unexpected side effects?

It will avoid to add a costly (in term of performance) custom serialization process here.

dunglas · 2015-12-31T10:32:16Z

Committed a better way to handle context not serializable. /cc @nicolas-grekas

xabbuh · 2016-01-02T09:00:49Z

src/Symfony/Component/PropertyInfo/PropertyInfoExtractorCacheDecorator.php

+use Doctrine\Common\Cache\Cache;
+
+/**
+ * Adds a cache layer on top of an extractor.


This is not really meaningful.

dunglas · 2016-01-18T15:18:22Z

Status: needs work

Should be ported to PSR-6

dunglas · 2016-01-26T12:07:37Z

Status: Needs review

Moved to PSR-6.

fabpot · 2016-01-26T12:25:25Z

src/Symfony/Component/PropertyInfo/composer.json

@@ -34,6 +35,7 @@
        "phpdocumentor/reflection": "<1.0.7"
    },
    "suggest": {
+        "symfony/cache": "To cache results",


"psr/cache-implementation": "For using the metadata cache.",?

dunglas · 2016-01-26T17:47:22Z

@fabpot symfony/cache replaced by psr/cache-implementation in suggest.

fabpot · 2016-01-27T04:40:38Z

Thank you @dunglas.

@mihai-stancu

This PR was squashed before being merged into the 3.1-dev branch (closes #16917). Discussion ---------- [PropertyInfo] Cache support | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #16893 | License | MIT | Doc PR | todo Replace #16893 with several advantages: - Less complex patch - Work for all usages of PropertyInfo (even outside of the Serializer) - Avoid a circular reference between Serializer's CallMetadataFactory and PropertyInfo's SerializerExtractor - Allow @mihai-stancu's #16143 to work as-is Commits ------- 86c20df [PropertyInfo] Cache support

Tobion · 2016-01-27T10:23:08Z

src/Symfony/Component/PropertyInfo/Tests/PropertyInfoCacheExtractorTest.php

+ */
+class PropertyInfoCacheExtractorTest extends AbstractPropertyInfoExtractorTest
+{
+    public function setUp()


setUp is a protected method

Quick search revealed there's few more occurrences. I'll send a PR to fix them.

carsonbot added the Status: Needs Review label Dec 9, 2015

dunglas mentioned this pull request Dec 9, 2015

[DON'T MERGE] [Serializer] Add PropertyInfo support in metadata #16893

Closed

dunglas force-pushed the property_info_cache branch from 22c5ede to 3d15937 Compare December 9, 2015 07:20

dunglas changed the title ~~[PropertInfo] Cache support~~ [PropertyInfo] Cache support Dec 9, 2015

dunglas added the PropertyInfo label Dec 9, 2015

dunglas force-pushed the property_info_cache branch 2 times, most recently from 3d7efb2 to 677b45d Compare December 9, 2015 07:23

aitboudad reviewed Dec 9, 2015
View reviewed changes

dunglas mentioned this pull request Dec 9, 2015

[PropertyAccess] Add PSR-6 cache #16838

Closed

xabbuh reviewed Dec 15, 2015
View reviewed changes

dunglas force-pushed the property_info_cache branch from 3ed99f3 to 436f650 Compare December 16, 2015 07:31

nicolas-grekas reviewed Dec 18, 2015
View reviewed changes

dunglas mentioned this pull request Dec 30, 2015

Recursive denormalize using PropertyInfo #16143

Closed

xabbuh reviewed Jan 2, 2016
View reviewed changes

dunglas mentioned this pull request Jan 17, 2016

[Cache] Symfony PSR-6 implementation #17408

Merged

2 tasks

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jan 18, 2016

dunglas added 4 commits January 26, 2016 12:46

[PropertyInfo] Cache support

adb4493

[PropertyInfo] Cache Decorator

6cc4445

[PropertyInfo] Deal with not serializable context

681bc86

wip

4008130

dunglas force-pushed the property_info_cache branch from bd1022c to 335381b Compare January 26, 2016 12:06

[PropertyInfo] PSR-6 cache

9396a19

dunglas force-pushed the property_info_cache branch from 335381b to 9396a19 Compare January 26, 2016 12:07

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jan 26, 2016

fabpot reviewed Jan 26, 2016
View reviewed changes

Replace symfony/cache by psr/cache-implementation in suggest

43f3446

fabpot closed this Jan 27, 2016

dunglas deleted the property_info_cache branch January 27, 2016 07:12

Tobion reviewed Jan 27, 2016
View reviewed changes

fabpot mentioned this pull request May 13, 2016

Release v3.1.0-BETA1 #18776

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PropertyInfo] Cache support #16917

[PropertyInfo] Cache support #16917

dunglas commented Dec 9, 2015

aitboudad Dec 9, 2015

aitboudad Dec 9, 2015

sstok Dec 9, 2015

dunglas Dec 9, 2015

Tobion Dec 9, 2015

dunglas commented Dec 9, 2015

dunglas commented Dec 14, 2015

xabbuh Dec 15, 2015

xabbuh commented Dec 15, 2015

dunglas commented Dec 16, 2015

nicolas-grekas Dec 18, 2015

dunglas Dec 18, 2015

dunglas Dec 22, 2015

dunglas commented Dec 31, 2015

xabbuh Jan 2, 2016

dunglas commented Jan 18, 2016

dunglas commented Jan 26, 2016

fabpot Jan 26, 2016

dunglas commented Jan 26, 2016

fabpot commented Jan 27, 2016

Tobion Jan 27, 2016

jakzal Jan 27, 2016

[PropertyInfo] Cache support #16917

[PropertyInfo] Cache support #16917

Conversation

dunglas commented Dec 9, 2015

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

dunglas commented Dec 9, 2015

dunglas commented Dec 14, 2015

Choose a reason for hiding this comment

xabbuh commented Dec 15, 2015

dunglas commented Dec 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Dec 31, 2015

Choose a reason for hiding this comment

dunglas commented Jan 18, 2016

dunglas commented Jan 26, 2016

Choose a reason for hiding this comment

dunglas commented Jan 26, 2016

fabpot commented Jan 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment