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] `default_constructor_arguments` context option for denormalization #25493

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@Nek-
Contributor

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 from Feature/empty data for denormalization to [Serializer] `empty_data` context option for denormalization Dec 14, 2017

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Dec 18, 2017

namespace Symfony\Component\Serializer\Exception;
/**
* IncompleteInputDataException.

This comment has been minimized.

@dunglas

dunglas Dec 19, 2017

Member

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

This comment has been minimized.

@Nek-

Nek- Dec 20, 2017

Contributor

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])) {

This comment has been minimized.

@dunglas

dunglas Dec 19, 2017

Member

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

This comment has been minimized.

@Nek-

Nek- Dec 20, 2017

Contributor

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

'foo' => 10,
);
$this->expectException(\Symfony\Component\Serializer\Exception\IncompleteInputDataException::class);

This comment has been minimized.

@dunglas

dunglas Dec 19, 2017

Member

Prefer using @expectedException and @expectedExceptionMessage instead.

$normalizer = new ObjectNormalizer();
$serializer = new Serializer(array($normalizer));
$normalizer->setSerializer($serializer);

This comment has been minimized.

@dunglas

dunglas Dec 19, 2017

Member

This line is useless, Serializer will inject itself.

$normalizer = new ObjectNormalizer();
$serializer = new Serializer(array($normalizer));
$normalizer->setSerializer($serializer);

This comment has been minimized.

@dunglas

dunglas Dec 19, 2017

Member

Same.

@dunglas

This comment has been minimized.

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-

This comment has been minimized.

Contributor

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])) {

This comment has been minimized.

@dunglas

dunglas Dec 20, 2017

Member

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

This comment has been minimized.

@Nek-

Nek- Dec 21, 2017

Contributor

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

@Nek-

This comment has been minimized.

Contributor

Nek- commented Dec 21, 2017

@dunglas this is good now :).

@Nek-

This comment has been minimized.

Contributor

Nek- commented Dec 21, 2017

Status: Needs Review

@Nek-

This comment has been minimized.

Contributor

Nek- commented Jan 8, 2018

ping @dunglas 😄

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

This comment has been minimized.

@sroze

sroze Jan 8, 2018

Member

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

This comment has been minimized.

@sroze

sroze Jan 8, 2018

Member

Same reflection... what about default_constructor_arguments ?

@dunglas

(when wording issues in the changelog will be resolved)

Add new MissingConstructorArgumentsException
Before: impossible to catch value object hydratation failure
After: catch the new exception
No BC break.
@Nek-

This comment has been minimized.

Contributor

Nek- commented Jan 15, 2018

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

@Nek- Nek- changed the title from [Serializer] `empty_data` context option for denormalization to [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';

This comment has been minimized.

@sroze

sroze Jan 15, 2018

Member

Might as well rename the EMPTY_DATA constant then :)

This comment has been minimized.

@Nek-

Nek- Jan 15, 2018

Contributor

Arg! Fixed.

Add new feature to serializer: default_constructor_arguments
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.
@sroze

sroze approved these changes Jan 15, 2018

👌

@Taluu

This comment has been minimized.

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.

@Taluu

Taluu approved these changes Jan 16, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Jan 19, 2018

Thank you @Nek-.

@fabpot fabpot closed this in 2aa54b8 Jan 19, 2018

@Nek- Nek- deleted the Nek-:feature/empty-data-for-denormalization branch Jan 19, 2018

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jun 27, 2018

minor #8914 Add new serializer empty_data feature doc (Nek-, javiereg…
…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