[Serializer] Fix TemplateType handling in AbstractObjectNormalizer
#58033
Conversation
d0a7bf5 to
2ffa135
Compare
|
/cc @mtarld |
There was a problem hiding this comment.
the file should be renamed back to AbstractObjectNormalizerTest
|
According to #58031 (comment) this used to work in 7.0. However if I rebase your changes on the |
2ffa135 to
8eb5dc4
Compare
It worked because in the example in the issue there is a custom denormalizer which denormalizes the input before passing it to the object denormalizer. I'm aware that it is not how you should call denormalizer, but on the other hand, it did work in previous versions. The same would be $normalizer->denormalize(['value' => new DummyGenericsValue()], DummyGenericsValueWrapper::class)and this does work on top of 7.0. I did add a test for this case too but I'm not sure if this type of misuse should be supported? Or is my assumption about "you should only pass arrays/scalars to |
| @@ -665,6 +665,8 @@ private function validateAndDenormalize(Type $type, string $currentClass, string | |||
| if ($t instanceof CollectionType) { | |||
| $collectionKeyType = $t->getCollectionKeyType(); | |||
| $collectionValueType = $t->getCollectionValueType(); | |||
| } elseif ($t instanceof Type\TemplateType) { | |||
There was a problem hiding this comment.
I think we can add a proper use instead to be consistent
| @@ -665,6 +665,8 @@ private function validateAndDenormalize(Type $type, string $currentClass, string | |||
| if ($t instanceof CollectionType) { | |||
| $collectionKeyType = $t->getCollectionKeyType(); | |||
| $collectionValueType = $t->getCollectionValueType(); | |||
| } elseif ($t instanceof Type\TemplateType) { | |||
There was a problem hiding this comment.
That condition must IMO go under the first if of the foreach (L663).
Otherwise, a template of collection won't be handled properly.
There was a problem hiding this comment.
It seems to handle collections (i.e. T[]) properly this has to be done inside the if ($t instanceof CollectionType) block too
if ($collectionValueType instanceof TemplateType) {
$collectionValueType = $collectionValueType->getBound();
}(maybe same should be for $collectionValueKey too)
I wonder would it make more sense to make TemplateType::getBaseType() return $this->getBound() instead of throwing – then we didn't need any of this here. I'm not too familiar with the new TypeInfo component so this could be a silly proposal.
There was a problem hiding this comment.
I wonder would it make more sense to make TemplateType::getBaseType() return $this->getBound() instead of throwing – then we didn't need any of this here. I'm not too familiar with the new TypeInfo component so this could be a silly proposal.
I indeed think that the bound type should be considered as the base type. Could you please send a PR in that way?
8eb5dc4 to
bc29141
Compare
|
Closing in favor of #58259 |
…` (valtzu) This PR was merged into the 7.1 branch. Discussion ---------- [TypeInfo] Return bound type as base type in `TemplateType` | Q | A | ------------- | --- | Branch? | 7.1 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #58031 | License | MIT This supersedes #58033 as simpler approach. Commits ------- f897a7b Fix `TemplateType` handling in `AbstractObjectNormalizer`
TypeInfo component introduced in 7.1 causes
AbstractObjectNormalizerto throw an error when you have/** @var T */in your DTO – see the related issue for details.