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] Remove normalizer cache in Serializer class#17140
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
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.
this is a BC break. so it cannot be merged as-is in 2.3
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.
Oh, didn't see they were protected and not private. Should i just keep them or keep the logic that is filling them to ?
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'd say we should keep them but never fill them, and keep reading from them in case someone fills them with its own logic
57f572d toc3c04f8Comparejvasseur commentedDec 29, 2015
Updated to keep BC as suggested by@nicolas-grekas. |
xabbuh commentedDec 30, 2015
Can we add a test that would fail when the caching was added again to prevent regressions? |
nicolas-grekas commentedDec 30, 2015
A meaningful test would be one implementing a (de)normalizer that uses the data as described above. |
8f21051 to4ea7745Compare4ea7745 to8566dc1Comparejvasseur commentedJan 3, 2016
Tests added. |
xabbuh commentedJan 3, 2016
👍 Status: Reviewed |
dunglas commentedJan 11, 2016
👍 |
dunglas commentedJan 11, 2016
Btw, |
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.
those properties should be annotated with@deprecated
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 was planning to do a PR to master to deprecate them after this one is merged.
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 see. 👍
nicolas-grekas commentedJan 12, 2016
👍 |
Tobion commentedJan 12, 2016
Thank you@jvasseur. |
…jvasseur)This PR was merged into the 2.3 branch.Discussion----------[Serializer] Remove normalizer cache in Serializer class| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |The serializer cache the normalizer/denormalizer to use based only on the class and format. But the supportsNormalization and supportDenormalization methods can decide based on the data passed leading to hard to find bugs when the serializer used a cached normalizer it shouldn't use.Commits-------8566dc1 Remove normalizer cache in Serializer class
This PR was squashed before being merged into the 3.1-dev branch (closes#18588).Discussion----------[Serializer] Add deprecation notices| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | yes| Tests pass? | yes| Fixed tickets |#17140| License | MIT| Doc PR | noneAs discussed in#18583, add deprecation notices to `master` instead.Commits-------d3063ed [Serializer] Add deprecation notices
The serializer cache the normalizer/denormalizer to use based only on the class and format. But the supportsNormalization and supportDenormalization methods can decide based on the data passed leading to hard to find bugs when the serializer used a cached normalizer it shouldn't use.