Skip to content

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!


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.

@WebMamba
Copy link
Contributor Author

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


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.

@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