Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Feb 3, 2023

Conversation

SecondeJK
Copy link
Contributor

@SecondeJK SecondeJK commented Feb 1, 2023

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:

  • Uses the sms() client instead of the legacy message() client.
  • Defaults your message type to text which is GSM-7 encoded
  • Still enables the user to use the Laravel unicode() method to switch if needs be
  • If you send a GSM-7-compatible text as unicode, it will trigger an E_USER_WARNING but no exception, advising you to use the preferred encoding type
  • If you send a GSM-7 message that needs to be Unicode, it will trigger an E_USER_WARNING advising you that the end device is likely to have scrambled characters due to not using the preferred encoding type.

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
Copy link
Member

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

@ankurk91
Copy link
Contributor

ankurk91 commented Feb 2, 2023

Sure, tomorrow, my morning 👍

@SecondeJK
Copy link
Contributor Author

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

@SecondeJK
Copy link
Contributor Author

Version bump done.

@ankurk91
Copy link
Contributor

ankurk91 commented Feb 3, 2023

Working fine for me.

@driesvints driesvints marked this pull request as ready for review February 3, 2023 10:32
Copy link
Member

@driesvints driesvints left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @SecondeJK!

@driesvints driesvints changed the title Switch to sms() Client, improved GSM-7 Handling [3.x] Switch to sms() Client, improved GSM-7 Handling Feb 3, 2023
@potsky
Copy link
Contributor

potsky commented Feb 3, 2023

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.

@taylorotwell taylorotwell merged commit cd789f1 into laravel:3.x Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants