Improper handling of null values causing order rollbacks

[brian-labelle]

In the following class and method, null values are not properly handled. This can cause an exception to occur on order place, which will cause the order transaction to rollback and not be saved to the database. Depending on store setup, this can also mean that the a transaction was already authorized and payment captured, but the order record is not inserted into the sales_order table.

See: https://github.com/dotmailer/dotmailer-magento2-extension-sms/blame/7b78e4584159e7975168b3969ef922ed9f07089f/Model/Number/Utility.php#L16

str_replace(' ', '', $number); is not capable of handling a null value. Its third parameter can only accept string|array. Allowing null values to be passed into the method prepareMobileNumberForStorage but not checking for null before using it in string replace is causing an exception.

https://www.php.net/manual/en/function.str-replace.php

Additional details:

• There is no check here to ensure that the billing address actually has the telephone value https://github.com/dotmailer/dotmailer-magento2-extension-sms/blob/7b78e4584159e7975168b3969ef922ed9f07089f/Model/Queue/OrderItem/OrderItemNotificationEnqueuer.php#L85C1-L86C1

[sta1r]

Hi - thanks, we'll take a look.

[sta1r]

Casting to string like this:

public function prepareMobileNumberForStorage(?string $number)
{
    return (int) str_replace(' ', '', (string) $number);
}

will fix this.

I'm just trying to understand your setup. You have: a) transactional SMS enabled b) at least one type of notification enabled c) phone number validation disabled

Is this correct? Is this because you want it to be optional?

The validation in our module is intended to prevent empty numbers - because if they are empty, and assuming we fix this error, phone number will be 0 and the transactional SMS cannot be sent.

[brian-labelle]

I'm just trying to understand your setup. You have: a) transactional SMS enabled b) at least one type of notification enabled c) phone number validation disabled

Is this correct? Is this because you want it to be optional?

The validation in our module is intended to prevent empty numbers - because if they are empty, and assuming we fix this error, phone number will be 0 and the transactional SMS cannot be sent.

The setup looks like this:

• Transaction SMS enabled
• New Order SMS enabled
• Phone number validation is enabled
• The native Customer Address Telephone field is set to not required, and there are several customers with addresses that do not have telephone numbers. So this affects both orders placed via checkout as well as orders placed via rest API through third party subscriptions modules -- for customers who either do not supply a phone number on billing address on checkout, or do not have a telephone value on a saved address
• Adobe Commerce 2.4.7
• dotdigital/dotdigital-magento2-extension-sms version 2.3.1

We had to temporarily disable all transactional SMS in order to prevent the TypeError from occurring: TypeError: str_replace(): Argument #3 ($subject) must be of type array|string, null given in {redacted}/vendor/dotdigital/dotdigital-magento2-extension-sms/Model/Number/Utility.php:18

• Disabling all transactional SMS is our temporary mitigation

Your proposed change looks good

[sta1r]

OK, we're going to exit as well in the OrderItemNotificationEnqueuer if no number is found (per your suggestion). Releasing next week, will confirm here.

Thanks again for this and apologies for any inconvenience!

[sta1r]

Package submitted to Adobe, should be available tomorrow or Wednesday all being well.