-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Serializer] Fix argument object denormalization #20690
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] Fix argument object denormalization #20690
Conversation
👍 I had this patch in mind since day one but never found the time to do it thanks very much 🎉 I personally hesitated between:
|
Thanks for this fix, looks great for me. For me this feature was the most expected of the 3.2, I hope we can have this one merged within the first release ! |
@tyx : That's already too late for this 😅 http://symfony.com/blog/symfony-3-2-0-released |
😢 |
IMO it should be merged as a bug fix in the 3.1 branch (and so we'll be in the next minor version of 3.2). |
$jsonData = '{"bar":{"value":"baz"}}'; | ||
|
||
$serializer = new Serializer( | ||
array( |
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.
Can be inlined (same below).
👍 |
Sorry, you're right, it should be merged in 3.2. |
@@ -336,8 +337,11 @@ protected function instantiateObject(array &$data, $class, array &$context, \Ref | ||
$parameterData = $data[$key]; | ||
try { | ||
if (null !== $constructorParameter->getClass()) { | ||
if (!$this->serializer instanceof DenormalizerInterface) { | ||
throw new LogicException(sprintf('Cannot create an instance of %s from serialized data because the injected serializer is not a denormalizer', $constructorParameter->getClass())); |
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.
Should we add the FQCN of the serializer here to provide some more context information?
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.
Similar exceptions above do not add such information (I guess the stacktrace is enough for this)
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.
Hmm, but other ones are runtime exceptions. Whereas here we need to fix the code/configuration and the stacktrace will only show AbstractNormalizer
. So maybe you're right 👍
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.
Done
Thanks for fixing this bug @ogizanagi. |
This PR was merged into the 3.2 branch. Discussion ---------- [Serializer] Fix argument object denormalization | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20670 | License | MIT | Doc PR | N/A Fixes #20670. I've seen #19277 (diff) so I think it's the right thing to do, but I didn't follow the thread at the time, so I may have missed something. Ping @theofidry, @dunglas. Commits ------- 27de65a [Serializer] Fix argument object denormalization
Fixes #20670. I've seen #19277 (diff) so I think it's the right thing to do, but I didn't follow the thread at the time, so I may have missed something.
Ping @theofidry, @dunglas.