-
-
Notifications
You must be signed in to change notification settings - Fork 368
[LiveComponent] Fix BC break when using PropertyTypeExtractorInterface::getType()
on a #[LiveProp]
property x
when getter getX
exists
#2922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.x
Are you sure you want to change the base?
Conversation
0f4b08d
to
ee49115
Compare
… an interface) on `LiveProp`
ee49115
to
d071359
Compare
After some investigations, it happens because of When trying to resolve the type of |
This comment was marked as outdated.
This comment was marked as outdated.
LiveProp
PropertyTypeExtractorInterface::getType()
on a #[LiveProp]
property x
when getter getX
exists
if (method_exists($this->propertyTypeExtractor, 'getType') && !$this->typeResolver) { | ||
throw new \LogicException('Symfony TypeInfo is required to use LiveProps. Try running "composer require symfony/type-info".'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not happens since PropertyTypeInfoExtractor::getType()
exists since 7.1, which also ship TypeInfo, but just in case..
$reflectionType = $property->getType(); | ||
if ($reflectionType instanceof \ReflectionUnionType || $reflectionType instanceof \ReflectionIntersectionType) { | ||
throw new \LogicException(\sprintf('Union or intersection types are not supported for LiveProps. You may want to change the type of property %s in %s.', $property->getName(), $property->getDeclaringClass()->getName())); | ||
} | ||
|
||
if ($type instanceof UnionType && !$type instanceof NullableType || $type instanceof IntersectionType) { | ||
throw new \LogicException(\sprintf('Union or intersection types are not supported for LiveProps. You may want to change the type of property "%s" in "%s".', $propertyName, $className)); | ||
$infoType = $this->propertyTypeExtractor->getType($className, $property->getName()); | ||
|
||
$collectionValueType = $infoType instanceof CollectionType ? $infoType->getCollectionValueType() : null; | ||
|
||
if (null !== $collectionValueType && null !== $infoType) { | ||
$type = $infoType; | ||
} elseif (null !== $reflectionType) { | ||
$type = $this->typeResolver->resolve($reflectionType); | ||
} else { | ||
$type = Type::mixed(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really not sure about this code, it feels fragile.
I tried to re-use the legacy logic and use ReflectionType
or the result of PropertyTypeExtractorInterface::getType()
, and now all tests pass, even the one added in this PR (which was previously breaking)
…ce::getType()` on a `#[LiveProp]` property `x` when getter `getX` exists
9cd665f
to
30dca26
Compare
|
||
if ($type instanceof UnionType && !$type instanceof NullableType || $type instanceof IntersectionType) { | ||
throw new \LogicException(\sprintf('Union or intersection types are not supported for LiveProps. You may want to change the type of property "%s" in "%s".', $propertyName, $className)); | ||
if (null !== $collectionValueType && null !== $infoType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (null !== $collectionValueType && null !== $infoType) { | |
if ($infoType instance of CollectionType) { |
Seems to be enough no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think yes, it would ease the readability too
Prevent:
Given the LiveComponent:
Dumping
$propMetadata
in\Symfony\UX\LiveComponent\LiveComponentHydrator::dehydrateValue
gives:The class name is the
UserInterface
and notUser
🤔To tests it
sfcp req symfony/property-info:^6.4 -W && sfp vendor/bin/simple-phpunit tests/Functional/Form/ComponentWithFormTest.php --filter "testFormWithLivePropContainingAnEntityImplementingAnInterface"
sfcp req 'symfony/property-info:7.2.*' 'symfony/type-info:7.2.*' && sfp vendor/bin/simple-phpunit tests/Functional/Form/ComponentWithFormTest.php --filter "testFormWithLivePropContainingAnEntityImplementingAnInterface"