Skip to content

Corrected error handling in case of 502 GitHub response #759

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

Merged
merged 4 commits into from
Dec 23, 2018
Merged

Corrected error handling in case of 502 GitHub response #759

merged 4 commits into from
Dec 23, 2018

Conversation

sivaschenko
Copy link
Contributor

@sivaschenko sivaschenko commented Dec 12, 2018

In case when GitHub returns a 502 response, the structure of returned content is not handled by GithubExceptionThrower correctly and results in the following error:

PHP Fatal error:  Uncaught Error: Wrong parameters for Github\Exception\RuntimeException([string $message [, long $code [, Throwable $previous = NULL]]]) in /var/www/html/vendor/knplabs/github-api/lib/Github/HttpClient/Plugin/GithubExceptionThrower.php:87
Stack trace:
#0 /var/www/html/vendor/knplabs/github-api/lib/Github/HttpClient/Plugin/GithubExceptionThrower.php(87): Exception->__construct(Array, 502)
#1 /var/www/html/vendor/php-http/httplug/src/Promise/HttpFulfilledPromise.php(34): Github\HttpClient\Plugin\GithubExceptionThrower->Github\HttpClient\Plugin\{closure}(Object(GuzzleHttp\Psr7\Response))
#2 /var/www/html/vendor/knplabs/github-api/lib/Github/HttpClient/Plugin/GithubExceptionThrower.php(88): Http\Client\Promise\HttpFulfilledPromise->then(Object(Closure))
#3 /var/www/html/vendor/php-http/client-common/src/PluginClient.php(160): Github\HttpClient\Plugin\GithubExceptionThrower->handleRequest(Object(GuzzleHttp\Psr7\Request), Object(Closure), Object(Closure))
#4 /var/www/html/vendor/php-http/client-common/src/PluginClie in /var/www/html/vendor/knplabs/github-api/lib/Github/HttpClient/Plugin/GithubExceptionThrower.php on line 87

That happen i.e. when trying to do too many GraphQL requests in a short period of time.

See debug screenshot:

image

Added a proper handling.

@acrobat
Copy link
Collaborator

acrobat commented Dec 15, 2018

Hi @sivaschenko, thanks for the improvement of the error handling! Can you add a test to verify the behaviour change? Thanks!

@okorshenko
Copy link

Hi @acrobat Thank you for review. Could you please suggest what type of tests should be added? (API, integration, etc)

@acrobat
Copy link
Collaborator

acrobat commented Dec 18, 2018

@okorshenko I would suggest to add a unit test to check the behaviour of the GithubExceptionThrower plugin. For example pass a mocked "correct" request and it shouldn't do anything. And another test with a mocked "error" request, it should perform the correct changed logic for a 502 response.

@sivaschenko
Copy link
Contributor Author

Hi @acrobat GithubExceptionThrower was covered with a unit test. Happy Christmas! 🎅🏻

@acrobat
Copy link
Collaborator

acrobat commented Dec 23, 2018

Hi @sivaschenko, really nice testcase you provided there! Thanks for the PR and congrats on your first contribution!

Merry christmas to you!

@acrobat acrobat merged commit 49486a8 into KnpLabs:master Dec 23, 2018
@sivaschenko
Copy link
Contributor Author

Thanks for the merge! @acrobat are there any plans/schedule for the next release?

@acrobat
Copy link
Collaborator

acrobat commented Jan 8, 2019

@sivaschenko I will try to get one out in the next few days!

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

Successfully merging this pull request may close these issues.

3 participants