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] Add Support forobject_to_populate in CustomNormalizer#21716
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
object_to_populate to CustomNormalizerobject_to_populate in CustomNormalizer| $context[static::OBJECT_TO_POPULATE] instanceof $class | ||
| ) { | ||
| $object = $context[static::OBJECT_TO_POPULATE]; | ||
| if (null !== $object = $this->extractObjectToPopulate($class, $context, static::OBJECT_TO_POPULATE)) { | ||
| unset($context[static::OBJECT_TO_POPULATE]); |
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.
Shouldn't this line be moved in in the trait too?
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.
Would involve passing context by reference into the trait. That's the only reason I didn't do it.
| return $context[$key]; | ||
| } | ||
| return null; |
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.
Useless, can be removed.
| * @param string $class The class the object should be | ||
| * @param $context The denormalization context | ||
| * @param string $key They in which to look for the object to populate. | ||
| * Keeps backwards compatibility with `AbstractNormalizer`. |
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.
There is no reason to mention this in the comment IMO.
felipsmartinsFeb 24, 2017 • 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.
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.
Why not?
dunglas commentedFeb 24, 2017 • 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.
Just asking, what is the use case of this class? I think that it's always better to create custom normalizers (implementing the I would be in favor of deprecating this class now and remove it from Symfony 4. |
chrisguitarguy commentedFeb 24, 2017 • 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.
In my case, I was interested in using it with a "dumb" dto-like object. class SomeDtoimplements DenormalizableInterface{// ...} The idea being to keep everything close together: the normalization, denormalization, and the DTO itself. So basically what you are against :) I was interested in doing this because I think it's a lot nicer developer experience to keep the normalization/denormalization closer to the object being serialized/deserialized. Easier to understand and catch at a glance. Doesn't require you to register services in the container with a special tag, etc. |
@chrisguitarguy Can you please rebase it so that we can merge it? Thanks. Base should also be changed to 3.4. |
And use that trait in `AbstractNormalizer`.
And fallback to the default `new $class();` behavior.
3a76492 to60c1c8fCompare60c1c8f to75ad5b9Comparechrisguitarguy commentedSep 27, 2017 • 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.
Looks like he approved this PR without any further changes :) |
Thank you@chrisguitarguy. |
…ustomNormalizer (chrisguitarguy)This PR was squashed before being merged into the 3.4 branch (closes#21716).Discussion----------[Serializer] Add Support for `object_to_populate` in CustomNormalizer| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#21715| License | MIT| Doc PR | n/aThis pulls a trait out of `AbstractNormalizer` with a method to extract the object to populate and adds some tests for it. Then uses that trait in both `AbstractNormalizer` and `CustomNormalizer` so both can support the `object_to_populate` key.Commits-------ec9242d [Serializer] Add Support for in CustomNormalizer
This pulls a trait out of
AbstractNormalizerwith a method to extract the object to populate and adds some tests for it. Then uses that trait in bothAbstractNormalizerandCustomNormalizerso both can support theobject_to_populatekey.