[Serializer] Denormalize with typehinting. #14844
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Example: In the above example the denormalizer will crash because iti will attempt to call This PR will try to denormalize |
|
@mihai-stancu Can you add a test that would have failed without your changes? |
|
It's a complex topic:
During the development of API Platform I've created a component able to extract informations form various sources (and it is easy to extend). It also supports collections and everything I mentioned previously: https://github.com/dunglas/php-property-info Then I've integrated it with the Symfony Serializer in DunglasApiBundle:
Dealing with types is indeed a common need. JMSSerializer and DunglasApiBundle have that capability. In fact any advanced serializer will require such thing. Instead of that patch, what do you @symfony/deciders think of integrating my component (or moving it under the Symfony umbrella if it's necessary) and backporting the integration code from DunglasApiBundle directly in the Serializer component? |
|
@dunglas I've just had a look at the library, and that makes a lot of sense. One of my question is if it is useful as is for a large number of use cases? The answer would help defining how to proceed next. |
|
@xabbuh sure, can-do. |
|
@dunglas JMSSerializer and your bundle bring many more features to the table (as well as complexity). My patch is indeed geared towards smaller simpler use cases. |
|
@xabbuh test added. |
|
IMO we must decide first if we want an advanced type guessing system such as https://github.com/dunglas/php-property-info or not in the core cc @fabpot |
|
It makes a lot of sense to have a the current |
|
FYI, I've sent an email to @dunglas to confirm that I'm fine with integrating the library into Symfony as a new component. |
|
great! |
|
great news |
|
Awesome! |
fabpot
added a commit
that referenced
this pull request
Sep 26, 2015
This PR was squashed before being merged into the 2.8 branch (closes #15858). Discussion ---------- [PropertyInfo] Import the component | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | symfony/symfony-docs#5717 As discussed with @fabpot (see #14844), this PR moves [dunglas/php-property-info](https://github.com/dunglas/php-property-info) under the Symfony umbrella. Rationale behind this new component (extracted from README.md): PHP doesn't support explicit type definition. This is annoying, especially when doing meta programming. Various libraries including but not limited to Doctrine ORM and the Symfony Validator provide their own type managing system. This library extracts various information including the type and documentation from PHP class property from metadata of popular sources: * Setter method with type hint * PHPDoc DocBlock * Doctrine ORM mapping (annotation, XML, YML or custom format) * PHP 7 scalar typehint and return type * Serializer metadata **Usage:** ```php <?php // Use Composer autoload require 'vendor/autoload.php'; use Doctrine\ORM\EntityManager; use Doctrine\ORM\Tools\Setup; use Doctrine\ORM\Mapping\Column; use Doctrine\ORM\Mapping\Entity; use Doctrine\ORM\Mapping\Id; use Symfony\Component\PropertyInfo\Extractors\DoctrineExtractor; use Symfony\Component\PropertyInfo\Extractors\PhpDocExtractor; use Symfony\Component\PropertyInfo\Extractors\ReflectionExtractor; use Symfony\Component\PropertyInfo\PropertyInfo; /** * @entity */ class MyTestClass { /** * @id * @column(type="integer") */ public $id; /** * This is a date (short description). * * With a long description. * * @var \DateTime */ public $foo; private $bar; public function setBar(\SplFileInfo $bar) { $this->bar = $bar; } } // Doctrine initialization (necessary only to use the Doctrine Extractor) $config = Setup::createAnnotationMetadataConfiguration([__DIR__], true); $entityManager = EntityManager::create([ 'driver' => 'pdo_sqlite', // ... ], $config); $doctrineExtractor = new DoctrineExtractor($entityManager->getMetadataFactory()); $phpDocExtractor = new PhpDocExtractor(); $reflectionExtractor = new ReflectionExtractor(); $propertyInfo = new PropertyInfo( array($reflectionExtractor), array($doctrineExtractor, $phpDocExtractor, $reflectionExtractor), array($phpDocExtractor), array($reflectionExtractor) ); var_dump($propertyInfo->getProperties('MyTestClass')); var_dump($propertyInfo->getTypes('MyTestClass', 'foo')); var_dump($propertyInfo->getTypes('MyTestClass', 'id')); var_dump($propertyInfo->getTypes('MyTestClass', 'bar')); var_dump($propertyInfo->isReadable('MyTestClass', 'id')); var_dump($propertyInfo->isReadable('MyTestClass', 'bar')); var_dump($propertyInfo->isWritable('MyTestClass', 'foo')); var_dump($propertyInfo->isWritable('MyTestClass', 'bar')); var_dump($propertyInfo->getShortDescription('MyTestClass', 'foo')); var_dump($propertyInfo->getLongDescription('MyTestClass', 'foo')); ``` Output: ``` array(3) { [0] => string(2) "id" [1] => string(3) "foo" [2] => string(3) "Bar" } array(1) { [0] => class Symfony\Component\PropertyInfo\Type#36 (6) { private $builtinType => string(6) "object" private $nullable => bool(false) private $class => string(8) "DateTime" private $collection => bool(false) private $collectionKeyType => NULL private $collectionValueType => NULL } } array(1) { [0] => class Symfony\Component\PropertyInfo\Type#36 (6) { private $builtinType => string(3) "int" private $nullable => bool(false) private $class => NULL private $collection => bool(false) private $collectionKeyType => NULL private $collectionValueType => NULL } } array(1) { [0] => class Symfony\Component\PropertyInfo\Type#245 (6) { private $builtinType => string(6) "object" private $nullable => bool(false) private $class => string(11) "SplFileInfo" private $collection => bool(false) private $collectionKeyType => NULL private $collectionValueType => NULL } } bool(true) bool(false) bool(true) bool(true) string(35) "This is a date (short description)." string(24) "With a long description." ``` Commits ------- f1eb185 [PropertyInfo] Import the component
This PR was merged into the 2.8 branch. Discussion ---------- Guard minor tweaks | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Various completely minor things, most from suggestions on #14673 Commits ------- 869d5a7 tweaking message related to configuration edge case that we want to be helpful with da4758a Minor tweaks - lowering the required security-http requirement and nulling out a test field
…(WouterJ) This PR was merged into the 3.0-dev branch. Discussion ---------- [3.0][Config] Removed isFresh() related functionality | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- 4cdae98 Removed Resource#isFresh() related functionality
…aint (1ed) This PR was merged into the 2.8 branch. Discussion ---------- [Validator] Add Hungarian translation for the BIC constraint | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- f26425b [Validator] Add Hungarian translation for the BIC constraint
This PR was merged into the 2.8 branch. Discussion ---------- Making GuardTokenInterface extend TokenInterface | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #15884 | License | MIT | Doc PR | n/a See #15884 Commits ------- 7f04fbb Making GuardTokenInterface extend TokenInterface
This PR was merged into the 2.8 branch. Discussion ---------- Abstract voter tweaks | Q | A | ------------- | --- | Bug fix? | yes (a little) | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Based on suggestions from stof in #15870, this simplifies the BC and deprecation throwing code. This also adds a BadMethodCallException in case the user doesn't override `isGranted` *or* `voteOnAttribute`, because that's just plain wrong (as is calling `isGranted()` on the parent class directly, since that was formerly abstract). Commits ------- c03f5c2 Massively simplifying the BC and deprecated-throwing code thanks to suggestions by stof in #15870
…(xelaris) This PR was merged into the 2.8 branch. Discussion ---------- [2.8][WebProfilerBundle] Fix search button click listener | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | This fixes an issue when clicking the sidebar "Search" button **text** instead of the **button**. Then the click event target/srcElement is the *span* child-element, instead of the listening *a* element, which causes errors in the listener, since it expects the listening element. In consequence of that the search form isn't shown. To fix this, the same technique is used, as for the navigation tabs. Traversing the DOM up to the expected *a* element. Commits ------- f9ddddb [WebProfilerBundle] Fix search button click listener
…as set the response (weaverryan) This PR was merged into the 2.8 branch. Discussion ---------- Updating behavior to not continue after an authenticator has set the response | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | https://github.com/symfony/symfony/pull/14673/files#r40492765 | License | MIT | Doc PR | n/a This mirrors the behavior in core: *if* a listener sets a response (on success or failure), then the other listeners are not called. But if a response is *not* set (which is sometimes the case for success, like in BasicAuthenticationListener), then the other listeners are called, and can even fail. It's all a bit of an edge-case, as only one authenticator (like authentication listener) would normally be doing any work on a request, but I think matching the other listeners (since I'm not aware of anyone having issues with its behavior) is best. Commits ------- 5fa2684 Making all "debug" messages use the debug router f403444 Updating behavior to not continue after an authenticator has set the response
* 2.7: Detect Mintty for color support on Windows Detect Mintty for color support on Windows Add a group for tests of the finder against the FTP server Fix license headers Forbid serializing a Crawler Fix phpdoc block of NativeSessionStorage class Added exception when setAutoInitialize is called when locked [FrameworkBundle] Advanced search templates of bundles [Security] Allow user providers to be defined in many files Use random_bytes function if it is available for random number generation
* 2.8: (28 commits) Detect Mintty for color support on Windows Detect Mintty for color support on Windows [WebProfilerBundle] Fix search button click listener [Form][Type Date/Time] added choice_translation_domain option. Massively simplifying the BC and deprecated-throwing code thanks to suggestions by stof in #15870 Making all "debug" messages use the debug router Making GuardTokenInterface extend TokenInterface Updating behavior to not continue after an authenticator has set the response Add a group for tests of the finder against the FTP server Fix trigger_error calls Fix legacy security tests tweaking message related to configuration edge case that we want to be helpful with Minor tweaks - lowering the required security-http requirement and nulling out a test field Fix license headers Fix license headers Fix license headers Ensure the ClockMock is loaded before using it in the testsuite Allow serializer 3.0 in the PropertyInfo component Add the replace rules for the security-guard component Forbid serializing a Crawler ...
This PR was merged into the 2.8 branch. Discussion ---------- Add a few additional tests for the Crawler | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a While looking at the update of the Crawler in #16075, I spotted a few mistakes. But these were cases not covered by the testsuite. This is adding tests for these cases in the 2.8 branch (they could be added in 2.3 eventually though). Commits ------- 528d3bd Add a few additional tests for the Crawler
This PR was merged into the 3.0-dev branch. Discussion ---------- fix validator test dependency | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Fix test https://travis-ci.org/symfony/symfony/jobs/83308337#L3653 as property access is required for expression constraint. Commits ------- 3b7cb33 fix validator test dependency
This PR was merged into the 3.0-dev branch. Discussion ---------- Fix the crawler refactoring | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a This fixes a few mistakes I spotted in #16075 for the DomCrawler component. Regression tests are added separately in #16093 to be included in older branches too. Commits ------- d128735 Fix the crawler refactoring
…glas) This PR was squashed before being merged into the 2.8 branch (closes #15613). Discussion ---------- [DependencyInjection] Add autowiring capabilities | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | not yet This PR adds autowiring capabilities to the Dependency Injection component. It eases service registration by letting the component guessing dependencies to inject and even (under certain conditions) registering them using typehints of the constructor parameters. The following usages are supported: # Automatic dependency registration ```php class Foo { } class Bar { public function __construct(Foo $f) { } } ``` ```yaml services: bar: class: Bar autowire: true ``` It will register `Foo` as a private service (`autowired.foo`) and injects it as the first argument of the `bar` constructor. This method works only for typehints corresponding to instantiable classes (interfaces and abstract classes are not supported). # Autocompletion of definition arguments ```php interface A { } interface B extends A { } class Foo implements B { } class Bar { } class Baz extends Bar { } class LesTilleuls { public function __construct(A $a, Bar $bar) { } } ``` ```yaml services: foo: class: Foo baz: class: Baz les_tilleuls: class: LesTilleuls autowire: true ``` The autowiring system will find types of all services and completes constructor arguments of the `les_tilleuls` service definition using typehints. It works only if there is one service registered for a given type (if there are several services available for the same type and no explicit type definition, a `RuntimeException` is thrown). # Explicit type definition ```php interface A { } class A1 implements A { } class A2 implements A { } class B { public function __construct(A $a) { } } ``` ```yaml services: a1: class: A1 types: [ A ] a2: class: A2 # Will be autowired with A1 class b: class: B autowire: true # Not autowired class another_b: class: B arguments: [ @a2 ] autowire: true ``` When a service is explicitly associated with a type, it is always used to fill a definition depending of this type, even if several services have this type. If several services are associated with the same type, the last definition takes the priority. Of course explicit definitions are still supported. YAML, XML and PHP loaders have been updated to supports the new `type` parameter. Commits ------- aee5731 [DependencyInjection] Add autowiring capabilities
Passing implementations of the pre 2.5 validator API to the constructors of the `ValidatorExtension` and the `ValidationListener` must trigger a deprecation.
This PR was merged into the 2.8 branch. Discussion ---------- [Form] add missing deprecation triggers | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #15767 | License | MIT | Doc PR | Passing implementations of the pre 2.5 validator API to the constructors of the `ValidatorExtension` and the `ValidationListener` must trigger a deprecation. Commits ------- bcd3946 [Form] add missing deprecation triggers
…ns (nicolas-grekas) This PR was merged into the 2.8 branch. Discussion ---------- [DomCrawler] Deprecated using /_root/ in XPath expressions | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- 6042e86 [DomCrawler] Deprecated using /_root/ in XPath expressions
This PR was merged into the 2.8 branch. Discussion ---------- Simplify AbstractVoter | Q | A | ------------- | --- | Bug fix? | no | New feature? | no, just simplification | BC breaks? | no, because 2.8 is not yet released | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- 93de659 Simplify AbstractVoter
This PR was merged into the 2.8 branch. Discussion ---------- Pass missing request template variables Some render calls were missing the `request` variable, while it is used in the `layout.html.twig` template. | Q | A | --- | --- | Fixed tickets | - | License | MIT Commits ------- 7f1b2c2 Pass missing request template variables
…quest::get (Tobion) This PR was merged into the 3.0-dev branch. Discussion ---------- [HttpFoundation] change precedence of parameters in Request::get | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Allowing the request attributes to be overwritten via GET parameters is risky and made #8966 even worse. It is even more risky because it skips the requirements checks as configured in routing. So people that set requirements for routing placeholders like `\d+` or `html|json` can be sure it is validated when using the routing variables. But if developers use `$request->get()` to retrieve them, anybody from outside can set any value for those. Commits ------- e8d6764 [HttpFoundation] change precedence of parameters in Request::get
…vanlaak) This PR was merged into the 2.8 branch. Discussion ---------- Include working directory in ProcessFailedException ... because quite often the Exception is a result of the `www-data` user not having the appropriate rights at that working path. Maybe @schmittjoh can confirm this? | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Commits ------- dbaefb4 Include working directory in ProcessFailedException
* 2.7: [Security][bugfix] "Remember me" cookie cleared on logout with custom "secure"/"httponly" config options [1] [ci] Use current PHP_BINARY when running ./phpunit Fixed typos [UPGRADE-3.0] fix bullet indentation Fix PropertyAccessor modifying array in object when array key does not exist [Security] InMemoryUserProvider now concerns whether user's password is changed when refreshing
* 2.8: (21 commits) [Security][bugfix] "Remember me" cookie cleared on logout with custom "secure"/"httponly" config options [1] [ci] Use current PHP_BINARY when running ./phpunit Fixed typos [UPGRADE-3.0] fix bullet indentation Throw exception if tempnam returns false in ProcessPipes [DomCrawler] Deprecated using /_root/ in XPath expressions Pass missing request template variables Simplify AbstractVoter [Form] add missing deprecation triggers Throw exception if tempnam returns false Fix PropertyAccessor modifying array in object when array key does not exist [DependencyInjection] Add autowiring capabilities Fixing typo in variable name Add a few additional tests for the Crawler [Form] remove obsolete deprecation comments Updated the style of the event commands [Debug] Deprecate providing $fileLinkFormat as second argument [Form] minor CS fix Updated PHPDoc of the AbstractVoter class [Security] InMemoryUserProvider now concerns whether user's password is changed when refreshing ...
…stancu/symfony into denormalize_with_typehint
…hint Conflicts: src/Symfony/Component/Serializer/Normalizer/AbstractNormalizer.php src/Symfony/Component/Serializer/Normalizer/GetSetMethodNormalizer.php src/Symfony/Component/Serializer/Tests/Normalizer/GetSetMethodNormalizerTest.php
- Require symfony/property-info - Add PropertyInfoExtractor to AbstractNormalizer constructor - Refactor GetSetMethodNormalizer to use PropertyInfoExtractor - Update tests to provide PropertyInfoExtractor in construct
|
I'll reopen this PR against symfony:2.8 instead. |
dunglas
added a commit
that referenced
this pull request
Apr 19, 2016
…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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[2.7][Serializer] Feature using the type hinted class from the setter (or construct) parameter to denormalize values before calling the setter.