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

[5.6] Add reference to model in MassAssignmentException #23229

Merged
merged 2 commits into from
Feb 22, 2018

Conversation

rap2hpoutre
Copy link
Contributor

This pull request and this commit updated mass assignment exception wording. I think we could improve it a bit more by giving the model class name.

So replacing:

Add [name] to fillable property to allow mass assignment.

By:

Add [name] to fillable property to allow mass assignment on App\User.

Errors on models are sometimes confusing, because there is no explicit reference to the actual model in backtrace. This PR could be a one step more to make errors more understandable in Laravel.

@@ -230,8 +230,9 @@ public function fill(array $attributes)
if ($this->isFillable($key)) {
$this->setAttribute($key, $value);
} elseif ($totallyGuarded) {
$model = get_class($this);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use static::class instead, removes the overhead of a function call

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks, updated!

@taylorotwell
Copy link
Member

Check the stack trace.

@rap2hpoutre
Copy link
Contributor Author

rap2hpoutre commented Feb 20, 2018

@taylorotwell Thanks for your answer. Still, there is no reference in these cases about the class in the stack trace: only references to Model and Builder, not to the actual model itself. That's why I added a reference, people may need a reference to the class somewhere. In what case of Mass Assignment did you see a reference to the Model itself in the stack trace?

@taylorotwell taylorotwell reopened this Feb 22, 2018
@rap2hpoutre
Copy link
Contributor Author

Thanks for repopening. Style CI failed but I don't know why. Any idea @GrahamCampbell?

@taylorotwell taylorotwell merged commit d0314a0 into laravel:5.6 Feb 22, 2018
@GrahamCampbell GrahamCampbell changed the title Add reference to model in MassAssignmentException [5.6] Add reference to model in MassAssignmentException Feb 22, 2018
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.

None yet

3 participants