[Serializer] Integrate the PropertyInfo Component (recursive denormalization and hardening) #17660

dunglas · 2016-02-02T23:13:29Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#16143, #17193, #14844
License	MIT
Doc PR	todo

Integrates the PropertyInfo Component in order to:

denormalize a graph of objects recursively (see tests)
harden the hydratation logic

The hardening part is interesting. Considering the following example:

class Foo
{
    public function setDate(\DateTimeInterface $date)
    {
    }
}

// initialize $normalizer
$normalizer->denormalize(['date' => 1234], Foo::class);

Previously, a PHP error was thrown because the type passed to the setter (an int) doesn't match the one checked with the typehint. With the PropertyInfo integration, an UnexpectedValueExcption is throw instead.
It's especially interesting for web APIs dealing with JSON documents. For instance in API Platform, previously a 500 error was thrown, but thanks to this fix a 400 HTTP code with a descriptive error message will be returned. (/cc @csarrazi @mRoca @blazarecki, it's an alternative to https://github.com/dunglas/php-to-json-schema for protecting an API).

/cc @mihai-stancu

stloyd · 2016-02-03T07:47:38Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

+     * @throws UnexpectedValueException
+     * @throws LogicException
+     */
+    protected function validateAndDenormalize($currentClass, $attribute, $data, $format, array $context) {


{ should be on next line.

xabbuh · 2016-02-09T10:35:46Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

+     * @throws UnexpectedValueException
+     * @throws LogicException
+     */
+    protected function validateAndDenormalize($currentClass, $attribute, $data, $format, array $context)


Is this method intended to be overridden by subclasses?

Yes (by ApiBundle especially).

it would be better to design an extension point based on composition instead of designing for inheritance. Inheritance-based extension points are always a huge pain to maintain BC when refactoring the logic (see the extraction of name converter from this class for instance)

After double checking, this extension point isn't needed anymore for API Platform. As I've no use case anymore, I changed the visibility of this method to private.

@csarrazi

…n the type do not match (dunglas) This PR was squashed before being merged into the 3.1-dev branch (closes #17738). Discussion ---------- [PropertyAccess] Throw an InvalidArgumentException when the type do not match | Q | A | ------------- | --- | Bug fix? | no (?) | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Currently, when the Property Access Component call a setter with a value not matching its typehint, a `\TypeError` is thrown with PHP 7 and a `PHP Catchable fatal error` with PHP 5. This PR make the component returning an `InvalidArgumentException` with both version. It's a (better) alternative to #17660 (the hardening part) to make the Symfony Serializer (and probably many other pieces of code) more robust when types do not match. /cc @csarrazi @mRoca @blazarecki Commits ------- e70fdbc [PropertyAccess] Throw an InvalidArgumentException when the type do not match

fabpot · 2016-02-26T06:21:59Z

#17738 has been merged now.

dunglas · 2016-02-29T20:21:03Z

I've looked at this more deeply and #17959 and this one complement. They don't cover same cases. Both should be merged. (Will need a rebase).

dunglas · 2016-03-06T17:47:15Z

ping @symfony/deciders

xabbuh · 2016-03-07T12:14:32Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

@@ -24,8 +29,20 @@
    const ENABLE_MAX_DEPTH = 'enable_max_depth';
    const DEPTH_KEY_PATTERN = 'depth_%s::%s';

+    /**
+     * @var PropertyTypeExtractorInterface|null
+     */


The docblock is not needed (as the property is private, IDEs should be able to infer the type from the constructor).

dunglas · 2016-03-25T21:21:26Z

All comments of @xabbuh fixed now.

dunglas · 2016-03-31T19:15:45Z

Tests should be green now.

dunglas · 2016-04-01T10:21:58Z

AppVeyor errors not related.

fabpot · 2016-04-01T10:25:00Z

src/Symfony/Component/Serializer/Normalizer/AbstractObjectNormalizer.php

+            }
+        }
+
+        throw new UnexpectedValueException(sprintf('The type of the attribute "%s" for the class "%s" must be one of "%s" ("%s" given).', $attribute, $currentClass, implode('", "', array_keys($expectedTypes)), gettype($data)));


The type of the "%s" attribute for class "%s" must be one of "%s" ("%s" given).?

dunglas · 2016-04-18T14:11:46Z

ping @symfony/deciders

stof · 2016-04-18T14:17:12Z

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

@@ -498,6 +501,29 @@ public function testMaxDepth()

        $this->assertEquals($expected, $result);
    }
+
+    public function testDernomalizeRecursive()


typo in name (misplaced r)

This PR was squashed before being merged into the 3.1-dev branch (closes #17959). Discussion ---------- [Serializer] Harden the ObjectNormalizer | Q | A | ------------- | --- | Branch | master | Bug fix? | no | New feature? | yes | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Transform `\TypeError`s to catchable serializer exceptions. Follows #17738 and completes #17660. Commits ------- 26a07fb [Serializer] Harden the ObjectNormalizer

- Refactored PR 14844 "Denormalize with typehinting" - Now using PropertyInfo to extract type information - Updated tests - Updated composer.json

dunglas · 2016-04-18T20:54:37Z

Failure not related.

nicolas-grekas · 2016-04-19T13:57:32Z

👍

Recursive denormalization handling and hardening.

@csarrazi

…ursive denormalization and hardening) (mihai-stancu, dunglas) This PR was merged into the 3.1-dev branch. Discussion ---------- [Serializer] Integrate the PropertyInfo Component (recursive denormalization and hardening) | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #16143, #17193, #14844 | License | MIT | Doc PR | todo Integrates the PropertyInfo Component in order to: * denormalize a graph of objects recursively (see tests) * harden the hydratation logic The hardening part is interesting. Considering the following example: ```php class Foo { public function setDate(\DateTimeInterface $date) { } } // initialize $normalizer $normalizer->denormalize(['date' => 1234], Foo::class); ``` Previously, a PHP error was thrown because the type passed to the setter (an int) doesn't match the one checked with the typehint. With the PropertyInfo integration, an `UnexpectedValueExcption` is throw instead. It's especially interesting for web APIs dealing with JSON documents. For instance in API Platform, previously a 500 error was thrown, but thanks to this fix a 400 HTTP code with a descriptive error message will be returned. (/cc @csarrazi @mRoca @blazarecki, it's an alternative to https://github.com/dunglas/php-to-json-schema for protecting an API). /cc @mihai-stancu Commits ------- 5194482 [Serializer] Integrate the PropertyInfo Component 6b464b0 Recursive denormalize using PropertyInfo

