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.

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

ResultPager::get() can return string #1091

Open
dangrous opened this issue Nov 4, 2022 · 4 comments · May be fixed by #1093
Open

ResultPager::get() can return string #1091

dangrous opened this issue Nov 4, 2022 · 4 comments · May be fixed by #1093

Comments

@dangrous
Copy link

dangrous commented Nov 4, 2022

Because ResultPager::get() here returns the return value of ResponseMediator::getContent(), it can occasionally return a string, which causes an unhelpful TypeError since it should only ever return an array. Other usages of ResponseMediator::getContent() allow for this, but the ResultPager one does not.

This has been occurring in practice to us so figured I'd raise it here - let me know any other information you need on this issue, it is my first one!

@iwiznia
Copy link

iwiznia commented Nov 7, 2022

I am getting the same and to add more information, the response that causes this bug is 500 Internal Server Error, the body of the response is empty.

@iwiznia iwiznia linked a pull request Nov 7, 2022 that will close this issue
@acrobat
Copy link
Collaborator

acrobat commented Nov 16, 2022

@dangrous or @iwiznia do you have an example call that triggers this behaviour? I would expect that the ErrorPlugin would handle the server error response and throw an exception

@iwiznia
Copy link

iwiznia commented Nov 16, 2022

It's not any specific request, I am seeing this error pop up "randomly" and if I retry the request, it succeeds. I think it happens whenever it could not contact to the GH servers, thus the 500 error does not contain any data.
The PR I submitted to you fixes the problem.

@tomcorbett
Copy link
Contributor

tomcorbett commented Mar 21, 2024

@acrobat - from what I see - there are cases (consistently) where GitHub returns 204 "No Content" i.e. https://api.github.com/repos/tomsausage/empty-public/contributors

E.g. in

        // in the (sometimes) valid case that there is no content e.g. getting contributors where there are none
        // GitHub seems not to return an empty array, but instead, no content at all
        if ($result === "" && $this->client->getLastResponse()->getStatusCode() === 204) {
            $result = [];
        }

I know there is an existing PR, perhaps checking for this use case specifically? (Might not be 100% of the issue but at least is reproducible.

Will add PR to show what I mean

tomcorbett added a commit to tomcorbett/php-github-api that referenced this issue Mar 22, 2024
…h breaks ResultPager because no array is returned (it's an empty string) - issue ResultPager::get() can return string KnpLabs#1091
tomcorbett added a commit to tomcorbett/php-github-api that referenced this issue Mar 24, 2024
acrobat pushed a commit that referenced this issue Mar 24, 2024
…arios (tomcorbett)

This PR was squashed before being merged into the 3.14-dev branch.

Discussion
----------

ResultPager breaks because no array is returned (it's an empty string) - issue ResultPager::get() can return string #1091

Commits
-------

8a3f154 Handle case of GitHub returning 204 No Content in some scenarios which breaks ResultPager because no array is returned (it's an empty string) - issue ResultPager::get() can return string #1091
4fff555 fix style issue raised by continuous-integration/styleci/pr
eaa7993 Added unit test for case of GitHub API returning 204 empty content in ResultPager  #1091
08c3d7a Updated based on feedback from continuous-integration/styleci/pr
a3d2fb8 Another change requested by continuous-integration/styleci/pr (but not for my changed code)
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 a pull request may close this issue.

4 participants