Skip to content

Commit b558912

Browse files
nicolas-grekasnicolas-grekas
committed
bug symfony#52578 [Serializer] Fix denormalize constructor arguments (mtarld)
This PR was merged into the 5.4 branch. Discussion ---------- [Serializer] Fix denormalize constructor arguments | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix symfony#52499, Fix symfony#52422 | License | MIT Since this PR: symfony#51907, objects with partial constructor parameters were wrongly instantiated. This PR fixes that issue by delegating the properties values assignment, by unsetting normalized data only when the constructor has been called properly. This might correct symfony#50759 as well. Commits ------- 8f7c7ae [Serializer] Fix denormalize constructor arguments
2 parents abff175 + 8f7c7ae commit b558912

File tree

5 files changed

+61
-15
lines changed

5 files changed

+61
-15
lines changed

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

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -350,6 +350,7 @@ protected function instantiateObject(array &$data, string $class, array &$contex
350350
$constructorParameters = $constructor->getParameters();
351351
$missingConstructorArguments = [];
352352
$params = [];
353+
$unsetKeys = [];
353354
foreach ($constructorParameters as $constructorParameter) {
354355
$paramName = $constructorParameter->name;
355356
$key = $this->nameConverter ? $this->nameConverter->normalize($paramName, $class, $format, $context) : $paramName;
@@ -368,18 +369,17 @@ protected function instantiateObject(array &$data, string $class, array &$contex
368369
}
369370

370371
$params = array_merge($params, $variadicParameters);
371-
unset($data[$key]);
372+
$unsetKeys[] = $key;
372373
}
373374
} elseif ($allowed && !$ignored && (isset($data[$key]) || \array_key_exists($key, $data))) {
374375
$parameterData = $data[$key];
375376
if (null === $parameterData && $constructorParameter->allowsNull()) {
376377
$params[] = null;
377-
// Don't run set for a parameter passed to the constructor
378-
unset($data[$key]);
378+
$unsetKeys[] = $key;
379+
379380
continue;
380381
}
381382

382-
// Don't run set for a parameter passed to the constructor
383383
try {
384384
$params[] = $this->denormalizeParameter($reflectionClass, $constructorParameter, $paramName, $parameterData, $context, $format);
385385
} catch (NotNormalizableValueException $exception) {
@@ -390,7 +390,8 @@ protected function instantiateObject(array &$data, string $class, array &$contex
390390
$context['not_normalizable_value_exceptions'][] = $exception;
391391
$params[] = $parameterData;
392392
}
393-
unset($data[$key]);
393+
394+
$unsetKeys[] = $key;
394395
} elseif (\array_key_exists($key, $context[static::DEFAULT_CONSTRUCTOR_ARGUMENTS][$class] ?? [])) {
395396
$params[] = $context[static::DEFAULT_CONSTRUCTOR_ARGUMENTS][$class][$key];
396397
} elseif (\array_key_exists($key, $this->defaultContext[self::DEFAULT_CONSTRUCTOR_ARGUMENTS][$class] ?? [])) {
@@ -421,11 +422,25 @@ protected function instantiateObject(array &$data, string $class, array &$contex
421422
}
422423

423424
if (!$constructor->isConstructor()) {
424-
return $constructor->invokeArgs(null, $params);
425+
$instance = $constructor->invokeArgs(null, $params);
426+
427+
// do not set a parameter that has been set in the constructor
428+
foreach ($unsetKeys as $key) {
429+
unset($data[$key]);
430+
}
431+
432+
return $instance;
425433
}
426434

427435
try {
428-
return $reflectionClass->newInstanceArgs($params);
436+
$instance = $reflectionClass->newInstanceArgs($params);
437+
438+
// do not set a parameter that has been set in the constructor
439+
foreach ($unsetKeys as $key) {
440+
unset($data[$key]);
441+
}
442+
443+
return $instance;
429444
} catch (\TypeError $e) {
430445
if (!isset($context['not_normalizable_value_exceptions'])) {
431446
throw $e;

‎src/Symfony/Component/Serializer/Serializer.php‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,8 +228,20 @@ public function denormalize($data, string $type, string $format = null, array $c
228228
$context['not_normalizable_value_exceptions'] = [];
229229
$errors = &$context['not_normalizable_value_exceptions'];
230230
$denormalized = $normalizer->denormalize($data, $type, $format, $context);
231+
231232
if ($errors) {
232-
throw new PartialDenormalizationException($denormalized, $errors);
233+
// merge errors so that one path has only one error
234+
$uniqueErrors = [];
235+
foreach ($errors as $error) {
236+
if (null === $error->getPath()) {
237+
$uniqueErrors[] = $error;
238+
continue;
239+
}
240+
241+
$uniqueErrors[$error->getPath()] = $uniqueErrors[$error->getPath()] ?? $error;
242+
}
243+
244+
throw new PartialDenormalizationException($denormalized, array_values($uniqueErrors));
233245
}
234246

235247
return $denormalized;

‎src/Symfony/Component/Serializer/Tests/Fixtures/Php74Full.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ public function __construct($constructorArgument)
4646

4747
final class Php74FullWithTypedConstructor
4848
{
49-
public function __construct(float $something)
49+
public function __construct(float $something, bool $somethingElse)
5050
{
5151
}
5252
}

‎src/Symfony/Component/Serializer/Tests/Normalizer/Features/ConstructorArgumentsTestTrait.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,23 +58,23 @@ public function testMetadataAwareNameConvertorWithNotSerializedConstructorParame
5858
public function testConstructorWithMissingData()
5959
{
6060
$data = [
61-
'foo' => 10,
61+
'bar' => 10,
6262
];
6363

6464
$normalizer = $this->getDenormalizerForConstructArguments();
6565
try {
6666
$normalizer->denormalize($data, ConstructorArgumentsObject::class);
6767
self::fail(sprintf('Failed asserting that exception of type "%s" is thrown.', MissingConstructorArgumentsException::class));
6868
} catch (MissingConstructorArgumentsException $e) {
69-
self::assertSame(sprintf('Cannot create an instance of "%s" from serialized data because its constructor requires the following parameters to be present : "$bar", "$baz".', ConstructorArgumentsObject::class), $e->getMessage());
70-
self::assertSame(['bar', 'baz'], $e->getMissingConstructorArguments());
69+
self::assertSame(sprintf('Cannot create an instance of "%s" from serialized data because its constructor requires the following parameters to be present : "$foo", "$baz".', ConstructorArgumentsObject::class), $e->getMessage());
70+
self::assertSame(['foo', 'baz'], $e->getMissingConstructorArguments());
7171
}
7272
}
7373

7474
public function testExceptionsAreCollectedForConstructorWithMissingData()
7575
{
7676
$data = [
77-
'foo' => 10,
77+
'bar' => 10,
7878
];
7979

8080
$exceptions = [];
@@ -85,7 +85,7 @@ public function testExceptionsAreCollectedForConstructorWithMissingData()
8585
]);
8686

8787
self::assertCount(2, $exceptions);
88-
self::assertSame('Failed to create object because the class misses the "bar" property.', $exceptions[0]->getMessage());
88+
self::assertSame('Failed to create object because the class misses the "foo" property.', $exceptions[0]->getMessage());
8989
self::assertSame('Failed to create object because the class misses the "baz" property.', $exceptions[1]->getMessage());
9090
}
9191
}

‎src/Symfony/Component/Serializer/Tests/SerializerTest.php‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -868,7 +868,8 @@ public function testCollectDenormalizationErrors(?ClassMetadataFactory $classMet
868868
],
869869
"php74FullWithConstructor": {},
870870
"php74FullWithTypedConstructor": {
871-
"something": "not a float"
871+
"something": "not a float",
872+
"somethingElse": "not a bool"
872873
},
873874
"dummyMessage": {
874875
},
@@ -1032,6 +1033,24 @@ public function testCollectDenormalizationErrors(?ClassMetadataFactory $classMet
10321033
'useMessageForUser' => false,
10331034
'message' => 'The type of the "something" attribute for class "Symfony\Component\Serializer\Tests\Fixtures\Php74FullWithTypedConstructor" must be one of "float" ("string" given).',
10341035
],
1036+
[
1037+
'currentType' => 'string',
1038+
'expectedTypes' => [
1039+
'float',
1040+
],
1041+
'path' => 'php74FullWithTypedConstructor.something',
1042+
'useMessageForUser' => false,
1043+
'message' => 'The type of the "something" attribute for class "Symfony\Component\Serializer\Tests\Fixtures\Php74FullWithTypedConstructor" must be one of "float" ("string" given).',
1044+
],
1045+
[
1046+
'currentType' => 'string',
1047+
'expectedTypes' => [
1048+
'bool',
1049+
],
1050+
'path' => 'php74FullWithTypedConstructor.somethingElse',
1051+
'useMessageForUser' => false,
1052+
'message' => 'The type of the "somethingElse" attribute for class "Symfony\Component\Serializer\Tests\Fixtures\Php74FullWithTypedConstructor" must be one of "bool" ("string" given).',
1053+
],
10351054
$classMetadataFactory ?
10361055
[
10371056
'currentType' => 'null',

0 commit comments

Comments
 (0)