Skip to content

Commit d3e9177

Browse files
eanderson43eanderson43
committed
Prevent Attribute accessors from running for non-arrayable attributes
Ensure that Attribute-based accessors are only invoked for arrayable attributes during model serialization. This prevents hidden / non-visible attributes from triggering accessors as a side-effect of toArray() / toJson(), which can cause unnecessary work or unexpected exceptions. - Derive mutated attributes only from arrayable keys via getArrayableMutatedAttributes(). - Lazily resolve Attribute accessors in hasAttributeGetMutator() and cache the result per class/key. - Avoid invoking Attribute methods in getAttributeMarkedMutatorMethods(); inspect return types only. - Use the new Attribute-based mutator path consistently in mutateAttributeForArray(). Includes tests covering: - non-visible Attribute accessors not being called - visible Attribute accessors being called - hidden Attribute accessors not being called - set-only Attribute mutators not breaking serialization
1 parent 40506d8 commit d3e9177

File tree

2 files changed

+161
-25
lines changed

2 files changed

+161
-25
lines changed

‎src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php‎

Lines changed: 52 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -221,8 +221,11 @@ public function attributesToArray()
221221
$attributes = $this->getArrayableAttributes()
222222
);
223223

224+
// Important: derive mutated attributes *only* from the arrayable keys
225+
$mutatedAttributes = $this->getArrayableMutatedAttributes($attributes);
226+
224227
$attributes = $this->addMutatedAttributesToArray(
225-
$attributes, $mutatedAttributes = $this->getMutatedAttributes()
228+
$attributes, $mutatedAttributes
226229
);
227230

