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

Passed curl errors along to the user #165

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mmcev106
Copy link

@mmcev106 mmcev106 commented Jan 16, 2017

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

@RobWiddick
Copy link

RobWiddick commented Mar 3, 2017

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!)

@mmcev106
Copy link
Author

mmcev106 commented Mar 3, 2017

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

@RobWiddick
Copy link

RobWiddick commented Mar 3, 2017

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.

mmcev106 added 3 commits March 6, 2017 20:11
…allowed)

This solution takes into account robertark's suggestion of avoiding calling determineSuccess() all together if a curl error is detected.
@mmcev106
Copy link
Author

mmcev106 commented Mar 7, 2017

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
Copy link
Author

mmcev106 commented Mar 7, 2017

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.

@drewm
Copy link
Owner

drewm commented Mar 7, 2017

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.

@drzraf drzraf mentioned this pull request Oct 2, 2019
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