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

Feature: property-morphable abstract data #921

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
8 changes: 8 additions & 0 deletions src/Contracts/PropertyMorphableData.php
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;
}
30 changes: 25 additions & 5 deletions src/Resolvers/DataFromSomethingResolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Illuminate\Http\Request;
use Spatie\LaravelData\Contracts\BaseData;
use Spatie\LaravelData\Contracts\PropertyMorphableData;
use Spatie\LaravelData\Enums\CustomCreationMethodType;
use Spatie\LaravelData\Optional;
use Spatie\LaravelData\Support\Creation\CreationContext;
Expand Down Expand Up @@ -39,10 +40,9 @@ public function execute(
$payloadCount = count($payloads);

if ($payloadCount === 0 || $payloadCount === 1) {
return $this->dataFromArrayResolver->execute(
$class,
$pipeline->execute($payloads[0] ?? [], $creationContext)
);
$properties = $pipeline->execute($payloads[0] ?? [], $creationContext);

return $this->dataFromArray($class, $creationContext, $payloads, $properties);
Comment on lines -42 to +45
Copy link
Member

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?

Copy link
Contributor Author

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?

}

$properties = [];
Expand All @@ -57,7 +57,7 @@ public function execute(
}
}

return $this->dataFromArrayResolver->execute($class, $properties);
return $this->dataFromArray($class, $creationContext, $payloads, $properties);
}

protected function createFromCustomCreationMethod(
Expand Down Expand Up @@ -117,4 +117,24 @@ protected function createFromCustomCreationMethod(

return $class::$methodName(...$payloads);
}

protected function dataFromArray(
string $class,
CreationContext $creationContext,
array $payloads,
array $properties,
): BaseData {
$dataClass = $this->dataConfig->getDataClass($class);

if ($dataClass->isAbstract && $dataClass->propertyMorphable) {
/**
* @var class-string<PropertyMorphableData> $class
*/
if ($morph = $class::morph($properties)) {
return $this->execute($morph, $creationContext, ...$payloads);
}
}

return $this->dataFromArrayResolver->execute($class, $properties);
}
}
15 changes: 15 additions & 0 deletions src/Resolvers/DataValidationRulesResolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Copy link
Member

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:

Suggested change
$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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated this 👍

}

$withoutValidationProperties = [];

foreach ($dataClass->properties as $dataProperty) {
Expand Down Expand Up @@ -278,4 +288,9 @@ protected function inferRulesForDataProperty(
$path
);
}

