Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Serializer] Integrate the PropertyInfo Component (recursive denormalization and hardening)#17660
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
{ should be on next line.
| * | ||
| * @throws UnexpectedValueException | ||
| * @throws LogicException | ||
| */ |
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 this method intended to be overridden by subclasses?
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.
Yes (by ApiBundle especially).
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 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)
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.
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.
…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/aCurrently, 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@blazareckiCommits-------e70fdbc [PropertyAccess] Throw an InvalidArgumentException when the type do not match
fabpot commentedFeb 26, 2016
#17738 has been merged now. |
dunglas commentedFeb 29, 2016
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 commentedMar 6, 2016
ping @symfony/deciders |
| /** | ||
| * @var PropertyTypeExtractorInterface|null | ||
| */ |
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.
The docblock is not needed (as the property is private, IDEs should be able to infer the type from the constructor).
dunglas commentedMar 25, 2016
All comments of@xabbuh fixed now. |
dunglas commentedMar 31, 2016
Tests should be green now. |
dunglas commentedApr 1, 2016
AppVeyor errors not related. |
| return$data; | ||
| } | ||
| } | ||
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.
The type of the "%s" attribute for class "%s" must be one of "%s" ("%s" given).?
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.
Done
dunglas commentedApr 18, 2016
ping @symfony/deciders |
| $this->assertEquals($expected,$result); | ||
| } | ||
| publicfunctiontestDernomalizeRecursive() |
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.
typo in name (misplacedr)
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/aTransform `\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
8924b23 to0c10b4eComparedunglas commentedApr 18, 2016
Failure not related. |
nicolas-grekas commentedApr 19, 2016
👍 |
Recursive denormalization handling and hardening.
b107296 to5194482Compare…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 | todoIntegrates the PropertyInfo Component in order to:* denormalize a graph of objects recursively (see tests)* harden the hydratation logicThe hardening part is interesting. Considering the following example:```phpclass 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 tohttps://github.com/dunglas/php-to-json-schema for protecting an API)./cc@mihai-stancuCommits-------5194482 [Serializer] Integrate the PropertyInfo Component6b464b0 Recursive denormalize using PropertyInfo
dunglas commentedApr 19, 2016
Thanks for working on this@mihai-stancu! |
mihai-stancu commentedApr 19, 2016
@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 integrationDocs forsymfony/symfony#17660.Commits-------8e2deb0 [Serializer] Docs for the PropertyInfo integration
Integrates the PropertyInfo Component in order to:
The hardening part is interesting. Considering the following example:
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
UnexpectedValueExcptionis 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 tohttps://github.com/dunglas/php-to-json-schema for protecting an API).
/cc@mihai-stancu