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] Allow denormalization of constructor arguments to array of objects #27221

Conversation

cezarystepkowski
Copy link

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #25524
License MIT
Doc PR todo

Currently it is not possible to denormalize array of objects passed to the class constructor as described in the issue #25524. When instantiating an object if constructor parameter is of array type its value is being set as it is without any further denormalization.

This pull request handles it by providing this further denormalization in the same way like it is currently done for non-constructor parameters. It means that now the constructor parameter type can be read from the php doc annotation and if it is an array of objects it will be denormalized correctly.

@cezarystepkowski cezarystepkowski changed the title [Serializer] Allow denormalization of constructor arguments to array objects [Serializer] Allow denormalization of constructor arguments to array of objects May 10, 2018
@nicolas-grekas nicolas-grekas added this to the next milestone May 10, 2018
@dunglas
Copy link
Member

dunglas commented May 16, 2018

Shouldn't this be implemented directly in the PropertyInfo component just as it was done for parsing types using the reflection API in #25605?

@cezarystepkowski
Copy link
Author

@dunglas I think the answer to your question is here: #25524 (comment)

Maybe it's not super clear from the diff but I barely introduced any new logic with this pull request.
The only change was to abstract parsing of constructor parameter in AbstractNormalizer into protected method so that it can be overridden in AbstractObjectNormalizer so that AbstractObjectNormalizer can call validateAndDenormalize for arrays. validateAndDenormalize already parses the array of objects correctly.

@dunglas
Copy link
Member

dunglas commented May 29, 2018

@cezarystepkowski it makes sense. As we try to avoid the proliferation of such methods in the abstract classes, can you mark them @internal?

@cezarystepkowski cezarystepkowski force-pushed the serializer-denormalization-constructor-array-of-objects branch from 7b04753 to 1244faa Compare May 30, 2018 14:25
@cezarystepkowski
Copy link
Author

@dunglas I marked the methods as internal. Additionally, I rebased onto current master.

@ragboyjr
Copy link
Contributor

@cezarystepkowski what is the status of this? This change is awesome and would definitely be a huge improvement.

@cezarystepkowski cezarystepkowski force-pushed the serializer-denormalization-constructor-array-of-objects branch from 1244faa to 0f9d01f Compare November 25, 2018 10:26
@cezarystepkowski
Copy link
Author

@ragboyjr I rebased onto current master and resolved conflicts.

@dunglas All tests inside serializer component are passing. The coding standards failure is not caused by changes in this PR so I left it failing (I can fix it of course if needed). Is there anything else needed?

@cezarystepkowski cezarystepkowski force-pushed the serializer-denormalization-constructor-array-of-objects branch from 8272133 to 06ef399 Compare December 1, 2018 10:37
@cezarystepkowski
Copy link
Author

@nicolas-grekas both suggested changes applied.

@nicolas-grekas
Copy link
Member

Thanks. Would you mind rebasing please (and squashing meanwhile if you want)? We don't merge PRs that have merge commits.

@cezarystepkowski cezarystepkowski force-pushed the serializer-denormalization-constructor-array-of-objects branch from 06ef399 to cf58d82 Compare December 2, 2018 14:44
@cezarystepkowski
Copy link
Author

@nicolas-grekas rebased and squashed :)

@ogizanagi
Copy link
Member

A new entry is missing from src/Symfony/Component/Serializer/CHANGELOG.md to mention this feature :)

@cezarystepkowski
Copy link
Author

@ogizanagi I updated the changelog, as version 4.2.0 is already released, I've added the entry for 4.3.0. If it's not correct please let me know how should it be :)

@cezarystepkowski
Copy link
Author

I am closing this PR as the same functionality has been already merged here: #29513

@cezarystepkowski cezarystepkowski deleted the serializer-denormalization-constructor-array-of-objects branch January 17, 2019 14:16
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants