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

Talk comment API request doesn't check HTTP response code, fails non-gracefully if not OK #881

Open
iansltx opened this issue Oct 25, 2019 · 2 comments
Labels
Bug help wanted Stuff that we would love contributions for

Comments

@iansltx
Copy link
Contributor

iansltx commented Oct 25, 2019

See joindin/joindin-api#747 for more info on why this is a problem.

In TalkApi::getComments(), we should throw if the HTTP status code for that endpoint is non-200 (even 20x > 200 doesn't make sense here). We should catch that exception in callers and display something to the effect of "comments cannot be displayed right now", particularly if comments aren't the only thing on the page the user is viewing.

We should also log an error when we wind up in this situation, to ensure that monitoring will still pick up the errors (vs. silently running in a degraded state).

@iansltx iansltx added Bug help wanted Stuff that we would love contributions for labels Oct 25, 2019
@svpernova09
Copy link
Contributor

TalkApi::getComments() doesn't check any headers, the request simply returns ["Endpoint not found"] Rather than check for that string in web2 I think we should maybe have the Endpoint not found changed to return false? (which we do check for and throw an exception)

@iansltx
Copy link
Contributor Author

iansltx commented Oct 26, 2019

Issue there is we're using file_get_contents() to grab the response, which means it'll never return boolean false if we get a response at all. So just changing the response body doesn't help us.

Apparently https://www.php.net/manual/en/reserved.variables.httpresponseheader.php would allow us to get the response headers, but...that feels a bit icky given that it's a magically appearing local variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug help wanted Stuff that we would love contributions for
Projects
None yet
Development

No branches or pull requests

2 participants