-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
empty_data
context option for denormalization
6146b67
to
d305b76
Compare
namespace Symfony\Component\Serializer\Exception; | ||
|
||
/** | ||
* IncompleteInputDataException. |
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 you give a more explicit description or just remove this line?
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.
It's inspired by (almost) all other exceptions already present. Are you sure?
@@ -356,7 +359,10 @@ protected function instantiateObject(array &$data, $class, array &$context, \Ref | |||
} elseif ($constructorParameter->isDefaultValueAvailable()) { | |||
$params[] = $constructorParameter->getDefaultValue(); | |||
} else { | |||
throw new RuntimeException( | |||
if (isset($context[static::EMPTY_DATA][$class])) { |
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.
Shouldn't we check if $class
is an instance of the key using is_a
(I've no strong opinion about that).
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.
With the new way of managing empty data; this is not accurate anymore.
'foo' => 10, | ||
); | ||
|
||
$this->expectException(\Symfony\Component\Serializer\Exception\IncompleteInputDataException::class); |
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.
Prefer using @expectedException
and @expectedExceptionMessage
instead.
|
||
$normalizer = new ObjectNormalizer(); | ||
$serializer = new Serializer(array($normalizer)); | ||
$normalizer->setSerializer($serializer); |
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.
This line is useless, Serializer
will inject itself.
|
||
$normalizer = new ObjectNormalizer(); | ||
$serializer = new Serializer(array($normalizer)); | ||
$normalizer->setSerializer($serializer); |
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.
Same.
In your example, the |
a5c2a5e
to
df432b9
Compare
@dunglas updated following your advice :). |
@@ -353,10 +356,12 @@ protected function instantiateObject(array &$data, $class, array &$context, \Ref | |||
// 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])) { |
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.
I would not check for $allowed && !$ignored
, because this data is provided by the developer, not by the end user.
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.
It can lead to weird behaviors but I'm in favor of given more power to the developer too. 👍
df432b9
to
4ae704a
Compare
@dunglas this is good now :). |
Status: Needs Review |
ping @dunglas 😄 |
----- | ||
|
||
* added `IncompleteInputDataException` new exception for deserialization failure | ||
of objects that needs data insertion in constructor |
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.
Is it only when the constructor requires specific arguments? If yes then something like MissingConstructorArgumentsException
would make more sense. If not, then it needs to be clarified here :)
|
||
* 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 |
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.
Same reflection... what about default_constructor_arguments
?
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.
(when wording issues in the changelog will be resolved)
Before: impossible to catch value object hydratation failure After: catch the new exception No BC break.
4ae704a
to
7cf8514
Compare
@sroze that's absolutely relevant. I made the naming changes. 👍 |
empty_data
context option for denormalizationdefault_constructor_arguments
context option for denormalization
@@ -36,6 +37,7 @@ | |||
const GROUPS = 'groups'; | |||
const ATTRIBUTES = 'attributes'; | |||
const ALLOW_EXTRA_ATTRIBUTES = 'allow_extra_attributes'; | |||
const EMPTY_DATA = 'default_constructor_arguments'; |
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.
Might as well rename the EMPTY_DATA
constant then :)
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.
Arg! Fixed.
You can now add empty_data to the context on deserialization of objects. This allow you to deserialize objects that have requirements in the constructor that can't be given. Basically, it helps you hydrate with value objects, so the validation component can invalid the object without the serializer send an error.
7cf8514
to
7b8b165
Compare
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.
👌
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. |
Thank you @Nek-. |
…uiluz) This PR was merged into the 4.1 branch. Discussion ---------- Add new serializer empty_data feature doc This is the documentation related to the following feature: symfony/symfony#25493 Commits ------- 9f31bbb Fix not precise enough sentence b0d3fe8 Minor reword 5a48201 Add new serializer empty_data feature doc
Problems
In the case you want to deserialize value-objects, if all the data required by its constructor are not given as input, the serializer will throw a simple
RuntimeException
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.
empty_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 objectsHere is what you can do with this feature:
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
MissingConstructorArgumentsException
default_constructor_arguments