[8.x] Handle ->sole() exceptions
#35912
Conversation
| @@ -373,6 +373,10 @@ protected function prepareException(Throwable $e) | |||
| $e = new HttpException(419, $e->getMessage(), $e); | |||
| } elseif ($e instanceof SuspiciousOperationException) { | |||
| $e = new NotFoundHttpException('Bad hostname provided.', $e); | |||
| } elseif ($e instanceof RecordsNotFoundException) { | |||
| $e = new NotFoundHttpException('Not found.', $e); | |||
| } elseif ($e instanceof MultipleRecordsFoundException) { | |||
There was a problem hiding this comment.
This might also mean internal server error due to the database getting into a bad state?
There was a problem hiding this comment.
We could think of that.
The use case I thought was filter constraints being passed and only one result being expected/allowed to an endpoint. Which would be a user error, not a server error.
I guess the RecordsNotFoundException is fine as it is, but maybe we could change the MultipleRecordsFoundException to be a 500 instead.
I can update the PR to whichever you find better. The goal is to have it handled gracefully as a request exception.
There was a problem hiding this comment.
NB Changing to 500 amounts to just deleting this line and the line below.
There was a problem hiding this comment.
And removing the test for it. Just committed, thanks!
This PR is a follow up of PRs #35902 and #35908
In addition on not reporting the exceptions thrown by calling
->sole()(which is handled by PR #35908) this PR adds twoelseifclauses to\Illuminate\Foundation\Exceptions@prepareExceptionto handle bothRecordsNotFoundException(when not called from anEloquent\Builder) andMultipleRecordsFoundException.Tests were added for both exceptions handling.
EDIT: After conversation with @GrahamCampbell , I just kept the
RecordsNotFoundExceptionhandling. Leaving theMultipleRecordsFoundExceptionto be handled as a regular exception which defaults to a 500.