[5.6] Throttle requests exception

[bencomeau]

Feature

I am proposing that the ThrottleRequests middleware throw a more specific exception as opposed to the current \Symfony\Component\HttpKernel\Exception\HttpException.

Reasoning

The $dontReport property of the exception handler class contains the \Symfony\Component\HttpKernel\Exception\HttpException::class which means by default rate limiting will not be logged. If one wanted to log these events, they would need to update the report method of the exception handler class to something like:

if ($exception instanceof HttpException && $exception->getStatusCode() === 429) {
    // Handle the logging as desired
}

return parent::report($exception);

Furthermore, I don't believe they would be able to use Reportable Exceptions in this instance since the exception is thrown in the framework's middleware.

Solution

Create a new exception which the ThrottleRequests middleware can utilize; one that simply extends the \Symfony\Component\HttpKernel\Exception\HttpException class. Add an additional test to the ThrottleRequestsTest to confirm the middleware is indeed throwing the desired exception.

Final Thoughts

If approved, we may consider adding \Illuminate\Http\Exceptions\ThrottleRequestsException::class to the $dontReport property of the exception handler class. Otherwise, this update would remove the long-standing functionality of not logging rate limiting by default. I would submit a pull request to the laravel repository depending on the outcome of this pull request.

[ntzm]

You can use assertInstanceOf instead

[laurencei]

I personally like the idea - but this is a breaking change. Someone might be relying on the HttpException being thrown. Plus all existing projects would suddenly start logging throttled exceptions (since it is not part of dontReport in existing projects).

[taylorotwell]

I'm not sure there is any breaking change here if it extends HttpException. Previous try / catch will still work and it won't be logged either since that is an instanceof check and it will still see the HttpException type.

Am I wrong?

[bencomeau]

Ah yes, good point @taylorotwell -- you are correct.

[morloderex]

@taylorotwell it will still catch it because of inheritance.

[laurencei]

@bencomeau - now I’m confused. If the whole point of this PR was to allow for ThrottlingExceptions to be logged, but they won’t be logged because of inheritance of HttpException - then that means this PR does not work as originally described? What am I missing?

[GrahamCampbell]

TooManyRequestsHttpException already exists though...

[GrahamCampbell]

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/Exception/TooManyRequestsHttpException.php

[bencomeau]

@laurencei Thanks, hopefully I can add some details to help clear up confusion! With this functionality, users can:

1. More easily determine if the throttle middleware was applied, from within the report method of the exception handler:

   Before:

   if ($exception instanceof HttpException && $exception->getStatusCode() === 429) {
       // Handle the logging as desired
   }
   

   After:

   if ($exception instanceof ThrottleRequestsException) {
       // Handle the logging as desired
   }
   

2. Use Reportable & Renderable Exceptions whereas prior I do not believe this was possible.

Beyond that, I suppose the bottom-line goal was to abstract the exception a bit so it could more fluidly be handled.

@GrahamCampbell Thank you. I did notice this class existed in Symfony however given their handling of the parameters, I thought it was easier/cleaner to implement a different class. This is mostly based on the first parameter of the TooManyRequestsHttpException class, retryAfter, and the fact that we would need to pass null every time since the ThrottleRequests middleware already sets the retry after header value. Honestly, if everyone feels it should extend TooManyRequestsHttpException instead of HttpException I can do that -- I don't feel strongly enough either way.

[laurencei]

thanks @bencomeau

[GrahamCampbell]

Honestly, if everyone feels it should extend TooManyRequestsHttpException instead of HttpException I can do that -- I don't feel strongly enough either way.

I feel strongly it should. It is wrong for laravel to add it's own exception when one already exists, and should have been used.

[GrahamCampbell]

I think it was a mistake for symfony to make their http exception class not abstract tbh.

[bencomeau]

@GrahamCampbell Okay, thanks for the feedback. I do understand why you strongly feel it should -- makes enough sense to me to not reinvent the wheel :)

@taylorotwell Do you agree? Should I update this PR to extend Symfony's TooManyRequestsHttpException or should I submit a new PR?

[morloderex]

@bencomeau it has already been merged in you need to create a new one.