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.

Sign up for GitHub

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

Jump to bottom

fixed php warning in GithubExceptionThrower #992

Merged
merged 6 commits into from
Apr 4, 2021
Merged

fixed php warning in GithubExceptionThrower #992

merged 6 commits into from
Apr 4, 2021

Conversation

clxmstaab
Copy link
Contributor

fixes

Warning: Illegal string offset 'code' in GithubExceptionThrower.php on line 59

when deleting a deployment which is not yet set to inactive, see #991

I guess the warning beeing fixed is not explicitly related to the new endpoint added in #991 but I experienced it in this case.

@clxmstaab
fixed php warning in GithubExceptionThrower
2afb40c 
fixes
```
Warning: Illegal string offset 'code' in GithubExceptionThrower.php on line 59
```
when deleting a deployment which is not yet set to inactive
clxmstaab
clxmstaab commented Mar 29, 2021
View reviewed changes
lib/Github/HttpClient/Plugin/GithubExceptionThrower.php Outdated
@@ -59,7 +59,12 @@ public function doHandleRequest(RequestInterface $request, callable $next, calla
if (422 === $response->getStatusCode() && isset($content['errors'])) {
$errors = [];
foreach ($content['errors'] as $error) {
switch ($error['code']) {
$code = null;
Copy link
Contributor Author

@clxmstaab clxmstaab Mar 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when no $error['code'] was contained, the logic in the default-case of the switch statement worked for me.

this PR is just about removing the php warning which was emitted.

acrobat
acrobat requested changes Mar 30, 2021
View reviewed changes
Copy link
Collaborator

@acrobat acrobat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a testcase with the specific error response github is returning for this call? So we don't have any regressions in the future. Thanks!

lib/Github/HttpClient/Plugin/GithubExceptionThrower.php Outdated Show resolved Hide resolved
@clxmstaab
Copy link
Contributor Author

I verified the unit test fails without the patch:

$ vendor/bin/phpunit --verbose
PHPUnit 8.5.15 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.13
Configuration: C:\dvl\GitHub\php-github-api\phpunit.xml.dist

...............................................................  63 / 729 (  8%)
............................................................... 126 / 729 ( 17%)
............................................................... 189 / 729 ( 25%)
............................................................... 252 / 729 ( 34%)
............................................................... 315 / 729 ( 43%)
............................................................... 378 / 729 ( 51%)
............................................................... 441 / 729 ( 60%)
............................................................... 504 / 729 ( 69%)
............................................................... 567 / 729 ( 77%)
............................................................... 630 / 729 ( 86%)
............................................................... 693 / 729 ( 95%)
..............................E.....                            729 / 729 (100%)

Time: 477 ms, Memory: 20.00 MB

There was 1 error:

1) Github\Tests\HttpClient\Plugin\GithubExceptionThrowerTest::testHandleRequest with data set "Cannot delete active deployment" (GuzzleHttp\Psr7\Response Object (...), Github\Exception\ValidationFailedException Object (...))
Illegal string offset 'code'

C:\dvl\GitHub\php-github-api\lib\Github\HttpClient\Plugin\GithubExceptionThrower.php:62
C:\dvl\GitHub\php-github-api\test\Github\Tests\HttpClient\Plugin\GithubExceptionThrowerTest.php:32
C:\dvl\GitHub\php-github-api\lib\Github\HttpClient\Plugin\GithubExceptionThrower.php:121
C:\dvl\GitHub\php-github-api\test\Github\Tests\HttpClient\Plugin\GithubExceptionThrowerTest.php:50

ERRORS!
Tests: 729, Assertions: 1272, Errors: 1.

@acrobat acrobat merged commit e9576a6 into KnpLabs:2.x Apr 4, 2021
@acrobat
Copy link
Collaborator

acrobat commented Apr 4, 2021

Thank you @clxmstaab

acrobat added a commit that referenced this pull request Apr 4, 2021
@acrobat
Merge branch '2.x'
92acfbf 
* 2.x:
  bug #992 fixed php warning in GithubExceptionThrower (clxmstaab, acrobat)
  phpdoc: fix typo
  Update changelog for 2.19.2 release
@clxmstaab clxmstaab deleted the no-warn branch April 6, 2021 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acrobat acrobat acrobat approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@clxmstaab @acrobat