-
-
Notifications
You must be signed in to change notification settings - Fork 327
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
[LiveComponent] Implement hydratation of DTO object #1090
[LiveComponent] Implement hydratation of DTO object #1090
Conversation
Fantastic!!! |
ad144b3
to
c19db4d
Compare
c19db4d
to
bd7e719
Compare
$type ? $type->allowsNull() : true, | ||
$collectionValueType, | ||
); | ||
} |
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.
We should centralize this - as it's repeated in https://github.com/symfony/ux/blob/2.x/src/LiveComponent/src/Metadata/LiveComponentMetadataFactory.php
if (!$this->isValueValidDehydratedValue($value)) { | ||
$badKeys = $this->getNonScalarKeys($value, $propMetadata->getName()); | ||
$badKeysText = implode(', ', array_map(fn ($key) => sprintf('%s: %s', $key, $badKeys[$key]), array_keys($badKeys))); | ||
$propMetadata = new LivePropMetadata($key, new LiveProp(true), $type, false, true, null); |
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.
Any reason for this change and the changes above? We already have a $propMetadata
variable - is it not correct for some reason?
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.
The goal here is for each item of the array to go through the dehydrate method. I did that to support array of DTOs
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.
That makes sense - but I think the old code already did that. The main difference between the old and new code just seems to be that the old called ->dehydrateObjectValue()
on each item and the new code calls ->dehydrateValue()
.
But there is an important detail in the original code: to determine the "type" of each item in the array, we should NOT use $objectItem::class
. Instead, we need to read the "property info" off of the property. Otherwise, we may allow for a value to be dehydrated that won't be able to be rehydrated later. Example:
class Foo
{
public array $dtos1 = [];
/** @var Dto[] */
public array $dtos2 = [];
}
$foo = new Foo();
$foo->dtos1 = [new Dto()];
$foo->dtos2 = [new Dto()];
In this situation, the dtos1
property is invalid: we will not be able to hydrate this. We know that dtos1
holds an array, but an array of what? That is the purpose of the $propMetadata->collectionValueType()->getClassName();
: it tells us what the "metadata" (e.g. phpdoc) tells us the "type" of each item in the collection is. If that's missing, we COULD still successfully dehydrate, but hydration would fail. So, instead, we fail during dehydration to warn the user early.
ca5dafb
to
7595c70
Compare
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.
WOW. We're down to the last details. Looking at your test cases got me really excited though... :D. Can you also add some documentation? Thank you!
@@ -392,7 +394,14 @@ private function dehydrateObjectValue(object $value, string $classType, ?string | |||
} | |||
} | |||
|
|||
throw new \LogicException(sprintf('Unable to dehydrate value of type "%s" for property "%s" on component "%s". Either (1) change this to a simpler value, (2) add the hydrateWith/dehydrateWith options to LiveProp or (3) set "useSerializerForHydration: true" on the LiveProp.', $value::class, $propertyPathForError, $componentClassForError)); | |||
$hydratedObject = []; | |||
foreach ((new \ReflectionClass($classType))->getProperties() as $property) { |
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 we should use https://symfony.com/doc/current/components/property_info.html#list-information instead of ReflectionClass::getProperties()
. It'll let us also support private properties with getters & setters (I believe the property info will only return public properties + private properties with a getter).
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 this isn't resolved yet. We're using processAccessor to read the values, but not property info to list the properties.
src/LiveComponent/src/Metadata/LiveComponentMetadataFactory.php
Outdated
Show resolved
Hide resolved
src/LiveComponent/src/Metadata/LiveComponentMetadataFactory.php
Outdated
Show resolved
Hide resolved
src/LiveComponent/tests/Integration/LiveComponentHydratorTest.php
Outdated
Show resolved
Hide resolved
Thanks, Ryan for your review! I just gonna need some help for the docs and the errors messages 😁 |
@@ -392,7 +394,14 @@ private function dehydrateObjectValue(object $value, string $classType, ?string | |||
} | |||
} | |||
|
|||
throw new \LogicException(sprintf('Unable to dehydrate value of type "%s" for property "%s" on component "%s". Either (1) change this to a simpler value, (2) add the hydrateWith/dehydrateWith options to LiveProp or (3) set "useSerializerForHydration: true" on the LiveProp.', $value::class, $propertyPathForError, $componentClassForError)); | |||
$hydratedObject = []; | |||
foreach ((new \ReflectionClass($classType))->getProperties() as $property) { |
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 this isn't resolved yet. We're using processAccessor to read the values, but not property info to list the properties.
daf8429
to
ba53343
Compare
THANK YOU Matheo! This works AWESOME - it's a dream. I put some comments about the docs, but they are so minor, I will make them in a different PR. |
With this PR you van easily use DTO with your LiveComponents.