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

The isset check needs to be !empty #72

Merged
merged 5 commits into from
Aug 2, 2017
Merged

Conversation

yavorm
Copy link
Contributor

@yavorm yavorm commented Jun 15, 2017

The isset check fails when $json['error'] is set to false, which is sometimes the case.

The `isset` check fails when `$json['error']` is set to an empty string, which is sometimes the case.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 14.878% when pulling d8e5f06 on yavorm:patch-1 into e568bcc on sailthru:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 14.878% when pulling d320f92 on yavorm:patch-1 into e568bcc on sailthru:master.

@coveralls
Copy link

coveralls commented Jun 16, 2017

Coverage Status

Coverage remained the same at 14.878% when pulling d320f92 on yavorm:patch-1 into e568bcc on sailthru:master.

@yavorm
Copy link
Contributor Author

yavorm commented Jun 16, 2017

Looks like the hhvm test failed on master too.

@silverman63
Copy link
Contributor

Hey @yavorm, thanks for the PR.

I've been notified that there's an issue with the Travis image for HHVM, and need to update the image.

What cases have you seen the API return an empty error string?

@yavorm
Copy link
Contributor Author

yavorm commented Jun 16, 2017

@silverman63
getJobStatus is returning false in that key, not a string. Sorry, we fixed this in our fork a while back and I had forgotten what the specific response was. I'm submitting this PR now so we can potentially get rid of our fork and start using this repo.

@yavorm
Copy link
Contributor Author

yavorm commented Jun 16, 2017

Updated the PR description too.

@yavorm
Copy link
Contributor Author

yavorm commented Jun 21, 2017

@silverman63 Please let me know once you have time to evaluate/merge this PR. Thank you!

@silverman63
Copy link
Contributor

Hi @yavorm, will let you know. Thanks for your patience.

@yavorm
Copy link
Contributor Author

yavorm commented Jul 28, 2017

@silverman63 Can we try to re-run the tests and merge this fix?

@silverman63
Copy link
Contributor

@yavorm will take care this week. Will address #73, let yours retest, and merge. I'm not sure I can guarantee an official release just yet, but it will be in master.

@silverman63
Copy link
Contributor

@yavorm if you pull the latest master change, the test should pass for HHVM.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 14.85% when pulling e7e436d on yavorm:patch-1 into 9ce1843 on sailthru:master.

@yavorm
Copy link
Contributor Author

yavorm commented Aug 2, 2017

Thank you @silverman63, passed!

@silverman63 silverman63 merged commit 37f8ff4 into sailthru:master Aug 2, 2017
@djmarland djmarland mentioned this pull request Sep 21, 2017
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