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] Add Support of recursive denormalization on object_to_populate#30607
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
e8f290f to39af1d0Comparejoelwurtz commentedMar 26, 2019
Nice feature, does this works on collection ? (don't think so seeing the code, and wondering if it's doable) |
be46749 to7971b08Compareteohhanhui commentedApr 1, 2019
I don't think those issues are addressed. |
jewome62 commentedApr 2, 2019
@joelwurtz i test it tomorrow |
jewome62 commentedApr 3, 2019 • 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.
@joelwurtz you have right, an object collection in the parent has no populated object, the problem is to have a reference for linked the object passed in the 'object_to_populate' and the object received by decoding. Either we consider this PR for objects containing other objects, or we have a solution to bind collection objects to the input received
|
joelwurtz commentedApr 3, 2019
I don't think this PR should solve that problem, futhermore, some users would have different behavior:
|
src/Symfony/Component/Serializer/Normalizer/ObjectToPopulateTrait.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.
fabpot commentedApr 6, 2019
@jewome62 You need to rebase the PR as we don't merge PRs with merge commits. |
jewome62 commentedApr 6, 2019
I fix that, I have use the confilct resolver into from github |
1ebbadb to82d8bb2Comparejewome62 commentedApr 6, 2019
Failure of unit tests is not related to this PR |
fabpot commentedApr 7, 2019
Thank you@jewome62. |
…on object_to_populate (jewome62)This PR was squashed before being merged into the 4.3-dev branch (closes#30607).Discussion----------[Serializer] Add Support of recursive denormalization on object_to_populate| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | Pending| Fixed tickets |#21669| License | MIT| Doc PR | PendingCurrently the deserialization re-create new sub-object with object_to_populate.This option permit to make object_to_populate recursive.Commits-------5b72386 [Serializer] Add Support of recursive denormalization on object_to_populate
| constSKIP_NULL_VALUES ='skip_null_values'; | ||
| constMAX_DEPTH_HANDLER ='max_depth_handler'; | ||
| constEXCLUDE_FROM_CACHE_KEY ='exclude_from_cache_key'; | ||
| constDEEP_OBJECT_TO_POPULATE ='deep_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.
in#30888 i am adding phpdoc to these configuration constants to explain what they do and what values they accept.
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.
as this is already merged, i will rebase#30888 and document it there. and extract the test for this into a trait like we do the other tests there.
| if ($context[self::DEEP_OBJECT_TO_POPULATE] ??$this->defaultContext[self::DEEP_OBJECT_TO_POPULATE] ??false) { | ||
| try { | ||
| $context[self::OBJECT_TO_POPULATE] =$this->getAttributeValue($object,$attribute,$format,$context); |
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 overwrites the context. i think this means that if there are more loops, you might have a previous object to populate in the context. i think we should unset OBJECT_TO_POPULATE if it does not find anything and when the DEEP_OBJECT_TO_POPULATE flag is not true.
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.
we found where OBJECT_TO_POPULATE is unset. so this is fine as is.
i will try to do a test and fix to make OBJECT_TO_POPULATE more robust, as i think there are edge cases that would lead to weird behaviour.
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.
=>#30977
dbu commentedApr 7, 2019
documentation PR:symfony/symfony-docs#11344 |
This PR was merged into the master branch.Discussion----------[serializer] document DEEP_OBJECT_TO_POPULATEDocument the new feature added insymfony/symfony#30607Commits-------7ea06fa document DEEP_OBJECT_TO_POPULATE
…populate (dbu)This PR was merged into the 3.4 branch.Discussion----------[serializer] prevent mixup in normalizer of the object to populateEUFOSSA| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -OBJECT_TO_POPULATE is meant to specify the top level object. The implementation left the option in the context and it would be used whenever we have the first element that matches the class.#30607 (to master) introduces the feature to also keep the instances of attributes to deeply populate an existing object tree. In both cases, we do not want the mix up to happen with what the current OBJECT_TO_POPULATE is.Commits-------fdb668e prevent mixup of the object to populate
Currently the deserialization re-create new sub-object with object_to_populate.
This option permit to make object_to_populate recursive.