-
-
Notifications
You must be signed in to change notification settings - Fork 340
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] Add support for Doctrine ArrayCollections Hydration #1354
Conversation
Prevent errors like: "Can't get a way to read the property "element" in class "Doctrine\Common\Collections\ArrayCollection"."
(Also fix existing CodeStyling error in PopsTokenParser)
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.
Can you add tests ?
src/LiveComponent/src/DependencyInjection/Compiler/OptionalDependencyPass.php
Outdated
Show resolved
Hide resolved
src/LiveComponent/src/DependencyInjection/Compiler/OptionalDependencyPass.php
Outdated
Show resolved
Hide resolved
src/LiveComponent/src/Hydration/ArrayCollectionHydrationExtension.php
Outdated
Show resolved
Hide resolved
src/LiveComponent/src/Hydration/ArrayCollectionHydrationExtension.php
Outdated
Show resolved
Hide resolved
src/LiveComponent/src/Hydration/ArrayCollectionHydrationExtension.php
Outdated
Show resolved
Hide resolved
src/LiveComponent/src/Hydration/AbstractDoctrineHydrationExtension.php
Outdated
Show resolved
Hide resolved
src/LiveComponent/src/Hydration/AbstractDoctrineHydrationExtension.php
Outdated
Show resolved
Hide resolved
src/LiveComponent/src/Hydration/AbstractDoctrineHydrationExtension.php
Outdated
Show resolved
Hide resolved
src/LiveComponent/src/Hydration/AbstractDoctrineHydrationExtension.php
Outdated
Show resolved
Hide resolved
src/LiveComponent/src/Hydration/ArrayCollectionHydrationExtension.php
Outdated
Show resolved
Hide resolved
…impler to read/maintain"
Add more checks for validation Change variable names, to make it more clear
Add integration test for (de)hydration Doctrine ArrayCollection
Integration test is added. |
Thanks for all those changes :) Now is time for the "core" questions:
|
Add checks in hydrate function, if type is array and if the keys are present
Change assert not by order, but if arrayCollection contains the entity
The behavior of this functionality is the same as the existing Entity version. You can't edit the contents of the EntityObject in the ArrayCollection. Its only allowed to add or remove a EntityObject in the ArrayCollection by the className and identifierValue. Also just added an test with ArrayCollection as Liveprop with writable: true |
@smnandre I've addressed the feedback. Is there anything else needed from my side, or any further questions? Looking forward to your update. |
src/LiveComponent/tests/Integration/LiveComponentHydratorTest.php
Outdated
Show resolved
Hide resolved
'product' => $firstProduct->id, | ||
'productList' => [ | ||
[ | ||
'className' => $firstProduct::class, |
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.
Thus far, we've avoided sending PHP class names to the frontend. For example, an array($product1, $product2)
is already supported, as long as the LiveProp
that this is attached to properly documents that this is @var Product[]
. That logic, iirc, for hydration is this: https://github.com/symfony/ux/blob/2.x/src/LiveComponent/src/LiveComponentHydrator.php#L267-L272 - and here is the fixture class that we use in the tests - https://github.com/symfony/ux/blob/2.x/src/LiveComponent/tests/Integration/LiveComponentHydratorTest.php#L1643-L1648
Not exposing PHP class info to the frontend might be paranoid. But until we get a big need for it, I'd like to continue avoiding it.
But we should definitely support your situation. Does it already work if you add documentation above the property to say that this is a collection - e.g. something like:
class ItemsDTO
{
/** @var Item[] */
private ArrayCollection $items;
}
If this DOES already work, then what we need is a better error: if we detect that a property can't be hydrated, but it is some sort of iterable of objects, then we can tell them extra documentation is needed. If this does not currently work, then we should make it work - but the change might be inside LiveComponentHydrator
.
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.
@weaverryan thanks for your reply
I just tested it and indeed with the annotation and this work like a charm
class ItemsDTO
{
/** @var Item[] */
private array $items;
}
The ArrayCollection version does not work.
class ItemsDTO
{
/** @var Item[] */
private ArrayCollection $items;
}
This generates the following error. "Error rendering "form_name" component: Can't get a way to read the property "elements" in class "Doctrine\Common\Collections\ArrayCollection"."
It would be nice to have a clearer error message in this situation.
This PR is not needed anymore :)
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.
Yay!
It would be nice to have a clearer error message in this situation.
I agree! It feels like, a bit earlier, we should check to see if the property is an iterable. And, if so, throw a better exception. I'd love a PR if you can make one - you obviously understand the technical details quite well.
The existing DoctrineEntityHydration could not handle Doctrine ArrayCollections. When using the EntityType with the option 'multiple' => true, it returns an ArrayCollection, which result in an error. Additionally, setting useSerializerForHydration to true, in some cases, leads to a circular error.
This PR introduces an ArrayCollectionHydration class to enable support for ArrayCollections.
Example form:
Example DTO: