Skip to content

fixed php warning in GithubExceptionThrower - #992

Merged
acrobat merged 6 commits into
KnpLabs:2.xfrom
clxmstaab:no-warn
Apr 4, 2021
Merged

fixed php warning in GithubExceptionThrower#992
acrobat merged 6 commits into
KnpLabs:2.xfrom
clxmstaab:no-warn

Conversation

@clxmstaab

@clxmstaab clxmstaab commented Mar 29, 2021

Copy link
Copy Markdown
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
Comment thread lib/Github/HttpClient/Plugin/GithubExceptionThrower.php Outdated
$errors = [];
foreach ($content['errors'] as $error) {
switch ($error['code']) {
$code = null;

@clxmstaab clxmstaab Mar 29, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@acrobat acrobat left a comment

Copy link
Copy Markdown
Collaborator

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!

Comment thread lib/Github/HttpClient/Plugin/GithubExceptionThrower.php Outdated
clxmstaab
clxmstaab commented Mar 30, 2021
View reviewed changes
Comment thread test/Github/Tests/HttpClient/Plugin/GithubExceptionThrowerTest.php
@clxmstaab

clxmstaab commented Mar 30, 2021

Copy link
Copy Markdown
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

acrobat commented Apr 4, 2021

Copy link
Copy Markdown
Collaborator

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.

2 participants

@clxmstaab @acrobat