Framework: [Proposal] App::abort() should throw exception with HTTP status as exception code

[sirbarrence]

When testing code that calls App::abort(), it's tough to test what HTTP status code was used if the status was not a 404. All non-404 App::abort()s result in a \Symfony\Component\HttpKernel\Exception\HttpException, so just @expectedException is insufficient to distinguish which HTTP status was used. (404's are easy because they're always \Symfony\Component\HttpKernel\Exception\NotFoundHttpException.)

It would be convenient to be able to use the PHPUnit @expectedExceptionCode annotation to check that the correct HTTP status code was used. App::abort() doesn't send an exception code to the exception constructors, so the exceptions default them to 0. Would it be acceptable to modify Illuminate\Foundation\Application::abort to pass $code to the exception constructors as both the status and exception code? If so, I have this mod written with a few tests.

It's possible to catch the exception inside the test and examine the status code, but it's a real pain compared to using the annotation, especially if you need to test it a lot. You have to make sure to check for and re-throw PHPUnit_Framework_ExpectationFailedExceptions, which feels like a hack. I suppose I could make a helper method in my TestCase parent class though.

[taylorotwell]

I know we simple raise the Symfony HttpException. You can look into seeing if we can be more specific and send a pull request to this repository if so.

[sirbarrence]

Ah, I hadn't noticed there were other HTTP exceptions to use, but there are. I'll work on doing it that way instead.

[sirbarrence]

Those other \Symfony\Component\HttpKernel\Exception\HttpException subclasses like AccessDeniedHttpException don't have constructor parameters for headers like the parent HttpException does. I'm worried that changing App::abort() to use those subclasses instead of HttpException would cause regressions for people who pass headers to App::abort() and depend on the headers being set correctly in the response. App::abort() won't be able to do anything with the headers its given. Not sure why Symfony made the choice to omit headers from the subclass constructors.

Looks like I'll have to write a helper in my TestCase.php to deal with the boilerplate of checking the status code with a try/catch.

[sirbarrence]

In case it helps anyone else or in case someone has a better idea, I've decided to add the following to my TestCase.php:

public function assertHTTPExceptionStatus($expectedStatusCode, Closure $codeThatShouldThrow)
{
    try 
    {
        $codeThatShouldThrow($this);

        $this->assertFalse(true, "An HttpException should have been thrown by the provided Closure.");
    } 
    catch (\Symfony\Component\HttpKernel\Exception\HttpException $e) 
    {
        // assertResponseStatus() won't work because the response object is null
        $this->assertEquals(
            $expectedStatusCode,
            $e->getStatusCode(),
            sprintf("Expected an HTTP status of %d but got %d.", $expectedStatusCode, $e->getStatusCode())
        );
    }
}

Usage:

public function testGetFailsWith403()
{
    $this->assertHTTPExceptionStatus(403, function ($_this)
    {
        $_this->call('GET', '/thing-that-should-fail');
    });
}