228231
// Next we will handle any casts that have been setup for this model and cast
@@ -679,15 +682,24 @@ public function hasAttributeMutator($key)
679682
*/
680683
public function hasAttributeGetMutator($key)
681684
{
682-
if (isset(static::$getAttributeMutatorCache[get_class($this)][$key])) {
683-
return static::$getAttributeMutatorCache[get_class($this)][$key];
685+
$class = get_class($this);
686+
687+
if (array_key_exists($key, static::$getAttributeMutatorCache[$class] ?? [])) {
688+
return static::$getAttributeMutatorCache[$class][$key];
684689
}
685690

691+
// First, ensure there's actually an Attribute-returning method
686692
if (! $this->hasAttributeMutator($key)) {
687-
return static::$getAttributeMutatorCache[get_class($this)][$key] = false;
693+
return static::$getAttributeMutatorCache[$class][$key] = false;
688694
}
689695

690-
return static::$getAttributeMutatorCache[get_class($this)][$key] = is_callable($this->{Str::camel($key)}()->get);
696+
// Lazy evaluation: only now do we resolve and inspect the Attribute
697+
try {
698+
$attribute = $this->{Str::camel($key)}();
699+
return static::$getAttributeMutatorCache[$class][$key] = is_callable($attribute->get);
700+
} catch (\Throwable $e) {
701+
return static::$getAttributeMutatorCache[$class][$key] = false;
702+
}
691703
}
692704

693705
/**
@@ -752,14 +764,14 @@ protected function mutateAttributeForArray($key, $value)
752764
{
753765
if ($this->isClassCastable($key)) {
754766
$value = $this->getClassCastableAttributeValue($key, $value);
755-
} elseif (isset(static::$getAttributeMutatorCache[get_class($this)][$key]) &&
756-
static::$getAttributeMutatorCache[get_class($this)][$key] === true) {
767+
} elseif ($this->hasAttributeGetMutator($key)) {
768+
// Attribute::make(get: ...)
757769
$value = $this->mutateAttributeMarkedAttribute($key, $value);
758770

759-
$value = $value instanceof DateTimeInterface
760-
? $this->serializeDate($value)
761-
: $value;
762-
} else {
771+
if ($value instanceof DateTimeInterface) {
772+
$value = $this->serializeDate($value);
773+
}
774+
} elseif ($this->hasGetMutator($key)) {
763775
$value = $this->mutateAttribute($key, $value);
764776
}
765777

@@ -2437,6 +2449,20 @@ public function getMutatedAttributes()
24372449
return static::$mutatorCache[static::class];
24382450
}
24392451

2452+
protected function getArrayableMutatedAttributes(array $attributes): array
2453+
{
2454+
$keys = array_keys($attributes);
2455+
2456+
if ($keys === []) {
2457+
return [];
2458+
}
2459+
2460+
// Only consider keys that are actually arrayable *and* have a mutator
2461+
return array_values(array_filter($keys, function ($key) {
2462+
return $this->hasAnyGetMutator($key);
2463+
}));
2464+
}
2465+
24402466
/**
24412467
* Extract and cache all the mutated attributes of a class.
24422468
*
@@ -2449,8 +2475,17 @@ public static function cacheMutatedAttributes($classOrInstance)
24492475

24502476
$class = $reflection->getName();
24512477

2452-
static::$getAttributeMutatorCache[$class] = (new Collection($attributeMutatorMethods = static::getAttributeMarkedMutatorMethods($classOrInstance)))
2453-
->mapWithKeys(fn ($match) => [lcfirst(static::$snakeAttributes ? Str::snake($match) : $match) => true])
2478+
// Get Attribute return type methods without invoking them (lazy caching)
2479+
$attributeMutatorMethods = static::getAttributeMarkedMutatorMethods($classOrInstance);
2480+
2481+
// Initialize cache with method names but don't set to true yet
2482+
// This allows us to know which methods might be Attribute mutators
2483+
// without invoking them. The actual 'get' callable check happens lazily
2484+
// in hasAttributeGetMutator() when the attribute is actually accessed.
2485+
static::$getAttributeMutatorCache[$class] = (new Collection($attributeMutatorMethods))
2486+
->mapWithKeys(fn ($match) => [
2487+
lcfirst(static::$snakeAttributes ? Str::snake($match) : $match) => null
2488+
])
24542489
->all();
24552490

24562491
static::$mutatorCache[$class] = (new Collection(static::getMutatorMethods($class)))
@@ -2482,17 +2517,9 @@ protected static function getAttributeMarkedMutatorMethods($class)
24822517
{
24832518
$instance = is_object($class) ? $class : new $class;
24842519

2485-
return (new Collection((new ReflectionClass($instance))->getMethods()))->filter(function ($method) use ($instance) {
2486-
$returnType = $method->getReturnType();
2487-
2488-
if ($returnType instanceof ReflectionNamedType &&
2489-
$returnType->getName() === Attribute::class) {
2490-
if (is_callable($method->invoke($instance)->get)) {
2491-
return true;
2492-
}
2493-
}
2494-
2495-
return false;
2496-
})->map->name->values()->all();
2520+
return (new Collection((new ReflectionClass($instance))->getMethods()))
2521+
->filter(fn ($method) => $method->getReturnType() instanceof ReflectionNamedType &&
2522+
$method->getReturnType()->getName() === Attribute::class)
2523+
->map->name->values()->all();
24972524
}
24982525
}

‎tests/Database/DatabaseEloquentModelTest.php‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3546,6 +3546,115 @@ public function testDefaultBuilderIsUsedWhenUseEloquentBuilderAttributeIsNotPres
35463546

35473547
$this->assertNotInstanceOf(CustomBuilder::class, $eloquentBuilder);
35483548
}
3549+
3550+
public function testAccessorsNotCalledForNonVisibleAttributes()
3551+
{
3552+
$model = new class extends Model {
3553+
protected $visible = ['id', 'name'];
3554+
protected $attributes = ['id' => 1, 'name' => 'Test'];
3555+
3556+
public $locationAccessorCalled = false;
3557+
public $timezoneAccessorCalled = false;
3558+
3559+
protected function location(): Attribute
3560+
{
3561+
$this->locationAccessorCalled = true;
3562+
return Attribute::make(
3563+
get: fn () => 'Paris',
3564+
);
3565+
}
3566+
3567+
protected function timezone(): Attribute
3568+
{
3569+
$this->timezoneAccessorCalled = true;
3570+
return Attribute::make(
3571+
get: fn () => 'Europe/Paris',
3572+
);
3573+
}
3574+
};
3575+
3576+
$array = $model->toArray();
3577+
3578+
$this->assertFalse($model->locationAccessorCalled, 'Location accessor should not be called for non-visible attributes');
3579+
$this->assertFalse($model->timezoneAccessorCalled, 'Timezone accessor should not be called for non-visible attributes');
3580+
$this->assertArrayNotHasKey('location', $array);
3581+
$this->assertArrayNotHasKey('timezone', $array);
3582+
$this->assertEquals(['id' => 1, 'name' => 'Test'], $array);
3583+
}
3584+
3585+
public function testAccessorsCalledForVisibleAttributes()
3586+
{
3587+
$model = new class extends Model {
3588+
protected $visible = ['id', 'name', 'location'];
3589+
protected $attributes = ['id' => 1, 'name' => 'Test', 'location' => 'original'];
3590+
3591+
public $locationAccessorCalled = false;
3592+
3593+
protected function location(): Attribute
3594+
{
3595+
$this->locationAccessorCalled = true;
3596+
return Attribute::make(
3597+
get: fn ($value) => 'Paris',
3598+
);
3599+
}
3600+
};
3601+
3602+
$array = $model->toArray();
3603+
3604+
$this->assertTrue($model->locationAccessorCalled, 'Location accessor should be called for visible attributes');
3605+
$this->assertArrayHasKey('location', $array);
3606+
$this->assertEquals('Paris', $array['location']);
3607+
$this->assertEquals(['id' => 1, 'name' => 'Test', 'location' => 'Paris'], $array);
3608+
}
3609+
3610+
public function testAccessorsNotCalledForHiddenAttributes()
3611+
{
3612+
$model = new class extends Model {
3613+
protected $hidden = ['location'];
3614+
protected $attributes = ['id' => 1, 'name' => 'Test', 'location' => 'original'];
3615+
3616+
public $locationAccessorCalled = false;
3617+
3618+
protected function location(): Attribute
3619+
{
3620+
$this->locationAccessorCalled = true;
3621+
return Attribute::make(
3622+
get: fn ($value) => 'Paris',
3623+
);
3624+
}
3625+
};
3626+
3627+
$array = $model->toArray();
3628+
3629+
$this->assertFalse($model->locationAccessorCalled, 'Location accessor should not be called for hidden attributes');
3630+
$this->assertArrayNotHasKey('location', $array);
3631+
$this->assertEquals(['id' => 1, 'name' => 'Test'], $array);
3632+
}
3633+
3634+
public function testSetOnlyAttributeMutatorDoesNotBreakSerialization()
3635+
{
3636+
$model = new Class extends Model {
3637+
protected $visible = ['id', 'name', 'foo'];
3638+
protected $attributes = ['id' => 1, 'name' => 'Test', 'foo' => 'ORIGINAL'];
3639+
3640+
protected function foo(): Attribute
3641+
{
3642+
return Attribute::make(
3643+
set: function ($value) {
3644+
return ['foo' => strtolower($value)];
3645+
}
3646+
);
3647+
}
3648+
};
3649+
3650+
// Set the attribute using the set mutator
3651+
$model->foo = 'BAR';
3652+
3653+
$array = $model->toArray();
3654+
3655+
// Should not crash, and should still include foo with whatever raw value is stored.
3656+
$this->assertArrayHasKey('foo', $array);
3657+
}
35493658
}
35503659

35513660
class CustomBuilder extends Builder

0 commit comments

Comments
 (0)