protected function isPropertyMorphable(DataClass $dataClass): bool
{
return $dataClass->isAbstract && $dataClass->propertyMorphable;
}
}
1 change: 1 addition & 0 deletions src/Support/DataClass.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public function __construct(
public readonly ?DataMethod $constructorMethod,
public readonly bool $isReadonly,
public readonly bool $isAbstract,
public readonly bool $propertyMorphable,
public readonly bool $appendable,
public readonly bool $includeable,
public readonly bool $responsable,
Expand Down
18 changes: 10 additions & 8 deletions src/Support/EloquentCasts/DataCollectionEloquentCast.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@ public function get($model, string $key, $value, array $attributes): ?DataCollec

$data = json_decode($value, true, flags: JSON_THROW_ON_ERROR);

$dataClass = $this->dataConfig->getDataClass($this->dataClass);
$isAbstractClassCast = $this->isAbstractClassCast();

$data = array_map(function (array $item) use ($dataClass) {
if ($dataClass->isAbstract && $dataClass->transformable) {
$data = array_map(function (array $item) use ($isAbstractClassCast) {
if ($isAbstractClassCast) {
$morphedClass = $this->dataConfig->morphMap->getMorphedDataClass($item['type']) ?? $item['type'];

return $morphedClass::from($item['data']);
Expand Down Expand Up @@ -73,10 +73,10 @@ public function set($model, string $key, $value, array $attributes): ?string
throw CannotCastData::shouldBeArray($model::class, $key);
}

$dataClass = $this->dataConfig->getDataClass($this->dataClass);
$isAbstractClassCast = $this->isAbstractClassCast();

$data = array_map(function (array|BaseData $item) use ($dataClass) {
if ($dataClass->isAbstract && $item instanceof TransformableData) {
$data = array_map(function (array|BaseData $item) use ($isAbstractClassCast) {
if ($isAbstractClassCast && $item instanceof TransformableData) {
$class = get_class($item);

return [
Expand All @@ -90,7 +90,7 @@ public function set($model, string $key, $value, array $attributes): ?string
: $item;
}, $value);

if ($dataClass->isAbstract) {
if ($isAbstractClassCast) {
return json_encode($data);
}

Expand All @@ -107,6 +107,8 @@ public function set($model, string $key, $value, array $attributes): ?string

protected function isAbstractClassCast(): bool
{
return $this->dataConfig->getDataClass($this->dataClass)->isAbstract;
$dataClass = $this->dataConfig->getDataClass($this->dataClass);

return $dataClass->isAbstract && $dataClass->transformable && ! $dataClass->propertyMorphable;
}
}
4 changes: 3 additions & 1 deletion src/Support/EloquentCasts/DataEloquentCast.php
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ public function set($model, string $key, $value, array $attributes): ?string

protected function isAbstractClassCast(): bool
{
return $this->dataConfig->getDataClass($this->dataClass)->isAbstract;
$dataClass = $this->dataConfig->getDataClass($this->dataClass);

return $dataClass->isAbstract && ! $dataClass->propertyMorphable;
}
}
2 changes: 2 additions & 0 deletions src/Support/Factories/DataClassFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use Spatie\LaravelData\Contracts\AppendableData;
use Spatie\LaravelData\Contracts\EmptyData;
use Spatie\LaravelData\Contracts\IncludeableData;
use Spatie\LaravelData\Contracts\PropertyMorphableData;
use Spatie\LaravelData\Contracts\ResponsableData;
use Spatie\LaravelData\Contracts\TransformableData;
use Spatie\LaravelData\Contracts\ValidateableData;
Expand Down Expand Up @@ -82,6 +83,7 @@ public function build(ReflectionClass $reflectionClass): DataClass
constructorMethod: $constructor,
isReadonly: method_exists($reflectionClass, 'isReadOnly') && $reflectionClass->isReadOnly(),
isAbstract: $reflectionClass->isAbstract(),
propertyMorphable: $reflectionClass->implementsInterface(PropertyMorphableData::class),
appendable: $reflectionClass->implementsInterface(AppendableData::class),
includeable: $reflectionClass->implementsInterface(IncludeableData::class),
responsable: $responsable,
Expand Down
80 changes: 80 additions & 0 deletions tests/CreationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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. new abstract class () wouldn't work). I also wanted to avoid duplicating all the classes I had in CreationTest.php many times (especially as the classes would need unique names), so for now I've created a describe section and put the class definitions at the top. Are you happy with that as a solution?

use Spatie\LaravelData\Tests\Fakes\SimpleData;
use Spatie\LaravelData\Tests\Fakes\SimpleDataWithoutConstructor;

Expand Down Expand Up @@ -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');
});
19 changes: 19 additions & 0 deletions tests/Fakes/Models/DummyModelWithPropertyMorphableCast.php
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,
};
}
}
15 changes: 15 additions & 0 deletions tests/Fakes/PropertyMorphableData/NestedPropertyMorphableData.php
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,
) {
}
}
15 changes: 15 additions & 0 deletions tests/Fakes/PropertyMorphableData/PropertyMorphableDataA.php
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');
}
}
12 changes: 12 additions & 0 deletions tests/Fakes/PropertyMorphableData/PropertyMorphableDataB.php
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');
}
}
Loading
Loading