[Serializer] Allow to use easily static constructors #19137
Conversation
| * | ||
| * @return \ReflectionMethod|null | ||
| */ | ||
| protected function getConstructor($class, \ReflectionClass $reflectionClass) |
There was a problem hiding this comment.
should we pass to this method all the arguments passed to instantiateObject ?
There was a problem hiding this comment.
I think that makes sense in case you want to support different factory methods depending on the input.
There was a problem hiding this comment.
indeed, updated :)
|
|
||
| public function testObjectWithStaticConstructor() | ||
| { | ||
| $normalizer = $this->getMockBuilder(ObjectNormalizer::class) |
There was a problem hiding this comment.
I don't understand why we would need too use a mock here. Why not using the ObjectNormalizer directly?
There was a problem hiding this comment.
Sorry, what I mean is create a custom normalizer to be used in the test that properly implements getConstructor().
There was a problem hiding this comment.
A mock allows to test the abstract class but i have no strong opinion about it and a fixture normalizer is fine for me too.
There was a problem hiding this comment.
I would prefer a fixture normalizer too.
| @@ -318,7 +335,11 @@ protected function instantiateObject(array &$data, $class, array &$context, \Ref | |||
| } | |||
| } | |||
|
|
|||
| return $reflectionClass->newInstanceArgs($params); | |||
| if ($constructor->isConstructor()) { | |||
There was a problem hiding this comment.
This is breaking and should also be called if $constructor is null. However that throws an exception if the class has no constructor, so what about calling newInstanceWithoutConstructor then?, being the absolute fallback.
There was a problem hiding this comment.
if $constructor is null, this code wouldn't be reached because of the if statement on line 303.
There was a problem hiding this comment.
Hehe. Totally overlooked indenting.
|
👍 |
1 similar comment
|
👍 |
| return $reflectionClass->newInstanceArgs($params); | ||
| if ($constructor->isConstructor()) { | ||
| return $reflectionClass->newInstanceArgs($params); | ||
| } else { |
There was a problem hiding this comment.
There isn't any sense for this else.
There was a problem hiding this comment.
IMO it's harder to understand this 2 lines without it.
|
Thank you @Ener-Getick. |
…Ener-Getick) This PR was merged into the 3.2-dev branch. Discussion ---------- [Serializer] Allow to use easily static constructors | Q | A | ------------- | --- | Branch? | "master" | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19027 (comment) | License | MIT | Doc PR | - This PR allows to simply use static constructors to instantiate objects with the serializer by extending the default normalizers. Commits ------- 9be6484 [Serializer] Allow to use easily static constructors
This PR allows to simply use static constructors to instantiate objects with the serializer by extending the default normalizers.