-
-
Notifications
You must be signed in to change notification settings - Fork 227
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?
Changes from 9 commits
c88b8fb
a17d5f9
9e6e93d
2764e83
1f630b1
46c9551
549889b
b978297
17be5be
cc76f20
635d813
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
<?php | ||
|
||
namespace Spatie\LaravelData\Contracts; | ||
|
||
interface PropertyMorphableData | ||
{ | ||
public static function morph(array $properties): ?string; | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -7,6 +7,7 @@ | |||||||||||||||||
use Illuminate\Validation\Rule; | ||||||||||||||||||
use Spatie\LaravelData\Attributes\Validation\ArrayType; | ||||||||||||||||||
use Spatie\LaravelData\Attributes\Validation\Present; | ||||||||||||||||||
use Spatie\LaravelData\Contracts\PropertyMorphableData; | ||||||||||||||||||
use Spatie\LaravelData\Support\DataClass; | ||||||||||||||||||
use Spatie\LaravelData\Support\DataConfig; | ||||||||||||||||||
use Spatie\LaravelData\Support\DataProperty; | ||||||||||||||||||
|
@@ -34,6 +35,15 @@ public function execute( | |||||||||||||||||
): array { | ||||||||||||||||||
$dataClass = $this->dataConfig->getDataClass($class); | ||||||||||||||||||
|
||||||||||||||||||
if ($this->isPropertyMorphable($dataClass)) { | ||||||||||||||||||
/** | ||||||||||||||||||
* @var class-string<PropertyMorphableData> $class | ||||||||||||||||||
*/ | ||||||||||||||||||
$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 commentThe reason will be displayed to describe this comment to others. Learn more. Let's rewrite this to:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated this 👍 |
||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
$withoutValidationProperties = []; | ||||||||||||||||||
|
||||||||||||||||||
foreach ($dataClass->properties as $dataProperty) { | ||||||||||||||||||
|
@@ -278,4 +288,9 @@ protected function inferRulesForDataProperty( | |||||||||||||||||
$path | ||||||||||||||||||
); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
protected function isPropertyMorphable(DataClass $dataClass): bool | ||||||||||||||||||
{ | ||||||||||||||||||
return $dataClass->isAbstract && $dataClass->propertyMorphable; | ||||||||||||||||||
} | ||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,10 @@ | |
use Spatie\LaravelData\Tests\Fakes\NestedLazyData; | ||
use Spatie\LaravelData\Tests\Fakes\NestedModelCollectionData; | ||
use Spatie\LaravelData\Tests\Fakes\NestedModelData; | ||
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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rubenvanassche I've had a go at inlining the classes. Can you have a look and let me know what you think? Unfortunately it's a bit difficult to work with the abstract classes anonymously as I can't create an instance of them (e.g. |
||
use Spatie\LaravelData\Tests\Fakes\SimpleData; | ||
use Spatie\LaravelData\Tests\Fakes\SimpleDataWithoutConstructor; | ||
|
||
|
@@ -1227,3 +1231,79 @@ public static function pipeline(): DataPipeline | |
[10, SimpleData::from('Hello World')] | ||
); | ||
})->todo(); | ||
|
||
it('will allow property-morphable data to be created', function () { | ||
$dataA = AbstractPropertyMorphableData::from([ | ||
'variant' => 'a', | ||
'a' => 'foo', | ||
'enum' => 'foo', | ||
]); | ||
|
||
expect($dataA) | ||
->toBeInstanceOf(PropertyMorphableDataA::class) | ||
->variant->toEqual('a') | ||
->a->toEqual('foo') | ||
->enum->toEqual(DummyBackedEnum::FOO); | ||
|
||
$dataB = AbstractPropertyMorphableData::from([ | ||
'variant' => 'b', | ||
'b' => 'bar', | ||
]); | ||
|
||
expect($dataB) | ||
->toBeInstanceOf(PropertyMorphableDataB::class) | ||
->variant->toEqual('b') | ||
->b->toEqual('bar'); | ||
}); | ||
|
||
it('will allow property-morphable data to be created from concrete', function () { | ||
$dataA = PropertyMorphableDataA::from([ | ||
'a' => 'foo', | ||
'enum' => 'foo', | ||
]); | ||
|
||
expect($dataA) | ||
->toBeInstanceOf(PropertyMorphableDataA::class) | ||
->variant->toEqual('a') | ||
->a->toEqual('foo') | ||
->enum->toEqual(DummyBackedEnum::FOO); | ||
}); | ||
|
||
it('will allow property-morphable data to be created from a nested collection', function () { | ||
$data = NestedPropertyMorphableData::from([ | ||
'nestedCollection' => [ | ||
['variant' => 'a', 'a' => 'foo', 'enum' => 'foo'], | ||
['variant' => 'b', 'b' => 'bar'], | ||
], | ||
]); | ||
|
||
expect($data->nestedCollection[0]) | ||
->toBeInstanceOf(PropertyMorphableDataA::class) | ||
->variant->toEqual('a') | ||
->a->toEqual('foo') | ||
->enum->toEqual(DummyBackedEnum::FOO); | ||
|
||
expect($data->nestedCollection[1]) | ||
->toBeInstanceOf(PropertyMorphableDataB::class) | ||
->variant->toEqual('b') | ||
->b->toEqual('bar'); | ||
}); | ||
|
||
|
||
it('will allow property-morphable data to be created as a collection', function () { | ||
$collection = AbstractPropertyMorphableData::collect([ | ||
['variant' => 'a', 'a' => 'foo', 'enum' => DummyBackedEnum::FOO->value], | ||
['variant' => 'b', 'b' => 'bar'], | ||
]); | ||
|
||
expect($collection[0]) | ||
->toBeInstanceOf(PropertyMorphableDataA::class) | ||
->variant->toEqual('a') | ||
->a->toEqual('foo') | ||
->enum->toEqual(DummyBackedEnum::FOO); | ||
|
||
expect($collection[1]) | ||
->toBeInstanceOf(PropertyMorphableDataB::class) | ||
->variant->toEqual('b') | ||
->b->toEqual('bar'); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
<?php | ||
|
||
namespace Spatie\LaravelData\Tests\Fakes\Models; | ||
|
||
use Illuminate\Database\Eloquent\Model; | ||
use Spatie\LaravelData\Tests\Fakes\PropertyMorphableData\AbstractPropertyMorphableData; | ||
use Spatie\LaravelData\Tests\Fakes\SimpleDataCollection; | ||
|
||
class DummyModelWithPropertyMorphableCast extends Model | ||
{ | ||
protected $casts = [ | ||
'data' => AbstractPropertyMorphableData::class, | ||
'data_collection' => SimpleDataCollection::class.':'.AbstractPropertyMorphableData::class, | ||
]; | ||
|
||
protected $table = 'dummy_model_with_casts'; | ||
|
||
public $timestamps = false; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
<?php | ||
|
||
namespace Spatie\LaravelData\Tests\Fakes\PropertyMorphableData; | ||
|
||
use Spatie\LaravelData\Attributes\Validation\In; | ||
use Spatie\LaravelData\Contracts\PropertyMorphableData; | ||
use Spatie\LaravelData\Data; | ||
|
||
abstract class AbstractPropertyMorphableData extends Data implements PropertyMorphableData | ||
{ | ||
public function __construct( | ||
#[In('a', 'b')] | ||
public string $variant, | ||
) { | ||
} | ||
|
||
public static function morph(array $properties): ?string | ||
{ | ||
return match ($properties['variant'] ?? null) { | ||
'a' => PropertyMorphableDataA::class, | ||
'b' => PropertyMorphableDataB::class, | ||
default => null, | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
<?php | ||
|
||
namespace Spatie\LaravelData\Tests\Fakes\PropertyMorphableData; | ||
|
||
use Spatie\LaravelData\Data; | ||
use Spatie\LaravelData\DataCollection; | ||
|
||
class NestedPropertyMorphableData extends Data | ||
{ | ||
public function __construct( | ||
/** @var AbstractPropertyMorphableData[] */ | ||
public ?DataCollection $nestedCollection, | ||
) { | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
<?php | ||
|
||
namespace Spatie\LaravelData\Tests\Fakes\PropertyMorphableData; | ||
|
||
use Spatie\LaravelData\Tests\Fakes\Enums\DummyBackedEnum; | ||
|
||
class PropertyMorphableDataA extends AbstractPropertyMorphableData | ||
{ | ||
public function __construct( | ||
public string $a, | ||
public DummyBackedEnum $enum, | ||
) { | ||
parent::__construct('a'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
<?php | ||
|
||
namespace Spatie\LaravelData\Tests\Fakes\PropertyMorphableData; | ||
|
||
class PropertyMorphableDataB extends AbstractPropertyMorphableData | ||
{ | ||
public function __construct( | ||
public string $b, | ||
) { | ||
parent::__construct('b'); | ||
} | ||
} |
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?
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.
@rubenvanassche this might just be from me misunderstanding how or why there are multiple payloads 😄
In order to know which concrete class to create we need to collect the properties from the payloads and pass them into the
morph
method. I've made the assumption these properties could come from$payloads[0]
,$payloads[1]
etc. or a combination thereof. It also made sense to me to have the set of properties run through the pipeline so that they're cast / have default values etc.Does that make sense or do you think I should change the approach there?