Skip to content
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

Merged
merged 11 commits into from
Sep 22, 2023

Conversation

WebMamba
Copy link
Contributor

@WebMamba WebMamba commented Sep 3, 2023

Q A
Bug fix? yes
New feature? no
Tickets Fix #955
License MIT

With this PR you van easily use DTO with your LiveComponents.

class CustomerDetails
{
    public string $name;
    public Address $address;
    public string $city;
}
class Address
{
    public string $street;
    public string $postCode;
}
#[AsLiveComponent(name: 'CustomerDetails')]
class CustomerDetailsComponent
{
    use DefaultActionTrait;

    #[ExposeInTemplate]
    public string $hello = 'hello';

    #[LiveProp(writable: true)]
    public ?CustomerDetails $customerDetails = null;

    public function mount(): void
    {
        $this->customerDetails = new CustomerDetails();
        $this->customerDetails->name = 'Matheo';
        $this->customerDetails->city = 'Paris';
        $this->customerDetails->address = new Address();
        $this->customerDetails->address->street = '3 rue de la Paix';
        $this->customerDetails->address->postCode = '92270';
    }

    #[LiveAction]
    public function switch(): void
    {
        $this->customerDetails = new CustomerDetails();
        $this->customerDetails->name = 'Paul';
        $this->customerDetails->city = 'Paris';
        $this->customerDetails->address = new Address();
        $this->customerDetails->address->street = '3 rue des mimosas';
        $this->customerDetails->address->postCode = '92270';
    }
}
<div {{ attributes }}>
    <p>{{ customerDetails.name }}</p>
    <p>{{ customerDetails.address.street }}</p>
    <button
            data-action="live#action"
            data-action-name="switch"
    >Switch</button>
</div>

@weaverryan
Copy link
Member

Fantastic!!!

@WebMamba WebMamba force-pushed the fix-hydratation-of-dto-object branch from ad144b3 to c19db4d Compare September 5, 2023 17:07
@WebMamba WebMamba force-pushed the fix-hydratation-of-dto-object branch from c19db4d to bd7e719 Compare September 5, 2023 17:12
$type ? $type->allowsNull() : true,
$collectionValueType,
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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.

@WebMamba WebMamba force-pushed the fix-hydratation-of-dto-object branch from ca5dafb to 7595c70 Compare September 12, 2023 10:18
Copy link
Member

@weaverryan weaverryan left a 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!

src/LiveComponent/src/LiveComponentHydrator.php Outdated Show resolved Hide resolved
@@ -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) {
Copy link
Member

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).

Copy link
Member

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/LiveComponentHydrator.php Outdated Show resolved Hide resolved
src/LiveComponent/src/LiveComponentHydrator.php Outdated Show resolved Hide resolved
src/LiveComponent/src/LiveComponentHydrator.php Outdated Show resolved Hide resolved
src/LiveComponent/tests/Fixtures/Kernel.php Show resolved Hide resolved
@WebMamba
Copy link
Contributor Author

Thanks, Ryan for your review! I just gonna need some help for the docs and the errors messages 😁

src/LiveComponent/doc/index.rst Outdated Show resolved Hide resolved
src/LiveComponent/doc/index.rst Outdated Show resolved Hide resolved
src/LiveComponent/doc/index.rst Outdated Show resolved Hide resolved
src/LiveComponent/doc/index.rst Outdated Show resolved Hide resolved
src/LiveComponent/doc/index.rst Outdated Show resolved Hide resolved
src/LiveComponent/doc/index.rst Outdated Show resolved Hide resolved
src/LiveComponent/doc/index.rst Show resolved Hide resolved
src/LiveComponent/src/LiveComponentHydrator.php Outdated Show resolved Hide resolved
@@ -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) {
Copy link
Member

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/LiveComponentHydrator.php Outdated Show resolved Hide resolved
@WebMamba WebMamba force-pushed the fix-hydratation-of-dto-object branch from daf8429 to ba53343 Compare September 21, 2023 16:49
@weaverryan
Copy link
Member

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.

@weaverryan weaverryan merged commit 7ae6aa1 into symfony:2.x Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Live] Better DTO Dehydration
2 participants