[Serializer] Remove normalizer cache in Serializer class #17140
Conversation
| @@ -40,8 +40,6 @@ class Serializer implements SerializerInterface, NormalizerInterface, Denormaliz | |||
| protected $encoder; | |||
| protected $decoder; | |||
| protected $normalizers = array(); | |||
| protected $normalizerCache = array(); | |||
| protected $denormalizerCache = array(); | |||
There was a problem hiding this comment.
this is a BC break. so it cannot be merged as-is in 2.3
There was a problem hiding this comment.
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.
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 to
c3c04f8
Compare
|
Updated to keep BC as suggested by @nicolas-grekas. |
|
Can we add a test that would fail when the caching was added again to prevent regressions? |
|
A meaningful test would be one implementing a (de)normalizer that uses the data as described above. |
8f21051 to
4ea7745
Compare
4ea7745 to
8566dc1
Compare
|
Tests added. |
|
👍 Status: Reviewed |
|
👍 |
|
Btw, |
| @@ -237,7 +237,6 @@ private function normalizeObject($object, $format = null, array $context = array | |||
| foreach ($this->normalizers as $normalizer) { | |||
| if ($normalizer instanceof NormalizerInterface | |||
| && $normalizer->supportsNormalization($object, $format)) { | |||
| $this->normalizerCache[$class][$format] = $normalizer; | |||
There was a problem hiding this comment.
those properties should be annotated with @deprecated
There was a problem hiding this comment.
I was planning to do a PR to master to deprecate them after this one is merged.
|
👍 |
|
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 | none As 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.