[4.x]: Applying previously saved (invalid) user address to a cart throws exception

[rob-baker-ar]

What happened?

Description

Part of the Commerce / Craft upgrade process from our v3 installation migrated addresses to Craft core. Prior to this, addresses did not get as much validation.

Now, when some logged in users chose an addresses added to the system prior to the v4 upgrade on the address stage of our checkout (an address that happens not to pass the new validation), an exception is thrown. This happens when an address from the user is duplicated to attach to the cart.

Exception trace as follows:

[craft\errors\InvalidElementException] craft\errors\InvalidElementException: Element 10524 could not be duplicated because it doesn't validate. in [...]/vendor/craftcms/cms/src/services/Elements.php:1500
Stack trace:
#0 [...]/vendor/craftcms/commerce/src/controllers/CartController.php(538): craft\services\Elements->duplicateElement()
#1 [...]/vendor/craftcms/commerce/src/controllers/CartController.php(214): craft\commerce\controllers\CartController->_setAddresses()
#2 [internal function]: craft\commerce\controllers\CartController->actionUpdateCart()
#3 [...]/vendor/yiisoft/yii2/base/InlineAction.php(57): call_user_func_array()
#4 [...]/vendor/yiisoft/yii2/base/Controller.php(178): yii\base\InlineAction->runWithParams()
#5 [...]/vendor/yiisoft/yii2/base/Module.php(552): yii\base\Controller->runAction()
#6 [...]/vendor/craftcms/cms/src/web/Application.php(301): yii\base\Module->runAction()
#7 [...]/vendor/yiisoft/yii2/base/Controller.php(215): craft\web\Application->runAction()
#8 [...]/modules/shop/controllers/BasketController.php(91): yii\base\Controller->run()
#9 [internal function]: [CUSTOM MODULE]\BasketController->actionUpdate()
#10 [...]/vendor/yiisoft/yii2/base/InlineAction.php(57): call_user_func_array()
#11 [...]/vendor/yiisoft/yii2/base/Controller.php(178): yii\base\InlineAction->runWithParams()
#12 [...]/vendor/yiisoft/yii2/base/Module.php(552): yii\base\Controller->runAction()
#13 [...]/vendor/craftcms/cms/src/web/Application.php(301): yii\base\Module->runAction()
#14 [...]/vendor/craftcms/cms/src/web/Application.php(625): craft\web\Application->runAction()
#15 [...]/vendor/craftcms/cms/src/web/Application.php(280): craft\web\Application->_processActionRequest()
#16 [...]/vendor/yiisoft/yii2/base/Application.php(384): craft\web\Application->handleRequest()
#17 [...]/web/index.php(19): yii\base\Application->run()
#18 {main}

Note: See the #9 above: [CUSTOM MODULE] - We are currently are using this custom module based controller to catch the exception and return an error response in production. For the purpose of the test for this issue, the entire body of that controller action is:

return $this->run('/commerce/cart/update-cart', func_get_args());

Steps to reproduce

1. Create an order for a logged in user with an address on Commerce v3 that would not pass validation on v4.
2. Upgrade to v4
3. Attempt to checkout (or just update the cart) with the address id created in (1.)

Expected behavior

There should be a way to capture a validation error so we can allow the user to correct the address.

Actual behavior

Exception thrown from controller context so only way I can see to capture the error as the moment would be to implement another controller, invoking the update basket action within a try / catch block and looking into what gets caught.

Craft CMS version

4.3.4

Craft Commerce version

4.2.4

PHP version

8.1.2

Operating system and version

No response

Database type and version

No response

Image driver and version

No response

Installed plugins and versions

-

[lukas-jansen]

Looks a bit similar to #3046 but didn't have the time yet to do further investigation.

[lukeholder]

Could you check out the following to loosen validation:

https://craftcms.com/knowledge-base/removing-commerce-address-validation

We try to reproduce with validation removed though and see if it's a bug.

[rob-baker-ar]

Thanks for looking at this.

Selectively (or entirely) disabling built in address validation as you refer to, does alter the behaviour I am seeing - i.e. saved addresses that fail built in validation throw this exception.

It looks like a validation step before cloning the address would fix this, but I get that might not be ideal conceptually: It's a saved address, it ought to be fine. That would hold true for any addresses created in recent versions but might not hold true for those with upgraded installs / migrated data from older versions.

[atelieridc]

Same issue here. Users/Customers who registered before our store migration to version 4 get an "internal server error" when trying to login or the checkout. We do have a custom module for address field validation.

[pdaleramirez]

I've tested the upgrade without any custom module, there were not errors being thrown. I think disabling custom module prior to upgrade would solve the issue.

[rob-baker-ar]

@pdaleramirez This issue describes a bug that happens when a user address is used on the front-end after an upgrade. Not during the upgrade process itself.

This should not be closed yet!

[pdaleramirez]

@rob-baker-ar I think the custom module is causing the issue. I've tested an address on Commerce 4 without the required attribute such as Address 1 I am not getting that error but a validation notice. We can debug this more by emailing your db in support@craftcms.com

[rob-baker-ar]

@pdaleramirez I do have some custom validation happening in a custom module on addresses. That is currently being invoked like this:

use yii\base\Event;
use yii\base\Model;
use craft\elements\Address;

Event::on(Address::class, Model::EVENT_BEFORE_VALIDATE, function (Event $event) {
    /** @var Address $address */
    $address = $event->sender;
    $validator = new AddressValidator($address);
    $validator->validate();
});

Where AddressValidator is a custom class that performs address validation and calls $address->addError() for the fields that have errors.

I noticed that here the example uses a Model::EVENT_DEFINE_RULES event instead. I had not done it that way as I'm not a fan of the way Yii does model validation via rules. We have other custom validation that hooks into Model::EVENT_BEFORE_VALIDATE for other models / elements without issue - it seems the way addresses get cloned might be different compared to other elements.

If refactoring our custom validation to use the Model::EVENT_DEFINE_RULES event for addresses will fix things, then I can certainly try that - do you think it will? It's a fair amount of work to do that so would like to have an indicator whether it will fix the issue or not. If there is another approach to custom validation that will avoid this issue, please point me in that direction.

[pdaleramirez]

@rob-baker-ar When adding new validation use the ADDRESS::EVENT_DEFINE_RULES Yii2 validation instead. docs here: https://craftcms.com/docs/4.x/extend/extending-system-components.html#custom-validation-rules