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

Eloquent custom class cast cache problem #33311

Closed
shaffe-fr opened this issue Jun 23, 2020 · 7 comments
Closed

Eloquent custom class cast cache problem #33311

shaffe-fr opened this issue Jun 23, 2020 · 7 comments

Comments

@shaffe-fr
Copy link
Contributor

  • Laravel Version: 7.15.0
  • PHP Version: 7.4.5
  • Database Driver & Version: N/A

Description:

Eloquent is caching the result of custom class cast calls. The cache key is the attribute name.
There is only one cache that is used for both original and dirty attributes.
As a result:

  • the value initialized by getOriginal or getAttribute will always be returned until the cache is reset,
  • any changes might be overwritten by the original value when mergeAttributesFromClassCasts is called from HasAttributes::getAttributes.

Steps To Reproduce:

  1. Create a model with a custom class cast property
  2. Set a value
  3. Persist the property with syncOriginal
  4. Change the property value to a null or non null value
  5. Call getOriginal: $classCastCache is initialized with the original value
  6. Call getDirty again: cache will be persisted by the HasAttributes::mergeAttributesFromClassCasts in HasAttributes::getAttributes
// Always returns the first fetched value
$user = new MyUser([
    'prop' => MyEnum::VALUE,
]);
$user->syncOriginal();
$user->prop = MyEnum::OTHER_VALUE;
dump($user->getOriginal('prop'), $user->prop);
// Expected: MyEnum::VALUE, MyEnum::OTHER_VALUE
// Actual: MyEnum::VALUE, MyEnum::VALUE

// Dirty changes lost
$user = new MyUser([
    'prop' => MyEnum::VALUE,
]);
$user->syncOriginal();
$user->prop = null;
dump($user->prop, $user, $user->getOriginal('prop'), $user->prop);
// Expected: null, MyEnum::VALUE, null
// Actual: null, MyEnum::VALUE, MyEnum::VALUE

Possible Fixes

We might create two caches: one for original values and one for changes.
We could also not use the cache with getOriginal.
I did the following in my BaseModel:

public function getOriginal($key = null, $default = null)
{
    $previousCache = $this->classCastCache;
    $this->classCastCache = [];

    $result = parent::getOriginal($key, $default);

    $this->classCastCache = $previousCache;

    return $result;
}
@taylorotwell
Copy link
Member

Share your cast class.

@shaffe-fr
Copy link
Contributor Author

class MyEnum implements Castable
{
    public const VALUE = 1;
    public const OTHER_VALUE = 2;
    public $value;
    public function __construct($value = null)
    {
        $this->value = $value;
    }

    public static function castUsing()
    {
        return new MyEnumCast(static::class);
    }
}

class MyEnumCast implements CastsAttributes
{
    protected $enumClass;

    public function __construct(string $enumClass)
    {
        $this->enumClass = $enumClass;
    }

    public function get($model, string $key, $value, array $attributes)
    {
        if ($value === null) {
            return $value;
        }

        if ($value instanceof MyEnum) {
            return $value;
        }

        return new $this->enumClass($value);
    }

    public function set($model, string $key, $value, array $attributes)
    {

        $value = $this->get($model, $key, $value, $attributes);

        return [$key => $value->value ?? null];
    }
}

@taylorotwell
Copy link
Member

Fixed by: b20125d

Can you try this patch in your own application?

@shaffe-fr
Copy link
Contributor Author

Thanks for the quick feedback.
This solves the problem when the key is given to getOriginal().
However, the following still fails:

$user = new MyUser([
    'prop' => MyEnum::VALUE,
]);
$user->syncOriginal();
$user->prop = MyEnum::OTHER_VALUE;

dump($user->prop, $user->getOriginal(), $user->prop);
/*
MyEnum {#1443
  +value: 2
}
array:1 [
  "prop" => MyEnum {#1443
    +value: 2
  }
]
MyEnum {#1443
  +value: 2
}
*/

@taylorotwell
Copy link
Member

I will make a slight change for this; however, it's important to note that if you pull a record from the database and hit getOriginal() with no arguments, you will get the raw, original attributes. If you have a custom class cast that maps to multiple attributes only the attributes it maps to would be included in the original array because there is no corresponding "column" for the class cast attribute if it serving as a "virtual" attribute of sorts.

@shaffe-fr
Copy link
Contributor Author

OK thanks.

@taylorotwell
Copy link
Member

Will be fixed in next patch release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants