PostmarkService::parseResponse ist obfuscating response details

[markushausammann]

By throwing your own generic exception in https://github.com/juriansluiman/SlmMail/blob/master/src/SlmMail/Service/PostmarkService.php#L350 you're reducing information and obfuscating the true reason for failure.

It must be possible for the consumer to process the response from postmark. Either make the response available or pass it to the exception as context so it can be extracted in a catch block.

[markushausammann]

I'm actually not even sure if it's a good idea to have this parseResponse method at all. You should just return the response so the consuming entity can deal with it as it pleases. Or, if you really want to process the response for the user, you need to map the possible API error responses as listed here http://developer.postmarkapp.com/developer-api-overview.html#error-codes and return this information to the consuming entity.

[bakura10]

Hi,

The reason we did unify the exception is to make it possible to switch from one provider to another without changing code. This is because some providers like elasticemail have VERY POOR support for errors, so this way we are able to make everything works the same. So I think this is a good idea. You can catch for SlmMail exception interface, even without knowing which provider you are using internally.

However, I agree that any exception that we could set as previous exception should be done. Don't hesitate to do a PR if you feel this way, we will be glad to review.

Envoyé de mon iPhone

Le 6 nov. 2014 à 17:16, Markus notifications@github.com a écrit :

I'm actually not even sure if it's a good idea to have this parseResponse method at all. You should just return the response so the consuming entity can deal with it as it pleases. Or, if you really want to process the response for the user, you need to map the possible API error responses as listed here http://developer.postmarkapp.com/developer-api-overview.html#error-codes and return this information to the consuming entity.

— Reply to this email directly or view it on GitHub.

[markushausammann]

Another problem is that the parseResponse is private, which means I can't even override it to solve the problem in my Service extension. I don't feel like providing a pull request after what you've done with my last one. You're welcome for my efforts to point the problems out to you, though. Happy fixing.

[juriansluiman]

@markushausammann the API error codes postmark provides are only valid for 422 errors. The exception you highlight is the generic RuntimeException, but actually a ValidationErrorException is thrown with a specific message (postmark calls this not a "validation error" but "input error", the idea is the same). I am talking about this exception.

Would it be sufficient to set an exception code besides the message for you? Then you can catch the specific exception and grab the exception code. It will change SlmMail to this:

case 422:
    $code    = (int) $result['ErrorCode'];
    $message = sprintf(
        'An error occured on Postmark (error code %s), message: %s',
        $result['ErrorCode'],
        $result['Message']
    );
    throw new Exception\ValidationErrorException($message, $code);

Your userland code will become then:

use SlmMail\Service\Exception\ValidationErrorException;

try {
    $transport->send($message);
} catch (ValidationErrorException $e) {
    $code = $e->getCode();
    // handle $code according to
    // http://developer.postmarkapp.com/developer-api-overview.html#error-codes
}

Another problem is that the parseResponse is private, which means I can't even override it to solve the problem in my Service extension.

As with my above example, I don't think (yet) you need to override the method. If the above example is not enough for your use case, please elaborate so we can think along your lines to help.

[markushausammann]

@juriansluiman yes, making the actual ErrorCode and Message extractable from the Exception would be the perfect solution, please change it in that way! This is really important because the reasons for the 422 response are very diverse and some of them can be handled gracefully and don't even need logging. For example 406 can happen and can easily be handled. 405 should probably alert somebody, 10 and 400 should log with high priority, etc.

[juriansluiman]

Closing, see #84

[markushausammann]

Thank you!

[markushausammann]

@juriansluiman would you mind tagging a minor release?

[juriansluiman]

Yes, but let's await merging first ;-)

[markushausammann]

Sure, I thought you had that power :)

[markushausammann]

Or is it running a build on travis or smth?

[juriansluiman]

@markushausammann all PRs are running against Travis and I prefer the policy not to merge PRs myself. So Michael reviews mines and vice versa (most of the times). Now all is green & merged, I tagged v1.5.1

[markushausammann]

Many thanks!