Skip to content

[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

Closed

Conversation

Nek-
Copy link
Contributor

@Nek- Nek- commented Dec 14, 2017

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

  1. Replace the current exception by a new specific one
  2. Add a new 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 objects

Here is what you can do with this feature:

class DummyValueObject
{
    public function __construct($foo, $bar) { $this->foo = $foo; $this->bar = $bar; }
}

$empty = new DummyValueObject('', '');
$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

  1. New exception MissingConstructorArgumentsException
  2. New context option default_constructor_arguments
class DummyValueObject
{
    public function __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`

@Nek- Nek- changed the title Feature/empty data for denormalization [Serializer] empty_data context option for denormalization Dec 14, 2017
@Nek- Nek- force-pushed the feature/empty-data-for-denormalization branch 2 times, most recently from 6146b67 to d305b76 Compare December 14, 2017 13:33
@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Dec 18, 2017
namespace Symfony\Component\Serializer\Exception;

/**
* IncompleteInputDataException.
Copy link
Member

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
Contributor Author

@Nek- Nek- Dec 20, 2017

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

@@ -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])) {
Copy link
Member

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

Copy link
Contributor Author

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

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

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

Choose a reason for hiding this comment

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

Same.

@dunglas
Copy link
Member

dunglas commented Dec 19, 2017

In your example, the foo 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- Nek- force-pushed the feature/empty-data-for-denormalization branch 3 times, most recently from a5c2a5e to df432b9 Compare December 20, 2017 19:43
@Nek-
Copy link
Contributor Author

Nek- commented Dec 20, 2017

@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])) {
Copy link
Member

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
Contributor Author

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- Nek- force-pushed the feature/empty-data-for-denormalization branch from df432b9 to 4ae704a Compare December 21, 2017 08:42
@Nek-
Copy link
Contributor Author

Nek- commented Dec 21, 2017

@dunglas this is good now :).

@Nek-
Copy link
Contributor Author

Nek- commented Dec 21, 2017

Status: Needs Review

@Nek-
Copy link
Contributor Author

Nek- commented Jan 8, 2018

ping @dunglas 😄

-----

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

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
Copy link
Contributor

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 ?

Copy link
Member

@dunglas 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)

Before: impossible to catch value object hydratation failure
After: catch the new exception
No BC break.
@Nek- Nek- force-pushed the feature/empty-data-for-denormalization branch from 4ae704a to 7cf8514 Compare January 15, 2018 06:43
@Nek-
Copy link
Contributor Author

Nek- commented Jan 15, 2018

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

@Nek- Nek- changed the title [Serializer] empty_data context option for denormalization [Serializer] default_constructor_arguments context option for denormalization Jan 15, 2018
@@ -36,6 +37,7 @@
const GROUPS = 'groups';
const ATTRIBUTES = 'attributes';
const ALLOW_EXTRA_ATTRIBUTES = 'allow_extra_attributes';
const EMPTY_DATA = 'default_constructor_arguments';
Copy link
Contributor

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

Copy link
Contributor Author

@Nek- Nek- Jan 15, 2018

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.
@Nek- Nek- force-pushed the feature/empty-data-for-denormalization branch from 7cf8514 to 7b8b165 Compare January 15, 2018 14:40
Copy link
Contributor

@sroze sroze left a comment

Choose a reason for hiding this comment

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

👌

@Taluu
Copy link
Contributor

Taluu commented Jan 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.

@fabpot
Copy link
Member

fabpot commented Jan 19, 2018

Thank you @Nek-.

@fabpot fabpot closed this in 2aa54b8 Jan 19, 2018
@Nek- Nek- deleted the feature/empty-data-for-denormalization branch January 19, 2018 22:41
@fabpot fabpot mentioned this pull request May 7, 2018
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jun 27, 2018
…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
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.

8 participants