feat(refactor): ModelProperty casting #1333

erikgaal · 2022-08-15T10:15:06Z

Added or updated tests
Documented user facing changes
Updated CHANGELOG.md

Fixes #890

Changes

Adds better support for casting using CastsAttributes, CastsInboundAttributes and Castable. Also refactored the ModelPropertyExtension and extracted a ModelCastHelper that can convert Eloquent casts to PHPStan types.

Breaking changes

szepeviktor · 2022-09-02T22:12:55Z

@erikgaal Thank you for your contribution!

What does DTO stand for?

erikgaal · 2022-09-03T08:52:43Z

@erikgaal Thank you for your contribution!

What does DTO stand for?

Hi there! It stands for DataTransferObject, but we can rename it to ValueObject so it matches the Laravel documentation.

szepeviktor · 2022-09-03T09:04:26Z

Thank you!

(🚊 typo 🚊)

canvural · 2022-09-04T16:55:26Z

Thank you for the work here. Just FYI I'm currently on holiday and not available to review until 20th September.

erikgaal · 2022-09-23T19:45:44Z

@canvural Hope you had a great holiday! Would you be available to review this PR?

canvural

Hi, I added some comments.

Please also add tests for the cast types that do not have any tests. Like immutable_date

You can do it by adding adding the casts to any model in the Application folder, and then add a new file to https://github.com/nunomaduro/larastan/blob/4bb0df1aee0d0b8485855f068693688c6f7ae7db/tests/Type/GeneralTypeTest.php and test with assertType

canvural · 2022-10-10T15:08:27Z

src/Properties/ModelCastHelper.php

