Updated exception event listener #15
Conversation
Updated exception event listener to better represent desired logic and handle edge scenarios like encountered here: zf-fr#14
Updated method's DocBlock throws statement.
|
|
||
| $result = json_decode($response->getBody(), true); | ||
| $errorName = isset($result['name']) ? $result['name'] : 'Unknown_Exception'; | ||
| if ( |
There was a problem hiding this comment.
This does not follow PSR, please write:
// All MailChimp error responses have a body and non-200 HTTP code
if (null !== $response && 200 !== $response->getStatusCode()) {
}
|
Also, Travis is not configured for MailChimp, but please make sure all the tests run. |
|
The idea behind this code is that we can't just trust third party service in respect to returned values. Even MailChimp is not protected from errors. So it's a good idea to check every possible scenario. This way program will be more stable and reliable. I don't see any harm in doing more checks. More checks is better than less. And there is no performance issue here. This approach sometimes called a defensive programming. |
|
This is not being defensive, this is making the code harder to read than it needs. Honestly I've never in my life checked if the json was properly decoded. This cannot fail, and even if it failed, this is such an extreme rare case (and would likely be because of MailChimp), that hard fail is not really a problem. |
| if (null !== $response && 200 !== $response->getStatusCode()) { | ||
| // MailChimp body is in JSON, decoding it. | ||
| $result = json_decode($response->getBody(), true); | ||
| if (JSON_ERROR_NONE !== json_last_error() || !is_array($result)) { |
There was a problem hiding this comment.
So please remove this, it's not needed and cannot happen.
|
Does my feedback makes sense? |
|
Hey @bakura10! |
|
Thanks. I have no time for it neither until next wednesday, 'ill do that later :). Please ping me if I forget, and thanks for your contribution ;). |
Updated exception event listener to better represent desired logic and handle edge scenarios like encountered here: #14
Probably we should also update unit tests.