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] Fix argument object denormalization #20690

Merged
merged 1 commit into from Dec 6, 2016
Merged

[Serializer] Fix argument object denormalization #20690

merged 1 commit into from Dec 6, 2016

Conversation

ogizanagi
Copy link
Member

@ogizanagi ogizanagi commented Nov 29, 2016

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.

@theofidry
Copy link
Contributor

👍 I had this patch in mind since day one but never found the time to do it thanks very much 🎉

I personally hesitated between:

  • This solution
  • Having a null encoder (the patch I used on my side meanwhile)
  • Adding a DenormalizerAware which IMO is more appropriate than SerializerAware

@tyx
Copy link
Contributor

tyx commented Nov 30, 2016

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 !

@ogizanagi
Copy link
Member Author

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

@tyx
Copy link
Contributor

tyx commented Nov 30, 2016

😢

@dunglas
Copy link
Member

dunglas commented Nov 30, 2016

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(
Copy link
Member

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).

@dunglas
Copy link
Member

dunglas commented Nov 30, 2016

👍

@ogizanagi
Copy link
Member Author

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).

@dunglas : 3.1? But it was introduced in #19277 which has been merged in 3.2 only 😕

@dunglas
Copy link
Member

dunglas commented Nov 30, 2016

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()));
Copy link
Member

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?

Copy link
Member Author

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)

Copy link
Member Author

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 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@dunglas
Copy link
Member

dunglas commented Dec 6, 2016

Thanks for fixing this bug @ogizanagi.

@dunglas dunglas merged commit 27de65a into symfony:3.2 Dec 6, 2016
dunglas added a commit that referenced this pull request Dec 6, 2016
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
@ogizanagi ogizanagi deleted the fix/3_2/serializer/fix_arg_object_denormalization branch December 6, 2016 11:14
@fabpot fabpot mentioned this pull request Dec 13, 2016
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