[PropertyInfo] Support multiple builtinType (union types) #38987

Korbeil · 2020-11-04T12:21:04Z

Q	A
Branch?	5.x
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	#38093
License	MIT
Doc PR	N/A

First step for #38093, before parsing union types in phpDoc, we need to handle union types correctly in the Type class.

I changed the $builtinType type in the Type construct in order to handle multiple builtin types.

UPGRADE-5.3.md

src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php

src/Symfony/Component/PropertyInfo/Type.php

stof · 2020-11-04T16:20:46Z

I don't think that's the right representation of a union type. Currently, an object type is represented as new Type('object', $nullable, MyClass::class)

So to support union types, I think we would need something dealing with an array of Type objects instead of having an array only in the buiiltinType argument (which does not support unions for object types).
your proposal supports string|int but not A|B

src/Symfony/Component/PropertyInfo/Type.php

Korbeil · 2020-11-04T16:33:35Z

I don't think that's the right representation of a union type. Currently, an object type is represented as new Type('object', $nullable, MyClass::class)

So to support union types, I think we would need something dealing with an array of Type objects instead of having an array only in the buiiltinType argument (which does not support unions for object types).
your proposal supports string|int but not A|B

It's actually true, but I don't think we should have multiple objects to describe that, it should be stored in a single object. It also highlight another issue that I would like to solve: use a separate parameter for the management of nullability when it should be included in builtin types from the beginning.

Can't we try to think on new object to manage this ? Something like class UnionType which will extends our actual Type ?

My ideal implementation would be something like this: https://gist.github.com/Korbeil/3cfaa05878657f4150bdb0ce0fb0b0e6

stof · 2020-11-04T17:17:36Z

It's actually true, but I don't think we should have multiple objects to describe that, it should be stored in a single object. It also highlight another issue that I would like to solve: use a separate parameter for the management of nullability when it should be included in builtin types from the beginning.

To me, the actual issue is more distinguishing builtin types from class types actually. nullability as separate made sense in PHP 7 where any type would be prefixed by ? to make it nullable.

Korbeil · 2020-11-04T18:30:40Z

To me, the actual issue is more distinguishing builtin types from class types actually. nullability as separate made sense in PHP 7 where any type would be prefixed by ? to make it nullable.

Do you already have some ideas about how to do that ? I actually can't think of a new object representation without some needed compatibility layer to avoid BC.

dunglas · 2020-11-04T19:04:31Z

I fear we'll not be able to prevent the compatibility layer. I like @stof proposal of returning an array of Type.

stof · 2020-11-04T19:10:44Z

We should probably return some kind of object wrapping an array of types, rather than a raw array of types. Otherwise, we'll face an issue again if/when we need to add support for intersection types (which also deal with a list of types, but not the same than for union types)

stof · 2020-11-04T19:11:54Z

If we want to fit that in the existing structure of the Type object, we might use union as the "builtin type" and have a getter returning the list of types, maybe

src/Symfony/Component/PropertyInfo/Type.php

Korbeil · 2020-11-05T12:35:08Z

I'll close this for now since the issue is not representating union types since PropertyTypeExtractorInterface already support it by returning an array.
The issue here is that subtypes (collection key & value) are an instance of Type which doesn't allow union types. For example: A|B|C is already supported, but array<A|B|C> isn't. I'll try to come with a better solution 😉 Thanks everyone for your comments and reviews.

Korbeil requested a review from dunglas as a code owner November 4, 2020 12:21

carsonbot added Status: Needs Review Feature Deprecation labels Nov 4, 2020

Korbeil force-pushed the feature/property-info-union-type branch from 5e52c97 to 16eee13 Compare November 4, 2020 12:21

Korbeil mentioned this pull request Nov 4, 2020

[PropertyInfo] Support multiple types (union types) for collection values #38093

Closed

Korbeil force-pushed the feature/property-info-union-type branch 3 times, most recently from 0d3648c to 1487518 Compare November 4, 2020 15:26