ExceptionApiProblem does not handle exceptions that return a non-numeric status code

[judgej]

Bug Report

Summary

Some exceptions return a code getCode() - that are non-numeric, but a string instead. For example an Illuminate\Database\QueryException in a Laravel application had a code "42S02" (note the "S" in the middle). Passing this into the Phpro\ApiProblem\Http\ExceptionApiProblem for rendering results in a further exception being thrown:

Phpro\ApiProblem\Http\HttpApiProblem::__construct(): Argument #1 ($statusCode) must be of type int, string given, called in /.../vendor/phpro/api-problem/src/Http/ExceptionApiProblem.php on line 26 {"exception":"[object] (TypeError(code: 0): Phpro\ApiProblem\Http\HttpApiProblem::__construct(): Argument #1 ($statusCode) must be of type int, string given, called in /.../vendor/phpro/api-problem/src/Http/ExceptionApiProblem.php on line 26 at /.../vendor/phpro/api-problem/src/Http/HttpApiProblem.php:60)

Current behaviour

How to reproduce

This is hard to reproduce outside of Laravel. You cannot create a Illuminate\Database\QueryException exception with a non-numeric code directly, but the framework can. Within laravel, accessing a table that does not exist through the database builder generates an SQL error with details:

+errorInfo: array:3 [
      0 => "42S02"
      1 => 1146
      2 => "Table 'print_trail_core.jobs' doesn't exist"
    ]

which is passed into a PDOException exception, which in turn is passed into an Illuminate\Database\QueryException exception. Fetching the code for that gives "42S02" and that is a problem when that exception is passed to Phpro\ApiProblem\Http\ExceptionApiProblem.

The constructor checks if the code is in the range 400 to 599 and uses it directly if it is, and (bizarrely) it is:

>>> $exceptionCode = "42S02"
=> "42S02"
>>> $statusCode = $exceptionCode >= 400 && $exceptionCode <= 599 ? $exceptionCode : 500
=> "42S02"

If I cast the status code to an int, then it comes out as 42 (again, not sure why - it should be a 500 surely?) but it does not throw a second exception.

Expected behaviour

An exception passed to Phpro\ApiProblem\Http\ExceptionApiProblem that returns a non-numeric getStatus() (and they do exist) should result in a code of 500, and not throw another exception complaining about the non-numeric status code that it tries to use to pass to the parent constructor (class Phpro\ApiProblem\Http\HttpApiProblem).

[judgej]

My workaround is to check the code before instantiating ExceptionApiProblem and if it is not numeric, then fall back to HttpApiProblem:

                if ($exception instanceof Throwable && is_numeric($exception->getCode())) {
                    $apiProblem = new ExceptionApiProblem($exception);
                } else {
                    $apiProblem = new HttpApiProblem(500, [
                        'detail' => config('app.debug') ? ($exception->getMessage() ?: \get_class($exception)) : 'An unexpected error occurred',
                    ]);
                }

The config('app.debug') is just to ensure message details, which can contain an SQL query that threw the error, is only returned in local debug mode.

[judgej]

I will also raise this with Laravel. since non-numeric exception codes should not end up in an exception code.

[veewee]

Thanks for reporting this very detailed issue!
That looks like a bug to me and will be handled in #15.

FYI :

If I cast the status code to an int, then it comes out as 42 (again, not sure why - it should be a 500 surely?) but it does not throw a second exception.

it is the other way around : it casts the int to string, making it:

>>> $exceptionCode = "42S02"
=> "42S02"
>>> $statusCode = $exceptionCode >= '400' && $exceptionCode <= '599' ? $exceptionCode : 500
=> "42S02"

which makes sense, since the string starts with "42S", which is grather than "400" but lower than "500".

[veewee]

Fixed + released
https://github.com/phpro/api-problem/releases/tag/v1.4.1

[judgej]

Brilliant, I'll have a play today.

[judgej]

I'm digging a little deeper into this, to see just why it si happening. It seems that the PDOException redefines code from int to string. Although getCode() is documented as returning an int, the documentation here appears to be wrong. Several of the top comments on that page from 11 years ago (!) make that point https://www.php.net/manual/en/class.pdoexception.php#95812 feels crazy it's been documented like that for so long.

The Laravel QueryException that adds a few convenience methods for reporting on the SQL error, just extends PDOException, so arguably what Laravel is doing is correct, just not well documented. The QueryException returns a non-numeric code because the inherited PDOException returns a non-numeric code. I personally think QueryException should move the non-numeric code to a separate property then substitute in a more appropriate numeric code, just to try to avoid surprises like this.

[veewee]

True, it is supposed to be an int.

If this repo contained static analyzers like psalm or phpstan, we would have known earlier... 🙂

It's fixed now.