- 
                Notifications
    You must be signed in to change notification settings 
- Fork 11.6k
Description
- Laravel Version: 8.78.1
- PHP Version: 8.0
- Database Driver & Version: Irrelevant
Description:
With reference to the method mutateAttributeMarkedAttribute found in the HasAttributes trait introduced with the new attribute casting feature in v8.77:
    protected function mutateAttributeMarkedAttribute($key, $value)
    {
        if (isset($this->attributeCastCache[$key])) {
            return $this->attributeCastCache[$key];
        }
        $value = call_user_func($this->{Str::camel($key)}()->get ?: function ($value) {
            return $value;
        }, $value, $this->attributes);
        if (! is_object($value)) {
            unset($this->attributeCastCache[$key]);
        } else {
            $this->attributeCastCache[$key] = $value;
        }
        return $value;
    }It seems to me that the calculated $value is only cached if it's an object, which essentially means any callable Attribute that returns scalar values (string, boolean etc.) will not be cached. I'm not sure of the reason behind this, but even with this aside, it seems the logic here introduces a bug. Considering the method mergeAttributesFromAttributeCasts() in the same trait, which is called upon saving/updating a model:
    protected function mergeAttributesFromAttributeCasts()
    {
        foreach ($this->attributeCastCache as $key => $value) {
            $callback = $this->{Str::camel($key)}()->set ?: function ($value) use ($key) {
                $this->attributes[$key] = $value;
            };
            $this->attributes = array_merge(
                $this->attributes,
                $this->normalizeCastClassResponse(
                    $key, call_user_func($callback, $value, $this->attributes)
                )
            );
        }
    }If I read it correctly, this method will merge $attributeCastCache back to the model before performing the query. Now if in the model I have an attribute accessor like this:
// User.php
protected function foo(): Attribute
{
   return Attribute::get(fn () => new Foo());
}Once $user->foo is accessed, a 'foo' key will be stored into $attributeCastCache and merged into $attributes. If my table doesn't have a foo column, however, saving the model will yield an error saying the column foo doesn't exist.
This problem won't happen if the closure provided in Attribute::get() returns a non-object value, because 'foo' won't be registered into $attributeCastCache and as such won't exist in the $attributes array after merging.
Steps to Reproduce:
- Checkout https://github.com/phanan/laravel-attr-bug
- Run composer install
- Run php artisan test. The test should break.
~/P/laravel (master) $ php artisan test
   FAIL  Tests\Unit\UserTest
  ⨯ custom attribute
  ---
  • Tests\Unit\UserTest > custom attribute
   Illuminate\Database\QueryException
  SQLSTATE[HY000]: General error: 1 no such column: foo (SQL: update "users" set "foo" = ?, "updated_at" = 2022-01-12 13:47:18 where "id" = 1)More Information:
The bug (?) didn't exist with the old syntax i.e. getFooAttribute(). Of course, as a workaround, I can change foo() from an attribute into a method like this:
public function foo(): Foo
{
    return new Foo();
}But this a) makes it a bit less elegant (IMHO) and b) makes it harder to add custom attributes into appends.
Of course, if this is just me not understanding Laravel, my apologies.