NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Serializer]`default_constructor_arguments` context option for denormalization#25493

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

Nek- wants to merge2 commits intosymfony:masterfromNek-:feature/empty-data-for-denormalization

Closed

[Serializer]`default_constructor_arguments` context option for denormalization#25493

Nek- wants to merge2 commits intosymfony:masterfromNek-:feature/empty-data-for-denormalization

Conversation

Copy link

Contributor

Nek- commentedDec 14, 2017•
edited
Loading

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	(there is no RFC for this)
License	MIT
Doc PR	symfony/symfony-docs#8914

Problems

In the case you want to deserialize value-objects, if all the data required by its constructor arenot given as input, the serializer will throw a simpleRuntimeException exception. This makes impossible to catch it. (as current fix on my projects I use exception message to be sure to catch the good one X.x")

The second problem is a missing feature to fill the required object with an empty one. This needs to be defined by the user because the serializer can't guess how to build it.

Here is a project that exposes the problem of the current behavior.https://github.com/Nek-/api-platform-value-object

Solutions suggested

I suggest a solution in 2 parts because the second part is more touchy.

Replace the current exception by a new specific one
Add a newempty_data option to the context of deserialization so you can specify data for objects impossible to instantiate, this is great because the serializer no more throw exception and the validator can then work as expected and send violations to the user. This solution is inspired by forms solutions to fix the issue with value objects

Here is what you can do with this feature:

class DummyValueObject{publicfunction__construct($foo,$bar) { $this->foo =$foo;$this->bar =$bar; }}$empty =newDummyValueObject('','');$result =$normalizer->denormalize(['foo' =>'Hello'], DummyValueObject::class,'json', ['empty_data' => [        DummyValueObject::class =>$empty,    ],]);// It's impossible to construct a DummyValueObject with only "foo" value. So the serializer// will replace it with the given empty data

There are 2 commits so I can quickly provide you only the first point if you want. Hope you'll like this.

Solution after discussion

New exceptionMissingConstructorArgumentsException
New context optiondefault_constructor_arguments

class DummyValueObject{publicfunction__construct($foo,$bar) { $this->foo =$foo;$this->bar =$bar; }}$result =$normalizer->denormalize(['foo' =>'Hello'], DummyValueObject::class,'json', ['default_constructor_arguments' => [        DummyValueObject::class => ['foo' =>'','bar' =>''],    ],]);// DummyValueObject is contructed with the given `foo` and empty `bar`

carsonbot added Status: Needs Review Feature labels

Dec 14, 2017

Nek- changed the title~~Feature/empty data for denormalization~~[Serializer]empty_data context option for denormalization

Dec 14, 2017

Nek-force-pushed thefeature/empty-data-for-denormalization branch 2 times, most recently from6146b67 tod305b76Compare

December 14, 2017 13:33

nicolas-grekas added this to the4.1 milestone

Dec 18, 2017

dunglas requested changes

Dec 19, 2017

View reviewed changes

src/Symfony/Component/Serializer/Exception/IncompleteInputDataException.php

		namespaceSymfony\Component\Serializer\Exception;

		/**
		* IncompleteInputDataException.

Copy link

Member

dunglasDec 19, 2017

Choose a reason for hiding this comment

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

Can you give a more explicit description or just remove this line?

Copy link

ContributorAuthor

Nek-Dec 20, 2017•
edited
Loading

Choose a reason for hiding this comment

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

It's inspired by (almost) all other exceptions already present. Are you sure?

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Serializer/Exception/RuntimeException.php#L15

src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php Outdated

		$params[] =$constructorParameter->getDefaultValue();
		}else {
		thrownewRuntimeException(
		if (isset($context[static::EMPTY_DATA][$class])) {

Copy link

Member

dunglasDec 19, 2017

Choose a reason for hiding this comment

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

Shouldn't we check if$class is an instance of the key usingis_a (I've no strong opinion about that).

Copy link

ContributorAuthor

Nek-Dec 20, 2017

Choose a reason for hiding this comment

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

With the new way of managing empty data; this is not accurate anymore.

src/Symfony/Component/Serializer/Tests/Normalizer/ObjectNormalizerTest.php Outdated

		'foo' =>10,
		);

		$this->expectException(\Symfony\Component\Serializer\Exception\IncompleteInputDataException::class);

Copy link

Member

dunglasDec 19, 2017

Choose a reason for hiding this comment

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

Prefer using@expectedException and@expectedExceptionMessage instead.

src/Symfony/Component/Serializer/Tests/Normalizer/ObjectNormalizerTest.php Outdated


		$normalizer =newObjectNormalizer();
		$serializer =newSerializer(array($normalizer));
		$normalizer->setSerializer($serializer);

Copy link

Member

dunglasDec 19, 2017

Choose a reason for hiding this comment

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

This line is useless,Serializer will inject itself.

src/Symfony/Component/Serializer/Tests/Normalizer/ObjectNormalizerTest.php Outdated


		$normalizer =newObjectNormalizer();
		$serializer =newSerializer(array($normalizer));
		$normalizer->setSerializer($serializer);

Copy link

Member

dunglasDec 19, 2017

Choose a reason for hiding this comment

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

Same.

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Dec 19, 2017

Copy link

Member

dunglas commentedDec 19, 2017

In your example, thefoo value is lost. Shouldn't we provide defaults for each constructor parameter instead (like we can do in the DI component to build services)? It will allow to only fill the missing parameters instead of discarding all data provided by the using.

Nek-force-pushed thefeature/empty-data-for-denormalization branch 3 times, most recently froma5c2a5e todf432b9Compare

December 20, 2017 19:43

Copy link

ContributorAuthor

Nek- commentedDec 20, 2017

@dunglas updated following your advice :).

Nek- mentioned this pull request

Dec 20, 2017

Add new serializer empty_data feature docsymfony/symfony-docs#8914

Merged

dunglas reviewed

Dec 20, 2017

View reviewed changes

src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php Outdated

		// Don't run set for a parameter passed to the constructor
		$params[] =$parameterData;
		unset($data[$key]);
		}elseif ($allowed && !$ignored &&isset($context[static::EMPTY_DATA][$class][$key])) {

Copy link

Member

dunglasDec 20, 2017

Choose a reason for hiding this comment

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

I would not check for$allowed && !$ignored, because this data is provided by the developer, not by the end user.

Copy link

ContributorAuthor

Nek-Dec 21, 2017

Choose a reason for hiding this comment

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

It can lead to weird behaviors but I'm in favor of given more power to the developer too. 👍

Nek-force-pushed thefeature/empty-data-for-denormalization branch fromdf432b9 to4ae704aCompare

December 21, 2017 08:42

Copy link

ContributorAuthor

Nek- commentedDec 21, 2017•
edited
Loading

@dunglas this is good now :).

Copy link

ContributorAuthor

Nek- commentedDec 21, 2017

Status: Needs Review

carsonbot added Status: Needs Review and removed Status: Needs Work labels

Dec 21, 2017

javiereguiluz added the Serializer label

Dec 28, 2017

Copy link

ContributorAuthor

Nek- commentedJan 8, 2018

ping@dunglas 😄

sroze reviewed

Jan 8, 2018

View reviewed changes

src/Symfony/Component/Serializer/CHANGELOG.md

		-----

		* added`IncompleteInputDataException` new exception for deserialization failure
		of objects that needs data insertion in constructor

Copy link

Contributor

srozeJan 8, 2018

Choose a reason for hiding this comment

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

Is it only when the constructor requires specific arguments? If yes then something likeMissingConstructorArgumentsException would make more sense. If not, then it needs to be clarified here :)

src/Symfony/Component/Serializer/CHANGELOG.md Outdated


		* added`IncompleteInputDataException` new exception for deserialization failure
		of objects that needs data insertion in constructor
		* added an optional`empty_data` option of context to specify a default data in

Copy link

Contributor

srozeJan 8, 2018

Choose a reason for hiding this comment

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

Same reflection... what aboutdefault_constructor_arguments ?

carsonbot removed the Status: Needs Review label

Jan 11, 2018

dunglas approved these changes

Jan 11, 2018

View reviewed changes

Copy link

Member

dunglas left a comment

Choose a reason for hiding this comment

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

(when wording issues in the changelog will be resolved)

carsonbot added the Status: Reviewed label

Jan 11, 2018

Add new MissingConstructorArgumentsException

a3b779f

Before: impossible to catch value object hydratation failureAfter: catch the new exceptionNo BC break.

Nek-force-pushed thefeature/empty-data-for-denormalization branch from4ae704a to7cf8514Compare

January 15, 2018 06:43

Copy link

ContributorAuthor

Nek- commentedJan 15, 2018

@sroze that's absolutely relevant. I made the naming changes. 👍

Nek- changed the title~~[Serializer]empty_data context option for denormalization~~[Serializer]default_constructor_arguments context option for denormalization

Jan 15, 2018

sroze reviewed

Jan 15, 2018

View reviewed changes

src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php Outdated

		constGROUPS ='groups';
		constATTRIBUTES ='attributes';
		constALLOW_EXTRA_ATTRIBUTES ='allow_extra_attributes';
		constEMPTY_DATA ='default_constructor_arguments';

Copy link

Contributor

srozeJan 15, 2018

Choose a reason for hiding this comment

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

Might as well rename theEMPTY_DATA constant then :)

Copy link

ContributorAuthor

Nek-Jan 15, 2018•
edited
Loading

Choose a reason for hiding this comment

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

Arg! Fixed.

Add new feature to serializer: default_constructor_arguments

7b8b165

You can now add empty_data to the context on deserializationof objects. This allow you to deserialize objects that haverequirements in the constructor that can't be given.Basically, it helps you hydrate with value objects, so thevalidation component can invalid the object without theserializer send an error.

Nek-force-pushed thefeature/empty-data-for-denormalization branch from7cf8514 to7b8b165Compare

January 15, 2018 14:40

sroze approved these changes

Jan 15, 2018

View reviewed changes

Copy link

Contributor

sroze left a comment

Choose a reason for hiding this comment

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

👌

Copy link

Contributor

Taluu commentedJan 16, 2018

Supporting a callback in the option with the data could help to instantiate value object or entity objects with required params that can't be set to their default value could be awesome.

Taluu approved these changes

Jan 16, 2018

View reviewed changes

Copy link

Member

fabpot commentedJan 19, 2018

Thank you@Nek-.

fabpot closed this in2aa54b8

Jan 19, 2018

Nek- deleted the feature/empty-data-for-denormalization branch

January 19, 2018 22:41

javiereguiluz mentioned this pull request

Jan 21, 2018

[Serializer] Added a default_constructor_arguments context option for denormalizationsymfony/symfony-docs#9092

Closed

Nek- mentioned this pull request

Mar 6, 2018

Make resource constructor parameters writablesapi-platform/core#1749

Merged

bwegrzyn mentioned this pull request

Apr 29, 2018

Unable to specify custom serializer context for GraphQL callsapi-platform/core#1904

Closed

fabpot mentioned this pull request

May 7, 2018

Release v4.1.0-BETA1#27181

Merged

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request

Jun 27, 2018

minor#8914Add new serializer empty_data feature doc (Nek-, javiereg…

cab06a1

…uiluz)This PR was merged into the 4.1 branch.Discussion----------Add new serializer empty_data feature docThis is the documentation related to the following feature:symfony/symfony#25493Commits-------9f31bbb Fix not precise enough sentenceb0d3fe8 Minor reword5a48201 Add new serializer empty_data feature doc

Labels

Feature Serializer Status: Reviewed

8 participants

Movatterモバイル変換

Uh oh!

[Serializer]default_constructor_arguments context option for denormalization#25493

[Serializer]default_constructor_arguments context option for denormalization#25493

Uh oh!

Conversation

Nek- commentedDec 14, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Problems

Solutions suggested

Solution after discussion

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nek-Dec 20, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commentedDec 19, 2017

Uh oh!

Nek- commentedDec 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nek- commentedDec 21, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Nek- commentedDec 21, 2017

Uh oh!

Nek- commentedJan 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas left a comment

Choose a reason for hiding this comment

Uh oh!

Nek- commentedJan 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nek-Jan 15, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sroze left a comment

Choose a reason for hiding this comment

Uh oh!

Taluu commentedJan 16, 2018

Uh oh!

fabpot commentedJan 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

[Serializer]`default_constructor_arguments` context option for denormalization#25493

[Serializer]`default_constructor_arguments` context option for denormalization#25493

Nek- commentedDec 14, 2017•
edited
Loading

Nek-Dec 20, 2017•
edited
Loading

Nek- commentedDec 21, 2017•
edited
Loading

Nek-Jan 15, 2018•
edited
Loading