+        $attributeType = match ($cast) {
+            'int', 'integer' => new IntegerType(),
+            'real', 'float', 'double' => new FloatType(),
+            'decimal' => new AccessoryNumericStringType(),


Accessory types cannot be used standalone. You need to do TypeCombinator::intersect(new StringType(), new AccessoryNumericStringType()) here

src/Properties/ModelCastHelper.php

canvural · 2022-10-10T15:16:36Z

src/Properties/ModelPropertyExtension.php

-        return true;
+    private function migrationsLoaded(): bool
+    {
+        return ! empty($this->tables);


Please use count instead of empty

canvural · 2022-10-10T15:17:53Z

src/Properties/ModelPropertyExtension.php

+            if (! (new ObjectType(Attribute::class))->isSuperTypeOf($returnType)->yes()) {
+                return false;
+            }
+
+            return true;


I think we can simplify it to return (new ObjectType(Attribute::class))->isSuperTypeOf($returnType)->yes()

src/Properties/ModelCastHelper.php

canvural · 2022-10-10T15:22:55Z

src/Properties/ModelCastHelper.php

+            'string' => new StringType(),
+            'bool', 'boolean' => TypeCombinator::union(new BooleanType(), new ConstantIntegerType(0), new ConstantIntegerType(1)),
+            'object' => new ObjectType('stdClass'),
+            'array', 'json' => new ArrayType(new MixedType(), new MixedType()),


We can do better than mixed for the array key here. Try new BenevolentUnionType([new IntegerType(), new StringType()])

canvural · 2022-10-10T15:30:35Z

src/Properties/ModelPropertyExtension.php

+    {
+        $propertyNameStudlyCase = Str::studly($propertyName);
+
+        if ($classReflection->hasNativeMethod("get{$propertyNameStudlyCase}Attribute")) {


Please use sprintf here instead.

canvural · 2022-10-10T15:31:34Z

src/Properties/ModelPropertyExtension.php

+                declaringClass: $classReflection,
+                readableType: $this->stringResolver->resolve($modelInstance->getKeyType()),
+                writableType: $this->stringResolver->resolve($modelInstance->getKeyType()),


Please don't use named arguments.

canvural · 2022-10-10T15:32:34Z

src/Properties/ModelPropertyExtension.php

+                cast: $cast,
+                originalType: $this->stringResolver->resolve($column->readableType),
+            );
+            $writeableType = $this->modelCastHelper->getWriteableType(
+                cast: $cast,
+                originalType: $this->stringResolver->resolve($column->writeableType),


Please don't use named arguments.

canvural · 2022-10-10T15:32:53Z

src/Properties/ModelPropertyExtension.php

+            declaringClass: $classReflection,
+            readableType: $readableType,
+            writableType: $writeableType,
+        );


Please don't use named arguments.

https://laravel.com/docs/9.x/eloquent-mutators#value-object-casting

erikgaal · 2022-12-19T11:39:31Z

Hi @canvural!

Sorry for taking so long to continue, but I've implemented most of your comments. Please also advise what to do with immutable_datetime.

Kind regards,
Erik

canvural · 2022-12-20T13:43:34Z

Thank you!

erikgaal · 2022-12-21T13:01:33Z

I was just thinking back about this PR and that I noted no breaking changes. However, that's not entirely true. If developers were relying on the old behaviour that typed CastsAttributes as mixed or the underlying primitive, that might be breaking as it will now resolve the correct type.

canvural · 2022-12-22T12:23:20Z

@erikgaal That's fine. More precise types leading to errors is acceptable. It's also in PHPStan's BC promise page: https://phpstan.org/user-guide/backward-compatibility-promise#type-inference-capabilities

Just FYI: this PR discovered an issue. https://github.com/nunomaduro/larastan/blob/master/src/Properties/SchemaAggregator.php#L368 this now leads to a object with enum as class name. No idea how it was working before 😄 Could be also a PHPStan change. I'll try to fix it.

canvural · 2022-12-22T12:35:31Z

Ah it didn't discover an error. It broke it 😅 In the latest release enum database column is correctly inferred as union of possible options. But after this PR it is object with enum class name.

canvural · 2022-12-22T12:43:33Z

Reason is that https://github.com/nunomaduro/larastan/blob/2.2.9/src/Properties/ModelPropertyExtension.php#L190-L222 this code was removed.

canvural force-pushed the master branch from 6e217aa to cd07899 Compare August 30, 2022 19:00

erikgaal force-pushed the patch-casts-attributes branch from bef7645 to 60bab86 Compare September 2, 2022 21:58

erikgaal changed the title ~~Support inbound casting~~ Refactor ModelProperty casting Sep 2, 2022

szepeviktor approved these changes Sep 4, 2022

View reviewed changes

canvural requested changes Oct 10, 2022

View reviewed changes

canvural force-pushed the master branch 2 times, most recently from 6e390bd to 325d2f3 Compare November 9, 2022 13:27

tadhgboyle mentioned this pull request Nov 11, 2022

Improve handling of read types on custom model casts #1444

Closed

3 tasks

erikgaal added 12 commits December 19, 2022 11:48

Support custom casts using CastsAttributes

f4ca6bf

Fixes from StyleCI

9098642

Rewrite ModelPropertyExtension with ModelCastHelper

884234b

CastInboundAttribute test

a435e1a

Fixes from StyleCI

695a193

Rename DTO namespace to ValueObjects to match Laravel documentation

80e529f

https://laravel.com/docs/9.x/eloquent-mutators#value-object-casting

Fix date readable type

8d618b5

Fixes from StyleCI

84bbbfc

Update Favorites namespace in test

d237229

Process feedback

69e9554

Handle encrypted casts

bdbea43

Add tests for all castable properties

3611020

erikgaal force-pushed the patch-casts-attributes branch from 2c4c18a to 3611020 Compare December 19, 2022 11:36

Apply fixes from StyleCI

981f75c

canvural changed the title ~~Refactor ModelProperty casting~~ feat(refactor): ModelProperty casting Dec 20, 2022

canvural merged commit e008b92 into larastan:master Dec 20, 2022

erikgaal deleted the patch-casts-attributes branch December 20, 2022 13:44

erikgaal mentioned this pull request Dec 28, 2022

feat: handle native enum casts #1494

Merged

attac-t mentioned this pull request Jan 19, 2023

Adding arguments to custom model casts via $casts[] results in incorrect property type #1525

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(refactor): ModelProperty casting #1333

feat(refactor): ModelProperty casting #1333

erikgaal commented Aug 15, 2022 •

edited

szepeviktor commented Sep 2, 2022

erikgaal commented Sep 3, 2022 •

edited

szepeviktor commented Sep 3, 2022

canvural commented Sep 4, 2022

erikgaal commented Sep 23, 2022

canvural left a comment

canvural Oct 10, 2022

canvural Oct 10, 2022

canvural Oct 10, 2022

canvural Oct 10, 2022

canvural Oct 10, 2022

canvural Oct 10, 2022

canvural Oct 10, 2022

canvural Oct 10, 2022

erikgaal commented Dec 19, 2022

canvural commented Dec 20, 2022

erikgaal commented Dec 21, 2022

canvural commented Dec 22, 2022

canvural commented Dec 22, 2022

canvural commented Dec 22, 2022

feat(refactor): ModelProperty casting #1333

feat(refactor): ModelProperty casting #1333

Conversation

erikgaal commented Aug 15, 2022 • edited

szepeviktor commented Sep 2, 2022

erikgaal commented Sep 3, 2022 • edited

szepeviktor commented Sep 3, 2022

canvural commented Sep 4, 2022

erikgaal commented Sep 23, 2022

canvural left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikgaal commented Dec 19, 2022

canvural commented Dec 20, 2022

erikgaal commented Dec 21, 2022

canvural commented Dec 22, 2022

canvural commented Dec 22, 2022

canvural commented Dec 22, 2022

erikgaal commented Aug 15, 2022 •

edited

erikgaal commented Sep 3, 2022 •

edited