mailchimp-api icon indicating copy to clipboard operation
mailchimp-api copied to clipboard

Passed curl errors along to the user

Open mmcev106 opened this issue 7 years ago • 6 comments

Passed curl errors along to the user (they were previously getting swallowed)

mmcev106 avatar Jan 16 '17 16:01 mmcev106

This doesn't really make sense because calling getLastResponse() returns $last_error. The only other place this property is set in this method is directly above, but even then, the method immediately returns false. (unless I'm missing something!)

RobWiddick avatar Mar 03 '17 16:03 RobWiddick

Do you have a specific recommendation on an alternate way to make sure curl errors make it back to the user? @robertark

mmcev106 avatar Mar 03 '17 16:03 mmcev106

Ah, I see what you mean. $last_error is getting overridden from the same method that set the error from cURL. Since the determineSuccess() method is only called once, from the makeRequest() method, perhaps there can be a check to see if $last_error is set first before calling determineSuccess() (or even a local method variable to check first). That way you can skip calling determineSuccess() if the previous cURL call failed, and leave $last_error alone.

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 determineSuccess() method if reused.

Another solution may be to set a new curl property, and check that status in the determineSuccess() method -- but the property would need to be reset on every call, probably from in the makeRequest() method.

RobWiddick avatar Mar 03 '17 21:03 RobWiddick

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.

mmcev106 avatar Mar 07 '17 03:03 mmcev106

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.

mmcev106 avatar Mar 07 '17 03:03 mmcev106

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.

drewm avatar Mar 07 '17 15:03 drewm