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

[8.x] Throw ModelNotFoundException for sole in Eloquent #35902

Merged
merged 2 commits into from
Jan 15, 2021
Merged

[8.x] Throw ModelNotFoundException for sole in Eloquent #35902

merged 2 commits into from
Jan 15, 2021

Conversation

rodrigopedra
Copy link
Contributor

Follow up to #35869

As suggested in comment #35869 (comment) by @taylorotwell , this PR throws a ModelNotFoundException when ->sole() is called from an Eloquent Query Builder and no records are found.

This is particularly useful as ModelNotFoundException exceptions are handled automatically in HTTP requests by returning a 404 response.

Maybe we should also add some kind of HTTP handling for the MultipleRecordsFoundException when called from an Eloquent Query Builder.

@themsaid
Copy link
Member

themsaid commented Jan 15, 2021

I think this is going to make things a bit confusing, you'll need to account for ModelNotFoundException and RecordsNotFoundException in your code based on wether the instance is a base builder or eloquent builder.

I think having RecordsNotFoundException associated with the sole() method makes it easier.

What we can do is make ModelNotFoundException extend RecordsNotFoundException.

@rodrigopedra
Copy link
Contributor Author

Making it extend RecordsNotFoundException is a good idea, but won't make the HTTP handler return a 404.

I can instead of extending it on an Eloquent builder add an if clause to the Illuminate\Foundation\Exceptions\Handler@prepareException to account for a RecordsNotFoundException.

What do you think?

@rodrigopedra
Copy link
Contributor Author

My bad, now I get what you meant from your first message.

Updated to extend from RecordsNotFoundException. Thanks!

@rodrigopedra
Copy link
Contributor Author

I suggest returning a 400 Bad Request for a MultipleRecordsFoundException, but I'll leave it up for discussion.

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.

3 participants