Support Laravel 8.77 Attributes #1289

SimonJnsson · 2021-12-30T07:58:20Z

Summary

This PR adds support for the new Attribute syntax added in Laravel 8.77, added by laravel/framework#40022.

Using Reflection on the get/set method provided to the Attribute, we can determine the return type of the getter and setter.

Below is an example usage (also found in added test).

use Illuminate\Database\Eloquent\Casts\Attribute;

class Users extends Model
{
    public function name(): Attribute
    {
        return new Attribute(
            get: function (?string $name): ?string {
                return $name;
            },
            set: function (?string $name): ?string {
                return $name === null ? null : ucfirst($name);
            }
        );
    }
}

// => after generate models

/**
 * App\Models\Users
 * 
 * @property string|null $name
 * …
 */

Type of change

This PR should be non breaking, but will however require laravel/framework >= 8.77 to work.

New feature (non-breaking change which adds functionality)

Checklist

Existing tests have been adapted and/or new tests have been added
Add a CHANGELOG.md entry
Code style has been fixed via composer fix-style

mfn

Approach overall LGTM thus I enabled running the tests, but they are failing for some of the PHP versions we need to support.

Can you please check this?

SimonJnsson · 2021-12-30T14:34:31Z

Approach overall LGTM thus I enabled running the tests, but they are failing for some of the PHP versions we need to support.

Can you please check this?

Thanks for taking the time to look at it! I'll get to work on the changes as soon as I get the time 👍

barryvdh · 2021-12-30T22:25:12Z

Looks good. I would just skip the tests when the attribute doesn't exist :)

SimonJnsson · 2022-01-03T07:43:10Z

@mfn i've updated the code to not use code that's incompatible with php 7.3. This should fix the failing unit test.

In regards to the failing integration tests, i'm a bit unsure why these are failing as it seems to somehow be happening when installing dependencies. Any pointers would be greatly appreciated.

SimonJnsson · 2022-01-03T07:45:52Z

@barryvdh sounds good, i've added a check for existance of the Attribute class in the test 👍

mfn

Tests green, change LGTM!

In regards to the failing integration tests

See no failing tests? 🤔

SimonJnsson · 2022-01-03T08:26:01Z

I was referring to this https://github.com/barryvdh/laravel-ide-helper/runs/4667417056?check_suite_focus=true

But it might've been a random one-off as it sees to be gone now 👍

barryvdh#1289 calls `getReturnType` for every method on every model, which in turn calls `getReturnTypeFromDocBlock` which has this code: ```php $phpDocContext = (new ContextFactory())->createFromReflector($reflection); ``` Extracting the docblock is super slow, always has been. Now that we do this for every method, this adds up a lot. Performance on a private commercial project _before_ barryvdh#1289 was introduced: ``` $ time ./artisan ide-helper:models --write --reset >/dev/null real 0m2.857s user 0m1.835s sys 0m0.129s ``` After #1829 : ``` $ time ./artisan ide-helper:models --write --reset >/dev/null real 0m54.147s user 0m47.132s sys 0m1.047s ``` However, in this case we **do not need** the phpdoc fallback (which is legitimate and by design for many other cases), because also the Laravel implementation only works by inspecting the _actual type_, see https://github.com/laravel/framework/blob/e0c2620b57be6416820ea7ca8e46fd2f71d2fe35/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L570-L575 ```php $returnType = (new ReflectionMethod($this, $method))->getReturnType(); return static::$attributeMutatorCache[get_class($this)][$key] = $returnType && $returnType instanceof ReflectionNamedType && $returnType->getName() === Attribute::class && is_callable($this->{$method}()->get); ``` This side-stepping the phpdoc parsing a) still works correctly and b) brings us back to the previous performance characteristics: ``` time ./artisan ide-helper:models --write --reset >/dev/null real 0m2.987s user 0m1.915s sys 0m0.120s ```

barryvdh#1289 calls `getReturnType` for every method on every model, which in turn calls `getReturnTypeFromDocBlock` which has this code: ```php $phpDocContext = (new ContextFactory())->createFromReflector($reflection); ``` Extracting the docblock is super slow, always has been. Now that we do this for every method, this adds up a lot. Performance on a private commercial project _before_ barryvdh#1289 was introduced: ``` $ time ./artisan ide-helper:models --write --reset >/dev/null real 0m2.857s user 0m1.835s sys 0m0.129s ``` After barryvdh#1289 : ``` $ time ./artisan ide-helper:models --write --reset >/dev/null real 0m54.147s user 0m47.132s sys 0m1.047s ``` However, in this case we **do not need** the phpdoc fallback (which is legitimate and by design for many other cases), because also the Laravel implementation only works by inspecting the _actual type_, see https://github.com/laravel/framework/blob/e0c2620b57be6416820ea7ca8e46fd2f71d2fe35/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L570-L575 ```php $returnType = (new ReflectionMethod($this, $method))->getReturnType(); return static::$attributeMutatorCache[get_class($this)][$key] = $returnType && $returnType instanceof ReflectionNamedType && $returnType->getName() === Attribute::class && is_callable($this->{$method}()->get); ``` This side-stepping the phpdoc parsing a) still works correctly and b) brings us back to the previous performance characteristics: ``` time ./artisan ide-helper:models --write --reset >/dev/null real 0m2.987s user 0m1.915s sys 0m0.120s ```

