Skip to content

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