Skip to content
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

[Serializer] Do not cache attributes if `attributes` in context #25185

Merged

Conversation

@sroze
Copy link
Member

commented Nov 28, 2017

Q A
Branch? 3.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #25108
License MIT
Doc PR ø

Caching attributes based on the class works only when these attributes are not overwritten. This disables the cache when they are.

To me, this extractAttributes method should actually be a AttributeResolver dependency that can be decorated using different caching strategies I'd say but... that's a much bigger refactoring that needs more reflection with @dunglas.

@sroze sroze force-pushed the sroze:bugfix-25108-do-not-cache-attributes-if-in-context branch from 98561d4 to 303280d Nov 28, 2017

@@ -126,7 +126,7 @@ protected function getAttributes($object, $format = null, array $context)
return $allowedAttributes;
}
if (isset($this->attributesCache[$class])) {
if (isset($this->attributesCache[$class]) && !isset($context['attributes'])) {

This comment has been minimized.

Copy link
@lyrixx

lyrixx Nov 28, 2017

Member

If $context['attributes'] is defined, we will fill the $this->attributesCache[$class] array with useless values.

IMHO, the correct patch is:

if (!isset($context['attributes']}
     return $this->extractAttributes($object, $format, $context);
}

before the cache layer

This comment has been minimized.

Copy link
@sroze

sroze Nov 28, 2017

Author Member

Good point, updated.

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Nov 28, 2017

@sroze sroze force-pushed the sroze:bugfix-25108-do-not-cache-attributes-if-in-context branch from 303280d to 6e87382 Nov 28, 2017

@lyrixx
lyrixx approved these changes Nov 28, 2017
@fabpot
fabpot approved these changes Nov 29, 2017
@fabpot

This comment has been minimized.

Copy link
Member

commented Nov 29, 2017

Thank you @sroze.

@fabpot fabpot merged commit 6e87382 into symfony:3.3 Nov 29, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
fabpot added a commit that referenced this pull request Nov 29, 2017
bug #25185 [Serializer] Do not cache attributes if `attributes` in co…
…ntext (sroze)

This PR was merged into the 3.3 branch.

Discussion
----------

[Serializer] Do not cache attributes if `attributes` in context

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

Caching attributes based on the class works only when these attributes are not overwritten. This disables the cache when they are.

To me, this `extractAttributes` method should actually be a `AttributeResolver` dependency that can be decorated using different caching strategies I'd say but... that's a much bigger refactoring that needs more reflection with @dunglas.

Commits
-------

6e87382 Do not cache cache attributes if `attributes` is in the context
This was referenced Nov 30, 2017

@sroze sroze deleted the sroze:bugfix-25108-do-not-cache-attributes-if-in-context branch Jan 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.