dunglas · 2016-04-19T15:19:53Z

Thanks for working on this @mihai-stancu!

mihai-stancu · 2016-04-19T15:25:15Z

@dunglas thank you for seeing it through!

…glas) This PR was submitted for the 3.1 branch but it was merged into the 3.2 branch instead (closes #7042). Discussion ---------- [Serializer] Docs for the PropertyInfo integration Docs for symfony/symfony#17660. Commits ------- 8e2deb0 [Serializer] Docs for the PropertyInfo integration

dunglas added the Serializer label Feb 2, 2016

carsonbot added the Status: Needs Review label Feb 2, 2016

stloyd reviewed Feb 3, 2016
View reviewed changes

dunglas mentioned this pull request Feb 4, 2016

[Question] Validation through json schemas api-platform/core#402

Closed

xabbuh reviewed Feb 9, 2016
View reviewed changes

dunglas mentioned this pull request Feb 9, 2016

[PropertyAccess] Throw an InvalidArgumentException when the type do not match #17738

Closed

mihai-stancu mentioned this pull request Feb 11, 2016

Recursive denormalize with property info #17193

Closed

dunglas mentioned this pull request Feb 29, 2016

[Serializer] Harden the ObjectNormalizer #17959

Closed

javiereguiluz added the Feature label Mar 4, 2016

xabbuh reviewed Mar 7, 2016
View reviewed changes

dunglas added the Ready label Mar 25, 2016

fabpot reviewed Apr 1, 2016
View reviewed changes

stof reviewed Apr 18, 2016
View reviewed changes

Recursive denormalize using PropertyInfo

6b464b0

- Refactored PR 14844 "Denormalize with typehinting" - Now using PropertyInfo to extract type information - Updated tests - Updated composer.json

dunglas force-pushed the serializer/property-info branch from 8924b23 to 0c10b4e Compare April 18, 2016 20:16

[Serializer] Integrate the PropertyInfo Component

5194482

Recursive denormalization handling and hardening.

dunglas force-pushed the serializer/property-info branch from b107296 to 5194482 Compare April 19, 2016 15:00

dunglas merged commit 5194482 into symfony:master Apr 19, 2016

dunglas deleted the serializer/property-info branch April 19, 2016 15:19

This was referenced May 5, 2016

fix #518 - Add an exception to show that the value is not readable. api-platform/core#527

Merged

Refactor denormalization process? api-platform/core#528

Closed

fabpot mentioned this pull request May 13, 2016

Release v3.1.0-BETA1 #18776

Merged

GuilhemN mentioned this pull request Jul 27, 2016

[FrameworkBundle] Inject the PropertyInfo in the ObjectNormalizer #19448

Closed

dunglas mentioned this pull request Sep 17, 2016

Allow GetSetMethodNormalizer in Serializer to be aware of type hinted parameters #19935

Closed

dunglas mentioned this pull request Oct 6, 2016

[Serializer] Docs for the PropertyInfo integration symfony/symfony-docs#7042

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serializer] Integrate the PropertyInfo Component (recursive denormalization and hardening) #17660

[Serializer] Integrate the PropertyInfo Component (recursive denormalization and hardening) #17660

dunglas commented Feb 2, 2016

stloyd Feb 3, 2016

xabbuh Feb 9, 2016

dunglas Feb 9, 2016

stof Apr 18, 2016

dunglas Apr 18, 2016

fabpot commented Feb 26, 2016

dunglas commented Feb 29, 2016

dunglas commented Mar 6, 2016

xabbuh Mar 7, 2016

dunglas commented Mar 25, 2016

dunglas commented Mar 31, 2016

dunglas commented Apr 1, 2016

fabpot Apr 1, 2016

dunglas Apr 18, 2016

dunglas commented Apr 18, 2016

stof Apr 18, 2016

dunglas commented Apr 18, 2016

nicolas-grekas commented Apr 19, 2016

dunglas commented Apr 19, 2016

mihai-stancu commented Apr 19, 2016

[Serializer] Integrate the PropertyInfo Component (recursive denormalization and hardening) #17660

[Serializer] Integrate the PropertyInfo Component (recursive denormalization and hardening) #17660

Conversation

dunglas commented Feb 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Feb 26, 2016

dunglas commented Feb 29, 2016

dunglas commented Mar 6, 2016

Choose a reason for hiding this comment

dunglas commented Mar 25, 2016

dunglas commented Mar 31, 2016

dunglas commented Apr 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Apr 18, 2016

Choose a reason for hiding this comment

dunglas commented Apr 18, 2016

nicolas-grekas commented Apr 19, 2016

dunglas commented Apr 19, 2016

mihai-stancu commented Apr 19, 2016