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

HTTP status code != API Problem status #6

Open
weierophinney opened this issue Dec 31, 2019 · 6 comments
Open

HTTP status code != API Problem status #6

weierophinney opened this issue Dec 31, 2019 · 6 comments

Comments

@weierophinney
Copy link
Contributor

When I throw exception in resource:

throw new \Exception('Error', 4000010);

I get:

  • 500 in HTTP status code
  • 4000010 in API problem status (content)

Apigility documentation says (https://apigility.org/documentation/api-primer/error-reporting):

status: the HTTP status code for the current request (optional; Apigility always provides this).

And API problem documentation (https://tools.ietf.org/html/draft-nottingham-http-problem-06):

"status" (number) - The HTTP status code ([RFC2616], Section 6) generated by the origin server for this occurrence of the problem.
The status member, if present, is only advisory; it conveys the HTTP status code used for the convenience of the consumer. Generators MUST use the same status code in the actual HTTP response, to assure that generic HTTP software that does not understand this format still behaves correctly.

BTW Response after

throw new \Exception('Error', 200);

looks funny :-)


Originally posted by @snapshotpl at zfcampus/zf-api-problem#27

@weierophinney
Copy link
Contributor Author

@weierophinney can you look at this issue? It's looks seriously


Originally posted by @snapshotpl at zfcampus/zf-api-problem#27 (comment)

@weierophinney
Copy link
Contributor Author

I totally agree that ZF Api Problem should not interpret exception codes as http status codes. Exception codes can mean anything. For example, \PDOException uses code to transport the SQLSTATE error code.

I therefore propose that we extend ProblemExceptionInterface with a new method getHttpStatusCode() which returns an integer value to use. Otherwise we just assume 500.

@weierophinney could you please give feedback? If you want, i could create a pull request.


Originally posted by @BreiteSeite at zfcampus/zf-api-problem#27 (comment)

@weierophinney
Copy link
Contributor Author

@BreiteSeite makes sense; feel free to start a PR! :)


Originally posted by @weierophinney at zfcampus/zf-api-problem#27 (comment)

@weierophinney
Copy link
Contributor Author

How is the status of this issue or it's pull request?
I think it would be very helpful, if the api-problem does not interpret the exception code as http status code.


Originally posted by @FrankGiesecke at zfcampus/zf-api-problem#27 (comment)

@weierophinney
Copy link
Contributor Author

I think api-problem should interpret the exception code only if the exception implements the ProblemExceptionInterface (like the api-problem DomainException).

Furthermore, IMHO, only codes in 4xx and 5xx ranges should be considered "problems".
For example:

throw new \Exception('Error', 302);

will produce a 301 Moved Permanently http status code, but without the Location header.
I don't think it makes sense.

For this reason, I had to patch temporary some projects. Currently, I attached a listener to intercept exceptions:

use Zend\Mvc\MvcEvent;
use ZF\ApiProblem\Exception\ProblemExceptionInterface;

class UnhandledExceptionListener
{
    public function __invoke(MvcEvent $e)
    {
        if (!$e->isError()) {
            return;
        }

        $exception = $e->getParam('exception');
        if ($exception
            && !$exception instanceof ProblemExceptionInterface
            && $exception instanceof \Exception
            && ($exception->getCode() < 400 || $exception->getCode() > 599)
        ) {
            $internalServerErrorEx = new InternalServerErrorException($exception);
            $e->setParam('exception', $internalServerErrorEx);
        }
    }
}


class InternalServerErrorException extends \Exception
{
    public function __construct($previous)
    {
        parent::__construct('Internal Server Error', 500, $previous);
    }
}

A possibile solution to avoid BCs in 1.0.x is to check the code ranges only. The $exception instanceof ProblemExceptionInterface could be introduced later.
@weierophinney could you give me a feedback about above solutions, please? Then I could create a PR. I think it's a very important issue.


Originally posted by @leogr at zfcampus/zf-api-problem#27 (comment)

@weierophinney
Copy link
Contributor Author

@weierophinney any news?


Originally posted by @leogr at zfcampus/zf-api-problem#27 (comment)

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

No branches or pull requests

1 participant