Fix the type of the message to save a lot of money 💸

[potsky]

Without specifying the type, the default is unicode and messages are split at 70 chars according to the official documentation : https://developer.vonage.com/en/messaging/sms/guides/concatenation-and-encoding

The default type is defined in class VonageMessage message line 26 as public $type = 'text';

In screen below:

• in red, the initial message of 100 chars is split and billed 2 times because of the default type of the vonage package v3 (fixed in v4)
• in green, the same message with the default type is billed normally

[taylorotwell]

The tests seem to be failing.

[potsky]

The tests seem to be failing.

My bad @taylorotwell, tests are fixed now.

[ankurk91]

I want this fix to be released next week.

What about upgrading to vonage-php-sdk-core v4.x?

https://github.com/Vonage/vonage-php-sdk-core/releases/tag/4.0.0

[potsky]

I want this fix to be released next week.

What about upgrading to vonage-php-sdk-core v4.x?

https://github.com/Vonage/vonage-php-sdk-core/releases/tag/4.0.0

🤞 Every day we lose several hundred euros because of this bug 😭 It could be really nice if it is released right now !

[driesvints]

Hi all, I just released this as v3.1.1. Remember that you can always pin a previous version of a package in composer from before the breakage. That way you don't have to wait for a release.

[potsky]

You rock @driesvints, thank you very much!

[Brenneisen]

@potsky @driesvints FYI: This is a little breaking change as the error previously caused unicode messages to be used instead of the default text because the type was not used. Users sending unicode messages will now receive the error message "Sending unicode text SMS without setting the type parameter to 'unicode'". Explicit use of the unicode() method fixes this error in userland.

[ankurk91]

Can we check if message has a Unicode characters before hitting the API ?

[potsky]

@Brenneisen you're right. Given that the type was never been taken into account and always set to unicode, even if you set it to text, it was always set to unicode...

@driesvints I am going to check if we can do this right now

[Brenneisen]

That should be possible. Vonage\SMS\Client has a public isUnicode($message) method.

[potsky]

I'll open a new PR with this check. If the provided type is text and the message contains unicode chars then the type is changed automatically to unicode.

@driesvints for a future upgrade of this package to support SDK v4, keep in mind that the behaviour above is a trick because of a bug before in this package. You will need to document in the UPGRADE.md : "In version 3, setting type to text was a preference and if messages containing unicode chars were sent, they were automatically set to unicode and this could increase your Vonage billing. In version 4, setting type to text and sending unicode message will fail"

[driesvints]

I'm going to revert all of this as well as the original #65 PR that introduced the original issue. We'll revisit this for v4 of the SDK.

[driesvints]

Released v3.1.2 with how things were.

[driesvints]

@potsky @ankurk91 @Brenneisen if you all could confirm things are okay again that'd be great.

[potsky]

@driesvints it is perfect. Message type is kept correctly and if message is fully GSM-charset compliant, it only counts for 1 message in the billing 👍

[potsky]

Moreover, if you put a unicode char in the message and set the message type to text, Vonage will replace the unicode char by ? instead of raising an error.

Thank you 🙏

[ankurk91]

I can also confirm that it is sending 1 SMS instead of 2 now.

[SecondeJK]

@ankurk91 @potsky I'll be issuing a new draft PR shortly to address this. The timing has been a little unfortunate, because the encoding detection was flawed, then removed, then reintroduced. Of the feedback of several users, the new PR will do the following:

1. Switch the channel to use the SMS client rather than the deprecated message client.
2. Bump composer to pull in v4.0 of the Vonage SDK which in turn:
3. defaults your message to GSM-7 (text). You will have the option of choosing Unicode if needed
4. when sending an SMS, the SDK will check if it is a GSM-7 message that should be sent with Unicode, or if sending Unicode when it could have been GSM-7. If a problem is found here the SDK will only trigger a E_USER_WARNING

Cheers,
Jim

[potsky]

Hi @SecondeJK!

Thank you for your message and upgrading to sdk v4 is a good thing.

Just a note: I don't know if you plan to use the built in isUnicode($message) of the SDK methods but there are false in v3 and v4. They do not follow their documentation.

We have developed our own detector following their documentation, if it can help:

    /**
     * @see https://developer.vonage.com/en/messaging/sms/guides/concatenation-and-encoding
     */
    private const ONE_BYTE_CHARS = [
        '!', '"', '#', '$', '%', "'", '(', ')', '*', '+', ',', '-', '.', '/', ':', ';', '<', '=', '>', '?', '@', '_', '¡', '£', '¥', '§', '¿', '&', '¤',
        '0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
        'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z',
        'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z',
        'Ä', 'Å', 'Æ', 'Ç', 'É', 'Ñ', 'Ø', 'ø', 'Ü', 'ß', 'Ö', 'à', 'ä', 'å', 'æ', 'è', 'é', 'ì', 'ñ', 'ò', 'ö', 'ù', 'ü', 'Δ', 'Φ', 'Γ', 'Λ', 'Ω', 'Π', 'Ψ',
        'Σ', 'Θ', 'Ξ', ' ', "\n",
    ];

    private const TWO_BYTES_CHARS = ['|', '^', '€', '{', '}', '[', ']', '~', '\\'];

    public static function checkMessageLength(string $message): void
    {
        $length = 0;
        $charsNotSupported = [];

        foreach (mb_str_split($message) as $char) {
            if (in_array($char, self::ONE_BYTE_CHARS)) {
                ++$length;
            } elseif (in_array($char, self::TWO_BYTES_CHARS)) {
                $length += 2;
            } else {
                $length += 2;
                $charsNotSupported[] = $char;
            }
        }

        if ($length > 160) {
            Debug::log('SMS message is too long', [
                'message' => $message,
                'length' => $length,
                'charsNotSupported' => $charsNotSupported,
            ]);
        } elseif (!empty($charsNotSupported)) {
            Debug::log('SMS chars not supported in GSM charset', [
                'message' => $message,
                'chars' => $charsNotSupported,
            ]);
        }
    }

[SecondeJK]

Thanks for this @potsky
Just FYI, we recognised that the behaviour was wrong - so instead isUnicode() is deleted in v4. Instead we look to see if the message is within the GSM-7 character map (method is called isGsm7(). I've done this with quite a neat regex that has very little overhead when executing