-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
Declare exceptions unreportable using the ShouldntReport interface #52337
Declare exceptions unreportable using the ShouldntReport interface #52337
Conversation
We actually added something like this to our project about 2 years ago. It's incredibly helpful! If this doesn't get approved, but someone is interested in adding it to their project: Add this interface to your application and add ShouldntReport to the // in your AppServiceProvider
$this->app->make(Illuminate\Contracts\Debug\ExceptionHandler::class)->ignore(ShouldntReport::class); |
I might be missing some things here but this seems highly contradictory of why you should use exceptions. Exceptions are faults happening in your system and should always be reportable. You give the example of using an exception to notify a user. That doesn't seem like the right way to me and should be an event instead which can be react on to inform the user? |
Things like validation errors and model not found are already added to the Exception Handler's $internalDontReport for the very reason that they are probably not actionable by a dev, and result in very expensive spam in tools like Sentry and DataDog. (I would argue there's a use a case for incrementing a counter when these exceptions occur, as it may be indicative of a probem) Where we use our implementation of ShouldntReport interface is on exceptions that we want to bubble up to the user, and give it a consistent shape. For that reason, these exceptions we do not report usually also implement Responsable. So anywhere within my app, if a custom exception like FailedToUploadToS3 is thrown (in multiple controllers and services), the user always sees the same response shape. Maybe something like: {
"code": "aws_upload_failure",
"type": "bad_file_name"
/**
* and this might change depending on the context of the exception as its built so maybe "invalid_permission", "needs_reauth", ...
*/
} Now I don't have to catch the exception in each controller and make sure it conforms to this response shape. I just let the API exception bubble up to the user, which looks like this: class FailedToUploadToS3 implements ShouldntReport, Responsable
{
private string $type;
public function __construct(string $type, ?\Throwable $previous = null)
{
$this->type = $type;
parent::__construct("Failed to Upload to S3", previous: $previous);
}
public function toResponse($request)
{
return response()->json(['code' => 'aws_upload_failure', 'type' => $this->type]);
}
} |
Gotcha @cosmastech thanks for the specific example. Still, shouldn't it always be up to the user to let it be ignored or not? |
@driesvints this is just an alternative syntax to |
Thanks for your specific use case and this is exactly what it's intended for. As @taylorotwell says it's an alternative syntax. I like being able to see exactly what's going on inside a class without leaving it and this implementation does that. |
Thanks @taylorotwell 👌 |
This is a simple quality of life improvement feature, as an alternative to having to ensure the class is referenced within the bootstrap file.
It isn't a breaking change and allows developers to define specific exceptions that should not be reportable from directly within the exception itself by implementing the
ShouldntReport
contract/interface. It also clearly highlights that specific exception wont be reported when viewing it.A basic use case for this would be when throwing any exception that doesn't need to be reported. For example, a Responsable exception that notifies the user they have been banned...
I hope you'll consider this one for merging!
Thanks