Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Serializer] Respect ignored attributes in cache key of normalizer#30907
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| return$allowedAttributes; | ||
| } | ||
| if (isset($context['attributes'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we previously skipped the cache if attributes are defined in the context. now we use the attributes as part of the cache key.
6737389 to7b64d88Compare| 'context' =>$context, | ||
| 'ignored' =>$this->ignoredAttributes, | ||
| 'camelized' =>$this->camelizedAttributes, | ||
| ])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
i hope i caught all normalizer state that could influence which attributes are available and not too many.
things like circular reference or callbacks should not change which attributes are available i guess?
(serialize one array rather than concat several serialize result, assuming that gives better performance)
dunglas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Such changes require careful manual testing and profiling. But LGTM.
dbu commentedApr 7, 2019
FTR: build error is due to a travis install error during hhvm build. i am confident this is not related to the PR ;-) the other builds are green. |
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
a81cbc0 toea4f387Compareea4f387 tobe2e3faCompare| * @internal | ||
| */ | ||
| protectedfunctioncreateChildContext(array$parentContext,$attribute) | ||
| protectedfunctioncreateChildContext(array$parentContext,$attribute/*, ?string $format*/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should i also trigger a deprecation warning here? we don't actually use the format at all...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not on 3.4 but on master yes - also, this should be/*, string $format = null */, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@dbu Can you fix the typos? (no? and a space before*/?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
as its a protected function, i actually think ?string would make more sense, you will always pass the parameter even when its null. nullable types is php 7.2 only, which i guess symfony 5 will require at least. i changed it to the optional argument syntax for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
also, if its indeed an optional argument, the deprecation warning would be unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok, fair enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
shall i change it to nullable argument? the nullable syntax was introduced in 7.1 actually. or shall i only change it on master when i also add a deprecation warning?
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| */ | ||
| privatefunctiongetCacheKey($format,array$context) | ||
| { | ||
| unset($context['cache_key']);// avoid artifically different keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if we would keep this field, every call to this method generates a new key as the previous key is part of what we hash. the correct thing is to not include the previous key in the hash as its not meaningful information.
20aae5f to100fe8dCompare100fe8d to926d228Comparefabpot commentedApr 8, 2019
Thank you@dbu. |
…rmalizer (dbu)This PR was squashed before being merged into the 3.4 branch (closes#30907).Discussion----------[Serializer] Respect ignored attributes in cache key of normalizerEUFOSSA| 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 | -Do not share the attributes cache in object normalizer when using a different setting for the ignoredAttributes setting.In Symfony 4.2, the setter is deprecated in favor of the ignored_attibutes option in the $context. When merging this up, we will however still need to respect the field as well for BC, the cache key does not look at the default context (apart from the deprecated modifiers, the default context is immutable)There might be performance regression for some use cases, but also could be a performance improvement when using 'attributes' in the context with lists of objects of the same class.Commits-------926d228 [Serializer] Respect ignored attributes in cache key of normalizer
fabpot commentedApr 8, 2019
@dbu I think we need your help with merging this one on 4.2 and master. |
dbu commentedApr 8, 2019
happy to help. do i create a branch from 4.2, merge 3.4 to it, then solve conflicts and do a PR with that? |
fabpot commentedApr 8, 2019
@dbu That would be super hepful, yes. |
xabbuh commentedApr 8, 2019
dbu commentedApr 8, 2019
ah, now i was confused until i red this :-D resolution looks good to me. |
xabbuh commentedApr 8, 2019
@dbu Thank you! 👍 |
dbu commentedApr 8, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
should i do the deprecation change against 4.2 or master?ec41d76#r33074748 => will do against master when changes have been merged there. |
fabpot commentedApr 8, 2019
@dbu 4.2 has been merged into master now. |
This PR was squashed before being merged into the 4.3-dev branch (closes#30888).Discussion----------[serializer] extract normalizer tests to traitseufossa| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | Relates to#30818| License | MIT| Doc PR | -As discussed with@joelwurtz, extract normalizer functionality tests into traits to ensure consistent behaviour of all normalizers.* [x] Rebase when#30977,#30950 and#30907 are merged to master **blocker*** [x] Clean up order of trait inclusion and methods in the tests* [x] Clean up fixture classes of the traits. I started having one class named the same as the trait, where possibleStuff that we should do eventually, but can also do in separate pull requests, after this one has been merged:* [ ] Extract all features that we can (the existing normalizer tests should more or less only have the legacy tests in them, all functionality should be in trait)* [ ] Run test coverage and increase coverage so that we cover all important features and all relevant error cases.Commits-------2b6ebea [serializer] extract normalizer tests to traits
…out the format parameter (dbu)This PR was submitted for the master branch but it was merged into the 4.3 branch instead (closes#31030).Discussion----------[Serializer] Deprecate calling createChildContext without the format parameter| 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#30907 deprecate omitting the format parameter when extending the AbstractNormalizer.Commits-------cb77902 deprecate calling createChildContext without the format parameter
…lasr)This PR was merged into the 4.4 branch.Discussion----------[Serializer] Fix tests wrongly marked as incomplete| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -#30907 has been merged meanwhile.Commits-------e632302 [Serializer] Fix tests marked as incomplete
Uh oh!
There was an error while loading.Please reload this page.
EUFOSSA
Do not share the attributes cache in object normalizer when using a different setting for the ignoredAttributes setting.
In Symfony 4.2, the setter is deprecated in favor of the ignored_attibutes option in the $context. When merging this up, we will however still need to respect the field as well for BC, the cache key does not look at the default context (apart from the deprecated modifiers, the default context is immutable)
There might be performance regression for some use cases, but also could be a performance improvement when using 'attributes' in the context with lists of objects of the same class.