Handling Oauth server internal errors. #621
Conversation
src/Provider/AbstractProvider.php
Outdated
| @@ -674,6 +674,10 @@ protected function parseResponse(ResponseInterface $response) | |||
| throw $e; | |||
| } | |||
|
|
|||
| if ($response->getStatusCode() == 500) { | |||
| throw new UnexpectedValueException('Failed to parse JSON and OAuth server response code is 500. It could be internal error on Oauth server.'); | |||
There was a problem hiding this comment.
@ramsey I'm not opposed to this change. It feels a little worrisome to me that we would single out a specific error code, but in this situation it probably makes sense.
The only thing I would like to see changed is passing the current $e as $previousException to the newly added exception.
There was a problem hiding this comment.
Code style also needs to be fixed here, as this newly added line goes way over 120 characters.
|
Fixed. |
|
Changes Unknown when pulling 8b66b79 on zargener:master into ** on thephpleague:master**. |
src/Provider/AbstractProvider.php
Outdated
| if ($response->getStatusCode() == 500) { | ||
| throw new UnexpectedValueException( | ||
| 'Failed to parse JSON and OAuth server response code is | ||
| 500. It could be internal error on Oauth server.', |
There was a problem hiding this comment.
This line break in the middle of the string is awkward. If trying to avoid going over 120 characters, please split the line up across lines using the concatenation operator.
There was a problem hiding this comment.
Suggestion for wording:
An OAuth server error was encountered that did not contain a JSON body
In situation when your OAuth server dies, the web server often gives you default error 500 page. This page has text/html type, but current behavior - is to try to parse in anyway. The problem is: without handling the 500 code on non-json page, you will get the type error on line 529, which can be hard to debug. So even if this fix will not enough and you reject it, please at least make some notice in documentation about handling such errors. While you auth with Googe, Facebook, etc providers all is usually ok - they test what they write. But there are lots of sites who provides the oauth with awful and unstable realisation.
|
Done. |
In situation when you OAuth server dies, the web server often gives you
default error 500 page. This page has text/html type, but current
behavior - is to try to parse in anyway.
The problem is: without handling the 500 code on non-json page, you will
get the type error on line 529, which can be hard to debug. So even if
this fix will not enough and you reject it, please at least make some
notice in documentation about handling such errors.
While you auth with Googe, Facebook, etc providers all is usually ok -
they test what they write. But there are lots of sites who provides the
oauth with awful and unstable realisation.