-
-
Notifications
You must be signed in to change notification settings - Fork 217
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
Feature: property-morphable abstract data #921
base: main
Are you sure you want to change the base?
Conversation
ff6dc15
to
1f630b1
Compare
The reason the return value for |
Really looking forward to this feature. Is it going to be in an upcoming release? |
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.
Hi @bentleyo,
This is a really cool PR! Would love to merge it but have a few question/suggestions. Once that has been worked out I think this one can be merged.
There are some points I couldn't add to the review:
- What would happen if we transform an morphed class? Of course it will just be converted into an array/json/... but since we also have support for including/excluding properties will that still work? Especially if a property exists in one class but not in another, again the same with nested structures. Tbh, I have no idea how the code would react we haven't taught about this when building the partials system. Some tests here would be really useful.
- At the moment there aren't docs on this new feature, which I'm sure a lot of people want to use. Would be cool to also include this!
Would love to merge this one! As (probably) my child will be born next week I will be on parental leave for four weeks so could be I'm not directly approving it 😄
return $this->dataFromArrayResolver->execute( | ||
$class, | ||
$pipeline->execute($payloads[0] ?? [], $creationContext) | ||
); | ||
$properties = $pipeline->execute($payloads[0] ?? [], $creationContext); | ||
|
||
return $this->dataFromArray($class, $creationContext, $payloads, $properties); |
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 not completely sure why we first run the pipeline here with the abstract class and then run it again with the inherited class? That first run seems to be completely unnecessary right?
$payload = $path->isRoot() ? $fullPayload : Arr::get($fullPayload, $path->get(), []); | ||
$class = $class::morph($payload) ?? $class; | ||
$dataClass = $this->dataConfig->getDataClass($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.
Let's rewrite this to:
$payload = $path->isRoot() ? $fullPayload : Arr::get($fullPayload, $path->get(), []); | |
$class = $class::morph($payload) ?? $class; | |
$dataClass = $this->dataConfig->getDataClass($class); | |
$morphedClass = $class::morph( | |
$path->isRoot() ? $fullPayload : Arr::get($fullPayload, $path->get(), []) | |
); | |
$dataClass = $this->dataConfig->getDataClass($morphedClass ?? $class); |
use Spatie\LaravelData\Tests\Fakes\PropertyMorphableData\AbstractPropertyMorphableData; | ||
use Spatie\LaravelData\Tests\Fakes\PropertyMorphableData\NestedPropertyMorphableData; | ||
use Spatie\LaravelData\Tests\Fakes\PropertyMorphableData\PropertyMorphableDataA; | ||
use Spatie\LaravelData\Tests\Fakes\PropertyMorphableData\PropertyMorphableDataB; |
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.
Let's try to inline these classes as much as possible, the Fakes directory is already sooo big with a lot of specific classes. My goal is trying to eliminate 90% of them one day so that the definitions of the classes are closer to the tests and so that we don't have to come up with more class names 😄.
Feel free to use anonymous classes or inline classes (just prefix the names with Test).
If a class is being used a lot it might be a fit to put in Fakes but most of the cases it isn't. Except for classes with class attributes because they aren't parsed when written inline if I'm correct.
@rubenvanassche thank you for reviewing! I'll respond over the next couple of days. This is a feature I'm willing to put effort into getting right 😊 How exciting! Congratulations 🎉 Enjoy the next few weeks with your family! |
This PR adds a new feature where abstract data classes can dynamically choose which concrete variant of the class to use based on the payload. This feature works with validation, collections and nested payloads.
While the existing abstract class functionality is good, I find it to be limiting for our use case as it's only available when casting. I've maintained this existing functionality without any breaking changes.
An example use case is having
Vehicle
as an abstract data class withtype
(to determine which concrete to use) and a commonmax_passengers
property used by every class that extendsVehicle
. Each concrete can then add its own additional properties e.g.:This is a feature that's been super useful for us (we have it working on v3 using a trait, but that's no longer possible) and it's a feature that I think would be really great to have built-in.
To use the feature you implement the new
PropertyMorphableData
contract and associated staticmorph
method. This method is supplied with an array of properties and should return a class string.E.g.