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

Conversation

Projects
None yet
6 participants
@ogizanagi
Member

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

This comment has been minimized.

Contributor

theofidry commented Nov 29, 2016

👍 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

This comment has been minimized.

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

This comment has been minimized.

Member

ogizanagi commented Nov 30, 2016

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

This comment has been minimized.

Contributor

tyx commented Nov 30, 2016

😢

@dunglas

This comment has been minimized.

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(

This comment has been minimized.

@dunglas

dunglas Nov 30, 2016

Member

Can be inlined (same below).

@dunglas

This comment has been minimized.

Member

dunglas commented Nov 30, 2016

👍

@ogizanagi

This comment has been minimized.

Member

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

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

@dunglas

This comment has been minimized.

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()));

This comment has been minimized.

@xabbuh

xabbuh Dec 2, 2016

Member

Should we add the FQCN of the serializer here to provide some more context information?

This comment has been minimized.

@ogizanagi

ogizanagi Dec 2, 2016

Member

Similar exceptions above do not add such information (I guess the stacktrace is enough for this)

This comment has been minimized.

@ogizanagi

ogizanagi Dec 2, 2016

Member

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 👍

This comment has been minimized.

@ogizanagi
@dunglas

This comment has been minimized.

Member

dunglas commented Dec 6, 2016

Thanks for fixing this bug @ogizanagi.

@dunglas dunglas merged commit 27de65a into symfony:3.2 Dec 6, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

dunglas added a commit that referenced this pull request Dec 6, 2016

bug #20690 [Serializer] Fix argument object denormalization (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

@ogizanagi ogizanagi deleted the ogizanagi:fix/3_2/serializer/fix_arg_object_denormalization branch Dec 6, 2016

@fabpot fabpot referenced this pull request Dec 13, 2016

Merged

Release v3.2.1 #20897

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment