Just one type of exception provides response object. #83
Conversation
Fix issue where an untyped exception was requested to hold response object. If error happens earlier (i.e. cURL error) this is not true and process fail to cleanly abort.
|
@jbutkus thanks for submitting.Is it me or are 159 and 156 duplicates? |
| $response = $e->getResponse(); | ||
| \Terminus::error("%s", $response->getBody(TRUE) ); | ||
| } catch( Guzzle\Http\Exception\BadResponseException $e ) { | ||
| $request = $e->getRequest(); | ||
| \Terminus::error("Request %s had failed: %s", (string)$request, $e->getMessage() ); |
There was a problem hiding this comment.
I think you meant to delete lines 156-158 right? Also the reason I was using $response->getBody() is Guzzle throws the exception even when the api returns a valid error response and in those cases we want to know what the API said, not just what the Guzzle exception is. In fact, in most cases the Guzzle exception is useless. So I'd say check for response object first and if so return the body, otherwise return the guzzle exception.
There was a problem hiding this comment.
The trouble is that if there is a cURL failure (timeout, certificate, etc.) - different Exception is thrown. Exception which doesn't implement getResponse method. And then crash contains meaningless information that non-existant method cannot be invoked.
I saw failures Site already exists: <uuid> which are properly handled by this getResponse body - I can extract valuable information then.
So I introduced these two extra tiers to catch different exception types and print corresponding error messages which allows to properly address failure (retry timeouts, notify on other failures, etc.).
There was a problem hiding this comment.
I understand why you did it but I think there's a typo because the exception class referred to in lines 156 and 159 seem to be the same.
There was a problem hiding this comment.
@jbutkus Just to clarify the issue: there are currently 2 duplicated catch blocks for Guzzle\Http\Exception\BadResponseException. Did you mean for one of them to capture a Curl exception instead?
…Exception interface.
|
Thanks @mikevanwinkle and @bensheldon - yes, that was a typo. I changed it to catch instances implementing HTTP exception interface. |
Just one type of exception provides response object.
Fix issue where an untyped exception was requested to hold response object.
If error happens earlier (i.e.
cURLerror) this is not true and process fail to cleanly abort.