Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Serializer] PropertyNormalizer doesn't support DiscriminatorMap#36695
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
agven commentedMay 5, 2020
| Q | A |
|---|---|
| Branch? | 4.4 |
| Bug fix? | yes |
| New feature? | no |
| Deprecations? | no |
| Tickets | Fix#36585 |
| License | MIT |
| Doc PR |
a5e2916 to3434775Compare0bafcf0 to25e9d9fCompare@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 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.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -412,11 +412,6 @@ public function testNoTraversableSupport() | |||
| $this->assertFalse($this->normalizer->supportsNormalization(new \ArrayObject())); | |||
| } | |||
| public function testNoStaticPropertySupport() | |||
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.
Tests must not be removed, and they must pass to prevent BC breaks.
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 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 ofticket #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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This looks like a BC break.
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.
Sorry, could you explain Why does backward compatibility break ?
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.
@dunglas do you have any update on this for me ?
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.
@dunglas ping
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
1abf867 tof17fcf5Comparef17fcf5 to2b070b2Compareimba28 commentedNov 8, 2023
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 | MITFollow up of#36695Commits-------50d086c [Serializer] Move discrimination to abstract