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

[9.x] Add Eloquent mode to prevent silently discarding fills for attributes not in $fillable #43893

Merged

Conversation

ralphjsmit
Copy link
Contributor

@ralphjsmit ralphjsmit commented Aug 27, 2022

One very irritating thing for me is to forget adding a new property to the $fillable array (because it's not my default way of working) and later discover that fills have been silently discarded, thus causing bugs.

Personally I always unguard my models using $guarded = [], because I know from myself that I never do anything like $model->update($request->all()). However, sometimes you join a new project where it's the convention to always use the $fillable array.

Currently Eloquent only throws an exception when the model is "totally guarded". This PR adds a method that is similar to the Model::preventLazyLoading() to also throw exceptions when a model change was discarded:

Model::preventDiscardingGuardedAttributeFills();

It also works with a boolean:

Model::preventDiscardingGuardedAttributeFills(! app()->isProduction());

Naming ideas are appreciated. I tried to come up with a shorter name, but I couldn't yet think of a name that could convey the behaviour in a better and shorter way.

People can also add a callback to handle the exception. The callback receives the model, the key and the value of the attribute:

Model::handleMassAssignmentViolationUsing(function(Model $model, string $key, mixed $value) {
    //
});

I think this would be very useful to prevent bugs on local environments when the developer forgets to add an attribute to $fillable. Thanks!

@ralphjsmit ralphjsmit changed the title [9.x] Always throw MassAssignmentExceptions locally [9.x] Add method to prevent discarding changes to guarded attributes and throw an exception instead Aug 27, 2022
@ralphjsmit ralphjsmit changed the title [9.x] Add method to prevent discarding changes to guarded attributes and throw an exception instead [9.x] Add Eloquent mode to prevent prevently silently discarding fills for attributes not in $fillable Aug 27, 2022
@taylorotwell taylorotwell merged commit 6c18db4 into laravel:9.x Sep 1, 2022
@foremtehan
Copy link
Contributor

add tests to your codes, thank you.

@ralphjsmit
Copy link
Contributor Author

add tests to your codes, thank you.

I always add tests. Please double check before you say that.

Schermafbeelding 2022-09-01 om 16 48 25

Schermafbeelding 2022-09-01 om 16 47 39

@edalzell
Copy link

edalzell commented Sep 6, 2022

Love this one, thanks @ralphjsmit!

@tillkruss
Copy link
Contributor

This should be the the Laravel default.

@ralphjsmit ralphjsmit changed the title [9.x] Add Eloquent mode to prevent prevently silently discarding fills for attributes not in $fillable [9.x] Add Eloquent mode to prevent silently discarding fills for attributes not in $fillable Sep 7, 2022
@ralphjsmit
Copy link
Contributor Author

@tillkruss Agree! You could PR it though, perhaps Taylor is open to making it the default in 9.x or 10.x. 👊

@taylorotwell
Copy link
Member

taylorotwell commented Sep 12, 2022

@ralphjsmit this PR actually doesn't work for simple cases like:

$user = new User(['unfillable' => 'foo']);

@taylorotwell
Copy link
Member

Attempted fix here: eff2275

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

Successfully merging this pull request may close these issues.

5 participants