Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Serializer] Add Support of recursive denormalization on object_to_populate #30607

Merged
merged 1 commit into from Apr 7, 2019

Conversation

Projects
None yet
9 participants
@jewome62
Copy link
Contributor

commented Mar 19, 2019

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 Pending

Currently the deserialization re-create new sub-object with object_to_populate.
This option permit to make object_to_populate recursive.

@jewome62 jewome62 requested a review from dunglas as a code owner Mar 19, 2019

@jewome62 jewome62 force-pushed the jewome62:deep-populate-object branch from e8f290f to 39af1d0 Mar 19, 2019

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 19, 2019

@jewome62 jewome62 changed the title Add Support of recursive denormalization on object_to_populate [Serializer] Add Support of recursive denormalization on object_to_populate Mar 19, 2019

@joelwurtz

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

Nice feature, does this works on collection ? (don't think so seeing the code, and wondering if it's doable)

@jewome62 jewome62 force-pushed the jewome62:deep-populate-object branch 4 times, most recently from be46749 to 7971b08 Apr 1, 2019

@teohhanhui

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

See #21669 (comment)

I don't think those issues are addressed.

@jewome62 jewome62 force-pushed the jewome62:deep-populate-object branch from 7971b08 to 585e86b Apr 2, 2019

@jewome62

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

@joelwurtz i test it tomorrow

@jewome62

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

@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

image

image

@joelwurtz

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

I don't think this PR should solve that problem, futhermore, some users would have different behavior:

  • Replace: like actually
  • Merge: mainly if the index key is the same as the populated object we use the previous object for as an object to populate; also if previous index is not present we remove it, and if the new index is not present in the old list we add it
  • Append: keep current lists and add new object to the list
@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

@jewome62 You need to rebase the PR as we don't merge PRs with merge commits.

@jewome62

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

@jewome62 You need to rebase the PR as we don't merge PRs with merge commits.

I fix that, I have use the confilct resolver into from github

@jewome62 jewome62 force-pushed the jewome62:deep-populate-object branch 2 times, most recently from 1ebbadb to 82d8bb2 Apr 6, 2019

@jewome62

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

Failure of unit tests is not related to this PR

@fabpot fabpot force-pushed the jewome62:deep-populate-object branch from 82d8bb2 to 5b72386 Apr 7, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 7, 2019

Thank you @jewome62.

@fabpot fabpot merged commit 5b72386 into symfony:master Apr 7, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 7, 2019

feature #30607 [Serializer] Add Support of recursive denormalization …
…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        | Pending

Currently 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
@@ -38,6 +39,7 @@ abstract class AbstractObjectNormalizer extends AbstractNormalizer
const SKIP_NULL_VALUES = 'skip_null_values';
const MAX_DEPTH_HANDLER = 'max_depth_handler';
const EXCLUDE_FROM_CACHE_KEY = 'exclude_from_cache_key';
const DEEP_OBJECT_TO_POPULATE = 'deep_object_to_populate';

This comment has been minimized.

Copy link
@dbu

dbu Apr 7, 2019

Contributor

in #30888 i am adding phpdoc to these configuration constants to explain what they do and what values they accept.

This comment has been minimized.

Copy link
@dbu

dbu Apr 7, 2019

Contributor

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.

@@ -274,6 +276,13 @@ public function denormalize($data, $class, $format = null, array $context = [])
continue;
}
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);

This comment has been minimized.

Copy link
@dbu

dbu Apr 7, 2019

Contributor

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.

This comment has been minimized.

Copy link
@dbu

dbu Apr 7, 2019

Contributor

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.

This comment has been minimized.

Copy link
@dbu

dbu Apr 7, 2019

Contributor

=> #30977

@dbu

This comment has been minimized.

Copy link
Contributor

commented Apr 7, 2019

documentation PR: symfony/symfony-docs#11344

wouterj added a commit to symfony/symfony-docs that referenced this pull request Apr 7, 2019

feature #11344 [serializer] document DEEP_OBJECT_TO_POPULATE (dbu)
This PR was merged into the master branch.

Discussion
----------

[serializer] document DEEP_OBJECT_TO_POPULATE

Document the new feature added in symfony/symfony#30607

Commits
-------

7ea06fa document DEEP_OBJECT_TO_POPULATE

fabpot added a commit that referenced this pull request Apr 8, 2019

bug #30977 [serializer] prevent mixup in normalizer of the object to …
…populate (dbu)

This PR was merged into the 3.4 branch.

Discussion
----------

[serializer] prevent mixup in normalizer of the object to populate

EUFOSSA

| 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.