[Serializer] Improve collecting errors on denormalization and promoted property constructors #45112
Conversation
|
After some crazy rollercoaster with git branches I think I managed to make it right this time. |
|
Hey! I think @Th3Mouk has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
|
This PR is based on #44782 which I messed up a bit. I fixed all checks but not sure why psalm is broken. Any feedback will be appreciated! |
barell
force-pushed
the
collect-denormalizaton-errors-in-constructor-improvements-5.4-fixed
branch
from
1cb0f8b to
ca2a646
Compare
|
@dunglas @nicolas-grekas any feedback on this PR will be appreciated. These fixes would really help simplify a lot of things where we use Symfony Serializer 🙏 |
| $expectedTypes = ['unknown']; | ||
|
|
||
| if ($constructorParameter->hasType()) { | ||
| if (\PHP_VERSION_ID >= 80000) { |
There was a problem hiding this comment.
This can be removed as Symfony will now require at least PHP8.1
| @@ -1050,6 +1051,141 @@ public function testCollectDenormalizationErrorsWithConstructor() | |||
|
|
|||
| $this->assertSame($expected, $exceptionsAsArray); | |||
| } | |||
|
|
|||
| /** @requires PHP 8.0 */ | |||
There was a problem hiding this comment.
You can remove it as well (same for others)
| } | ||
|
|
||
| /** @requires PHP 8.0 */ | ||
| public function testCollectDenormalizationErrorsWithConstructorPartiallyMissingParameters() |
There was a problem hiding this comment.
That test and the previous one (testCollectDenormalizationErrorsWithConstructorMissingParameters) are kind of testing the same stuff, WDYT?
| if ($constructorParameter->getType() instanceof \ReflectionUnionType) { | ||
| $expectedTypes = array_map(function ($type) { return $type->getName(); }, $constructorParameter->getType()->getTypes()); | ||
| } else { | ||
| $expectedTypes = [$constructorParameter->getType()->getName()]; | ||
| } |
There was a problem hiding this comment.
In PHP8.1, there is a new "intersection" type. And the ReflectionIntersectionType do not have any getName method.
Therefore IMHO, the easiest way to go is to rely on the fact that the ReflectionPropertyType is stringable.
So we can do something like that:
if ($constructorParameter->getType() instanceof \ReflectionUnionType) {
$expectedTypes = array_map(function (\ReflectionType $t): string { return (string) $t; }, $constructorParameter->getType()->getTypes());
} else {
$expectedTypes = [(string) $constructorParameter->getType()];
}It'll handle every kind of ReflectionPropertyType and only split the expected type array whenever it's an union type.
|
I'm closing because this stalled. Also because closing my encourage taking over? @mtarld maybe? :) |
There are few improvements/fixes: