[4.x]: Issue saving order histories

ryssbowh commented 1 week ago

What happened?

Description

I'm chasing a bug on a website where paypal users pay for an order, the funds are taken but the order is never completed on the Craft side. The order then stays as cart and is eventually deleted or just stays as cart.

We've got Sentry and it reported this exception which seems relevant as it happens right in the middle of completing an order : craft\services\Users::getUserById(): Argument #1 ($userId) must be of type int, null given

Trace :

"#0 {masked}/vendor/craftcms/commerce/src/services/OrderHistories.php(123): craft\\services\\Users->getUserById()",
"#1 {masked}/vendor/craftcms/commerce/src/elements/Order.php(3588): craft\\commerce\\services\\OrderHistories->createOrderHistoryFromOrder()",
"#2 {masked}/vendor/craftcms/commerce/src/elements/Order.php(2196): craft\\commerce\\elements\\Order->_saveOrderHistory()",
"#3 {masked}/vendor/craftcms/cms/src/services/Elements.php(3461): craft\\commerce\\elements\\Order->afterSave()",
"#4 {masked}/vendor/craftcms/cms/src/services/Elements.php(1113): craft\\services\\Elements->_saveElementInternal()",
"#5 {masked}/vendor/craftcms/commerce/src/elements/Order.php(1743): craft\\services\\Elements->saveElement()",
"#6 {masked}/vendor/craftcms/commerce/src/elements/Order.php(1652): craft\\commerce\\elements\\Order->markAsComplete()",
"#7 {masked}/vendor/craftcms/commerce/src/services/Transactions.php(470): craft\\commerce\\elements\\Order->updateOrderPaidInformation()",
"#8 {masked}/vendor/craftcms/commerce/src/services/Payments.php(616): craft\\commerce\\services\\Transactions->saveTransaction()",
"#9 {masked}/vendor/craftcms/commerce/src/services/Payments.php(641): craft\\commerce\\services\\Payments->_saveTransaction()",
"#10 {masked}/vendor/craftcms/commerce/src/services/Payments.php(437): craft\\commerce\\services\\Payments->_updateTransaction()",
"#11 {masked}/vendor/craftcms/commerce/src/controllers/PaymentsController.php(539): craft\\commerce\\services\\Payments->completePayment()",
"#12 [internal function]: craft\\commerce\\controllers\\PaymentsController->actionCompletePayment()",
"#13 {masked}/vendor/yiisoft/yii2/base/InlineAction.php(57): call_user_func_array()",
"#14 {masked}/vendor/yiisoft/yii2/base/Controller.php(178): yii\\base\\InlineAction->runWithParams()",
"#15 {masked}/vendor/yiisoft/yii2/base/Module.php(552): yii\\base\\Controller->runAction()",
"#16 {masked}/vendor/craftcms/cms/src/web/Application.php(340): yii\\base\\Module->runAction()",
"#17 {masked}/vendor/craftcms/cms/src/web/Application.php(639): craft\\web\\Application->runAction()",
"#18 {masked}/vendor/craftcms/cms/src/web/Application.php(302): craft\\web\\Application->_processActionRequest()",
"#19 {masked}/vendor/yiisoft/yii2/base/Application.php(384): craft\\web\\Application->handleRequest()",
"#20 {masked}/web/index.php(20): yii\\base\\Application->run()",
"#21 {main}"

The piece of code that's responsible for the exception :

$userId = $order->getCustomerId();
// If the user is logged in, use the current user
if (!Craft::$app->request->isConsoleRequest
    && !Craft::$app->getResponse()->isSent
    && (Craft::$app->getSession()->getHasSessionId() || Craft::$app->getSession()->getIsActive())
    && $currentUser = Craft::$app->getUser()->getIdentity()
) {
    $userId = $currentUser->id;
}

$user = Craft::$app->getUsers()->getUserById($userId);

I can never reproduce the issue locally, but throwing an exception manually at the same place results in the same behaviour : funds taken but Craft order not completed.

We have 2 scenarios when paying with paypal, I see errors on both :

Paying on the cart without being logged in : We retrieve the user's address & email from Paypal and update the cart before trying to complete the order. This is done with custom code

Paying on the checkout page where users enter their email address : We update the cart with the user email with a usual update-cart call, so I can't understand why the customer wouldn't be set on the order.

Logically since $order->getCustomerId() can be null it would make sense to check for that before calling Craft::$app->getUsers()->getUserById($userId), and maybe throwing a warning instead ?

Expected behavior

Order history isn't saved and a warning is thrown ?

Craft CMS version

4.11.5

Craft Commerce version

4.6.10

PHP version

8.2

Operating system and version

No response

Database type and version

No response

Image driver and version

No response

Installed plugins and versions

-

linear[bot] commented 1 week ago

PT-2178 [4.x]: Issue saving order histories

lukeholder commented 6 days ago

@ryssbowh it sounds like your custom code might be somehow clearing the user/customer from the cart before it is completed. A cart should not be able to start a payment process without an email (and therefore a user) so it sounds like when it starts payment it exists but is cleared from the cart before the order is completed.

It is hard to say what is happening without seeing your code. Could you expand on what you are doing or share your code?

You can also email your code to supprt@craftcms.com and reference this issue number. Thanks.

nfourtythree commented 5 days ago

Hi @ryssbowh

You are correct in what you are saying that customerId is nullable on the order element (so we have pushed a fix). However, the part of the code you quote is related to creating order history information i.e. when the order status is changing.

An order can only have a status once it is completed and therefore it must have a customer (and so a customer ID), as Luke said, this leads me to believe something else is happening here and you mentioned your custom code. We haven't seen any behaviour the likes of which you mentioned. Is there any further information you could give us relating to the checkout flow that might steer us in the right direction?

Thanks

ryssbowh commented 5 days ago

I have the same feeling that something else is happening, this is a complex website that started on Craft 2 so lots and lots of custom code that isn't the most readable so I'm not going to bother you with sending code for now.

Having Craft commerce not throwing an error is already a big help, thanks for the quick fix.

Do I require the 4.x branch to get that fix on composer ?

craftcms / commerce