laravel / vonage-notification-channel

Vonage Notification Channel for Laravel.
https://laravel.com/docs/notifications#sms-notifications
MIT License
744 stars 51 forks source link

[3.x] Switch to `sms()` Client, improved GSM-7 Handling #72

Closed SecondeJK closed 1 year ago

SecondeJK commented 1 year ago

This PR is a redone version of the previously merged #61, which was reverted because v4.0 of the Vonage Core SDK was not being pulled in. Subsequently, messages were defaulted to Unicode for encoding, resulting in more SMS deliveries to end devices.

This PR now pulls in 4.0.4 of the SDK which does the following:

For users, if you are not sure of the encoding type for your Notification message, a static helper method is now available for you to check before sending the notification through the channel.

\Vonage\SMS\Message\SMS::isGsm7($message);

Vonage are committed to users and to Laravel itself to make the best developer experience possible. If there are questions or concerns, hit me up.

driesvints commented 1 year ago

Lgtm. @potsky @ankurk91 @Brenneisen would any of you be able to try this one out before we merge it?

ankurk91 commented 1 year ago

Sure, tomorrow, my morning 👍

SecondeJK commented 1 year ago

I'll need to do a minor version bump tomorrow morning GMT to squash a bug.

SecondeJK commented 1 year ago

Version bump done.

ankurk91 commented 1 year ago

Working fine for me.

potsky commented 1 year ago

Sorry I'm coming after the war but it's good for me too.

@SecondeJK just a feedback : I have no PHP warning if I add the char ç in the message text and ç is not in the default GSM charset.

The message sent with the text type is only in one part and the ç is replace by ? in the received SMS message which is really ok for me.