-
Notifications
You must be signed in to change notification settings - Fork 11.7k
Prevent Attribute accessors from running for non-arrayable attributes #58041
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
base: 12.x
Are you sure you want to change the base?
Prevent Attribute accessors from running for non-arrayable attributes #58041
Conversation
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
d3e9177 to
311430c
Compare
|
|
||
| return false; | ||
| })->map->name->values()->all(); | ||
| return (new Collection((new ReflectionClass($instance))->getMethods())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried about the breaking change to this method. We've changed it to return methods that just have a set and no get. Is there a way to make this not breaking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about the breaking change. I reverted getAttributeMarkedMutatorMethods() to the original to maintain BC.
The actual fix for #55067 is in attributesToArray() method (lines 225-229), not in getAttributeMarkedMutatorMethods().
The key change is that attributesToArray() now derives mutated attributes only from the arrayable keys:
$mutatedAttributes = array_values(
array_filter(array_keys($attributes), function ($key) {
return $this->hasAnyGetMutator($key);
})
);where $attributes has already been filtered by getArrayableAttributes().
The getAttributeMarkedMutatorMethods() function is only used for cache population and doesn't affect serialization logic. Set-only mutators won't cause issues because hasAttributeGetMutator() returns false for them, so they won't be invoked regardless of the cache.
I've added a regression test testHiddenAttributeAccessorIsNotInvokedDuringSerialization() that verifies hidden attributes with accessors that would throw exceptions are not invoked during toArray()
|
Please mark as ready for review when comment above has been answered. |
…aravel#55067) This change ensures that Attribute-based get mutators are only invoked during serialization when the attribute is actually arrayable. Hidden and non-visible Attribute accessors are no longer invoked during serialization. `attributesToArray()` now only processes mutators for attributes that are actually arrayable (visible and not hidden). This resolves an issue where hidden Attribute accessors could run unexpectedly during serialization, potentially causing unnecessary work or exceptions when accessing unloaded relationships. Key changes: - Added `getArrayableMutatedAttributes()` to filter mutated attributes based on arrayable keys - Reverted to original `getAttributeMarkedMutatorMethods()` to avoid breaking changes - Added regression test verifying hidden Attribute accessors are not invoked during serialization Resolves laravel#55067
| return (new Collection((new ReflectionClass($instance))->getMethods())) | ||
| ->filter( | ||
| fn ($method) => $method->getReturnType() instanceof ReflectionNamedType && | ||
| $method->getReturnType()->getName() === Attribute::class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm missing something but the breaking change is still here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my mistake:
I’ve reverted getAttributeMarkedMutatorMethods() to match 12.x, and the fix is now limited to attributesToArray() (via getArrayableMutatedAttributes()).
Mutated attributes are now only derived from arrayable keys, so hidden/non-visible Attribute accessors are no longer invoked during toArray().
…to preserve backwards compatibility
|
I noticed the CI run is failing on This PR only changes Eloquent’s HasAttributes serialization path and a corresponding Eloquent test; it doesn’t touch the queue failed-job provider or its tests, so this looks unrelated / timing-sensitive. |
|
I am unable to recreate this issue, and looking through the code I don't it supported there. On current 12.x branch, Am I missing something? User model: <?php
namespace App\Models;
// use Illuminate\Contracts\Auth\MustVerifyEmail;
use Illuminate\Database\Eloquent\Casts\Attribute;
use Illuminate\Database\Eloquent\Factories\HasFactory;
use Illuminate\Foundation\Auth\User as Authenticatable;
use Illuminate\Notifications\Notifiable;
class User extends Authenticatable
{
use HasFactory, Notifiable;
protected $guarded = [];
protected $visible = ['id'];
public function name(): Attribute
{
return Attribute::make(
get: function () {
info('In attribute...');
return 'EXAMPLE USER NAME';
},
);
}
protected function casts(): array
{
return [
'email_verified_at' => 'datetime',
'password' => 'hashed',
];
}
}Route: <?php
use App\Models\User;
use Illuminate\Support\Facades\Route;
Route::get('/', function () {
$user = User::find(1);
return $user->toArray();
});Nothing is written in the logs. |
When serializing models to arrays/JSON, Eloquent currently considers all Attribute-based accessors via
getMutatedAttributes(), even for attributes that are not arrayable (hidden or not visible).This can:
Changes
attributesToArray()now derives mutated attributes only from the model's arrayable attributes via a new helper:mutateAttributeForArray():Attribute-based get mutators whenhasAttributeGetMutator()is true (and serializesDateTimeInterfacevalues),getFooAttributemutators.getAttributeMarkedMutatorMethods()no longer invokes Attribute methods; it only inspects their return types to see if they returnAttribute.hasAttributeGetMutator()now lazily resolves and caches the presence of agetclosure, and only invokes the underlying method when required.Behavior
toArray()/toJson()now invoke Attribute accessors only for attributes that:Attributemutators (noget) continue to work as before and are not treated as get mutators.Tests
Added tests to
DatabaseEloquentModelTest:testAccessorsNotCalledForNonVisibleAttributestestAccessorsCalledForVisibleAttributestestAccessorsNotCalledForHiddenAttributestestSetOnlyAttributeMutatorDoesNotBreakSerializationThese ensure that only visible/arrayable attributes trigger Attribute accessors during
toArray().Fixes #55067