-
Notifications
You must be signed in to change notification settings - Fork 505
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
Passed curl errors along to the user #165
base: master
Are you sure you want to change the base?
Conversation
This doesn't really make sense because calling |
Do you have a specific recommendation on an alternate way to make sure curl errors make it back to the user? @robertark |
Ah, I see what you mean. This could occur before https://github.com/mmcev106/mailchimp-api/blob/cc0b9f73c51f87157941ca57f33217973d27154b/src/MailChimp.php#L251 That way, in the future, nothing messes up in the Another solution may be to set a new curl property, and check that status in the |
…tting swallowed)" This reverts commit cc0b9f7.
…allowed) This solution takes into account robertark's suggestion of avoiding calling determineSuccess() all together if a curl error is detected.
I changed when determineSuccess() is called as @robertark suggested, which is definitely a better solution. I believe this reorganization should only affect $this->last_error, and only in the case where curl itself fails with an error. |
I am confused about the travis test failures due to the test environment variables not being initialized. I'm not sure how that is supposed to work on travis, but I don't think anything in my commit would have affected it. If anyone can shed light on that, I would much appreciate it. |
MailChimp list IDs are set as env vars for unit tests. For some reason when the automated PR check happens those env vars aren't used, so the tests fail. It's not your code - it happens every time. |
Passed curl errors along to the user (they were previously getting swallowed)