[Serializer] PropertyNormalizer doesn't support DiscriminatorMap #36695
Conversation
agven
commented
May 5, 2020
agven
commented
| Q | A |
|---|---|
| Branch? | 4.4 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Tickets | Fix #36585 |
| License | MIT |
| Doc PR |
agven
force-pushed
the
serializer_property_normalizer
branch
from
a5e2916
to
3434775
Compare
agven
force-pushed
the
serializer_property_normalizer
branch
3 times, most recently
from
0bafcf0
to
25e9d9f
Compare
|
@dunglas what do you think ? |
|
@dunglas, @nicolas-grekas please take a look and answer the question what do I need to do in order to make my pull request acceptable ? |
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php
Show resolved
Hide resolved
| @@ -412,11 +412,6 @@ public function testNoTraversableSupport() | |||
| $this->assertFalse($this->normalizer->supportsNormalization(new \ArrayObject())); | |||
| } | |||
|
|
|||
| public function testNoStaticPropertySupport() | |||
There was a problem hiding this comment.
Tests must not be removed, and they must pass to prevent BC breaks.
There was a problem hiding this comment.
I deleted this test because there isn't reason to leave it. If you have another opinion, please give explanation. What does wrong if class contains static properties ? (not in general) I mean in serialization context.
Also I would like to tell you that this PR contains BC, because we need to fix base normalizer class. Why we need to do it - please see description of ticket #36585
| } | ||
|
|
||
| return $attribute === $this->discriminatorCache[$cacheKey] ? $this->classDiscriminatorResolver->getTypeForMappedObject($object) : $this->propertyAccessor->getValue($object, $attribute); | ||
| return $this->propertyAccessor->getValue($object, $attribute); |
There was a problem hiding this comment.
Sorry, could you explain Why does backward compatibility break ?
There was a problem hiding this comment.
@dunglas do you have any update on this for me ?
There was a problem hiding this comment.
@dunglas , @nicolas-grekas Let's try to make a decision. What will we do next ?
There are two options:
- We will fix this issue together. It means that I am waiting your propose how we fix it because my propose contains into this PR.
- I'll close this issue and bug because of "looks like a BC break"
There was a problem hiding this comment.
The changes done in this PR look like a new feature according to our definition, so this should target 5.4, where we can handle the BC break is possible.
agven
force-pushed
the
serializer_property_normalizer
branch
6 times, most recently
from
1abf867
to
f17fcf5
Compare
agven
force-pushed
the
serializer_property_normalizer
branch
from
f17fcf5
to
2b070b2
Compare
|
Since this pull requests seems to have been stale for a while: Are there any plans to fix #36585? |
|
Replaced by #52681 |
…ormalizer (mtarld) This PR was merged into the 5.4 branch. Discussion ---------- [Serializer] Fix support for DiscriminatorMap in PropertyNormalizer | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #36585 | License | MIT Follow up of #36695 Commits ------- 50d086c [Serializer] Move discrimination to abstract
