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

Declare exceptions unreportable using the ShouldntReport interface #52337

Merged

Conversation

chrispage1
Copy link
Contributor

@chrispage1 chrispage1 commented Jul 31, 2024

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...

<?php

namespace App\Exceptions;

use Illuminate\Contracts\View\View;
use Illuminate\Contracts\Support\Responsable;
use Illuminate\Foundation\Exceptions\Contracts\ShouldntReport;

class BannedException extends \Exception implements ShouldntReport, Responsable
{
    public function toResponse($request): View
    {
        return view('banned');
    }
}

I hope you'll consider this one for merging!

Thanks

@cosmastech
Copy link
Contributor

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 Handler@dontReport array.

// in your AppServiceProvider
$this->app->make(Illuminate\Contracts\Debug\ExceptionHandler::class)->ignore(ShouldntReport::class);

@driesvints
Copy link
Member

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?

@cosmastech
Copy link
Contributor

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]);
    }
}

@driesvints
Copy link
Member

Gotcha @cosmastech thanks for the specific example. Still, shouldn't it always be up to the user to let it be ignored or not?

@taylorotwell
Copy link
Member

@driesvints this is just an alternative syntax to $exceptions->dontReport(...) in the app/bootstrap.php file.

@chrispage1
Copy link
Contributor Author

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]);
    }
}

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.

@taylorotwell taylorotwell merged commit 2a87f91 into laravel:11.x Aug 1, 2024
28 of 29 checks passed
@chrispage1
Copy link
Contributor Author

Thanks @taylorotwell 👌

@chrispage1 chrispage1 deleted the feature/shouldnt-report-interface branch August 2, 2024 06:34
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.

4 participants