#1289 calls `getReturnType` for every method on every model, which in turn calls `getReturnTypeFromDocBlock` which has this code: ```php $phpDocContext = (new ContextFactory())->createFromReflector($reflection); ``` Extracting the docblock is super slow, always has been. Now that we do this for every method, this adds up a lot. Performance on a private commercial project _before_ #1289 was introduced: ``` $ time ./artisan ide-helper:models --write --reset >/dev/null real 0m2.857s user 0m1.835s sys 0m0.129s ``` After #1289 : ``` $ time ./artisan ide-helper:models --write --reset >/dev/null real 0m54.147s user 0m47.132s sys 0m1.047s ``` However, in this case we **do not need** the phpdoc fallback (which is legitimate and by design for many other cases), because also the Laravel implementation only works by inspecting the _actual type_, see https://github.com/laravel/framework/blob/e0c2620b57be6416820ea7ca8e46fd2f71d2fe35/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L570-L575 ```php $returnType = (new ReflectionMethod($this, $method))->getReturnType(); return static::$attributeMutatorCache[get_class($this)][$key] = $returnType && $returnType instanceof ReflectionNamedType && $returnType->getName() === Attribute::class && is_callable($this->{$method}()->get); ``` This side-stepping the phpdoc parsing a) still works correctly and b) brings us back to the previous performance characteristics: ``` time ./artisan ide-helper:models --write --reset >/dev/null real 0m2.987s user 0m1.915s sys 0m0.120s ```

* ✨ Set properties from model functions returning an Attribute * ✅ Add test for model Attributes * 🎨 Fix code style * 🔖 Update changelog * ✅ Update test to not require php 8 * 🐛 Fix PHP 7.3 incompatibility * ✅ Update tests to only run when Illuminate Attribute exists

…ryvdh#1292) barryvdh#1289 calls `getReturnType` for every method on every model, which in turn calls `getReturnTypeFromDocBlock` which has this code: ```php $phpDocContext = (new ContextFactory())->createFromReflector($reflection); ``` Extracting the docblock is super slow, always has been. Now that we do this for every method, this adds up a lot. Performance on a private commercial project _before_ barryvdh#1289 was introduced: ``` $ time ./artisan ide-helper:models --write --reset >/dev/null real 0m2.857s user 0m1.835s sys 0m0.129s ``` After barryvdh#1289 : ``` $ time ./artisan ide-helper:models --write --reset >/dev/null real 0m54.147s user 0m47.132s sys 0m1.047s ``` However, in this case we **do not need** the phpdoc fallback (which is legitimate and by design for many other cases), because also the Laravel implementation only works by inspecting the _actual type_, see https://github.com/laravel/framework/blob/e0c2620b57be6416820ea7ca8e46fd2f71d2fe35/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L570-L575 ```php $returnType = (new ReflectionMethod($this, $method))->getReturnType(); return static::$attributeMutatorCache[get_class($this)][$key] = $returnType && $returnType instanceof ReflectionNamedType && $returnType->getName() === Attribute::class && is_callable($this->{$method}()->get); ``` This side-stepping the phpdoc parsing a) still works correctly and b) brings us back to the previous performance characteristics: ``` time ./artisan ide-helper:models --write --reset >/dev/null real 0m2.987s user 0m1.915s sys 0m0.120s ```

SimonJnsson added 5 commits December 30, 2021 08:39

✨ Set properties from model functions returning an Attribute

321b4d6

✅ Add test for model Attributes

7cad61a

🎨 Fix code style

6624136

🔖 Update changelog

68b791d

✅ Update test to not require php 8

67e8f7b

SimonJnsson marked this pull request as ready for review December 30, 2021 08:05

mfn requested changes Dec 30, 2021

View reviewed changes

SimonJnsson added 2 commits January 3, 2022 08:11

🐛 Fix PHP 7.3 incompatibility

a5024da

✅ Update tests to only run when Illuminate Attribute exists

641b539

mfn approved these changes Jan 3, 2022

View reviewed changes

barryvdh merged commit 29dd57e into barryvdh:master Jan 3, 2022

jonnywilliamson mentioned this pull request Jan 3, 2022

Latest commit causes massive slowdown on writing to models #1291

Closed

mfn mentioned this pull request Jan 3, 2022

Improve performance for supporting Laravel 8.77+ cast attributes #1292

Merged

9 tasks

mfn mentioned this pull request Jan 9, 2022

Support Castables #1287

Merged

5 tasks

pindab0ter mentioned this pull request Feb 10, 2022

New format computed property accessor not recognised #1315

Open

mridul89 mentioned this pull request Mar 4, 2022

handle protected methods with the attribute syntax #1325

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Laravel 8.77 Attributes #1289

Support Laravel 8.77 Attributes #1289

SimonJnsson commented Dec 30, 2021 •

edited

Loading

mfn left a comment

SimonJnsson commented Dec 30, 2021

barryvdh commented Dec 30, 2021

SimonJnsson commented Jan 3, 2022

SimonJnsson commented Jan 3, 2022

mfn left a comment

SimonJnsson commented Jan 3, 2022

Support Laravel 8.77 Attributes #1289

Support Laravel 8.77 Attributes #1289

Conversation

SimonJnsson commented Dec 30, 2021 • edited Loading

Summary

Type of change

Checklist

mfn left a comment

Choose a reason for hiding this comment

SimonJnsson commented Dec 30, 2021

barryvdh commented Dec 30, 2021

SimonJnsson commented Jan 3, 2022

SimonJnsson commented Jan 3, 2022

mfn left a comment

Choose a reason for hiding this comment

SimonJnsson commented Jan 3, 2022

SimonJnsson commented Dec 30, 2021 •

edited

Loading