Vonage / vonage-php-code-snippets

PHP code examples for using Vonage APIs
Apache License 2.0
29 stars 57 forks source link

Sending SMS error handling #67

Closed boenrobot closed 3 years ago

boenrobot commented 3 years ago

I'm integrating this client with an existing system where we were previously using raw HTTP calls, and while we're at it, I'm also expanding our error handling.

To this end, I'm a bit confused on how to record errors. The examples here: https://developer.nexmo.com/messaging/sms/overview/php suggest checking if the status is different from 0, and then don't show how to get the error message. According to the reference here: https://developer.nexmo.com/api/sms each message should have an "error-text" property describing the reason the particular message may have failed.

But if that's true, then the current Collection object is not covering this case. All objects are purely of type SentSMS and that object doesn't contain a getter for "error-text". And there's no secondary type for errors that's ever produced either.

I did checked in the source and found that there is an exception handling which checks for other errors not covered in either of those pages though.

So... Are the examples on the site not up to date (and all errors are covered by exceptions and different HTTP status codes), or is the error handling in this project incomplete, in that one just can't get the error message? If the latter, should there maybe be a separate object?

dragonmantank commented 3 years ago

The snippet isn't so much wrong as doesn't tell the entire story properly, so I'll make sure it gets changed.

If the SMS API generates an error, we actually throw an Exception that can be caught, and that exception will have the error text. The snippet as-is should never hit the else portion of the block, because a non-0 status would throw the exception and blow up.

The new snippet will look like this (wrote this without testing, but the idea is the same):

try {
    $response = $client->sms()->send(
        new \Vonage\SMS\Message\SMS(TO_NUMBER, BRAND_NAME, 'A text message sent using the Nexmo SMS API')
    );
    $message = $response->current();
    // Do what you want to the message
    echo "The message was sent successfully\n";
} catch (\Exception $e) {
    echo "The message failed with status code " . $e->getCode() . ": " . $e->getMessage() . "\n";
}
boenrobot commented 3 years ago

But... what kinds of errors does the API return really? If the protocol reference is accurate, there should be an "error-text" property in an HTTP 200 response, but the PHP source doesn't handle that (or at least, I could not find it). Instead, in the source, all I could find was other HTTP statuses being expected for errors, and the error text being composed by one of several properties, none called "error-text". I also saw a toggle on whether to error even on HTTP 200, but even so, the "error-text" mentioned in the reference is nowhere to be found.

dragonmantank commented 3 years ago

There's a custom error handler for the SMS (here), and it handles two basic error conditions. The first is if something blows up before it hits the SMS API, which has a slightly different structure. It's rare, but can happen.

Otherwise it parses the response, and loops over each message as each can fail or succeed. It should pull the error-text property from the response:

https://github.com/Vonage/vonage-php-sdk-core/blob/77740665cb96370e8525688b7079c3aa9aebd40d/src/SMS/ExceptionErrorHandler.php#L50

boenrobot commented 3 years ago

Ah. Thank you. I now see the error of my ways.

I was initializing \Vonage\SMS\Client, rather than \Vonage\Client and then calling ->sms(), contrary to the examples. But not doing it that via \Vonage\Client doesn't set all the SMS specific settings there are to the client, including the mentioned exception handler. And since I was instantiating the class directly, I wasn't going through the factory call at any point.

In my defense, I'm planning on only using the SMS API... at least during that request... so I thought I could save some minor overhead by instantiating the SMS client directly.

dragonmantank commented 3 years ago

Gotcha! In theory you can manually instantiate \Vonage\SMS\Client directly (or use the \Vonage\SMS\ClientFactory), but with the amount of other dependencies you'd also have to set up it's just easier to use \Vonage\Client as the entry point since it the SMS client ultimately needs it anyway.

From a performance standpoint you're not going to be gaining much making a product-specific client. None of the other services will be instantiated as nothing is created until it's invoked so there's no wasted memory or file I/O for the services you aren't using.

For now I'll go ahead and close this issue, but if you have anything else feel free to reach out!