Inconsistent Cart Item <> Cart Storage Behavior

[solverat]

Q	A
Bug report?	yes
Feature request?	no
BC Break report?	no
RFC?	no

https://github.com/coreshop/CoreShop/blob/836bb013a1163c6471aa8b288998a6483775bb60/src/CoreShop/Bundle/OrderBundle/Manager/CartManager.php#L74-L84

After all items have been persisted, the cart processor kicks in:

https://github.com/coreshop/CoreShop/blob/836bb013a1163c6471aa8b288998a6483775bb60/src/CoreShop/Bundle/OrderBundle/Manager/CartManager.php#L87

In some cases, the cart processor throws an exception. After that, the customer cannot add any items anymore, because of this:

Duplicate full path [ /coreshop/carts/2023/08/24/cart64e73dc0a5d463.86004598/items/1 ] - cannot save object

It's quite obvious why this is happening: The cart object does not hold any items in its relations (because the cart persistence crashed), but physically they exist.

Solution:

• Check the path via DataObject\Service::pathExists()
• OR: Create an unique key (1-xy)

[dpfaffenbauer]

why does to cart processor fail? thats the main problem here

[solverat]

Yes. We can't tell why it's happening at now, but there are a LOT of processes (all cart processors + a lot of post update events).

Sometimes it's even okay, if there is an exception. So, after the customer has been informed by any kind of error (and changed something - custom additional text for example), it is not possible to re-add an item ever again.

[dpfaffenbauer]

welcome to a world were Pimcore doesn't allow transactions with dataobjects and concurrency ;)

I honestly don't know how to properly solve it. We could either try-catch the cart processors and save the cart afterwards to get the relations in order. but what side-effects does that come with?

[solverat]

welcome to a world were Pimcore doesn't allow transactions with dataobjects and concurrency ;)

Yes.... :angry:

Speaking of transactions, :), the craziest thing we could do:

$this->connection->beginTransaction();

try {

   // save/create all order items
   // save/process cart

    $this->connection->commit();

} catch (\Throwable $e) {

    if ($this->connection->isTransactionActive()) {
        $this->connection->rollBack();
    }

    throw $e;
}

The simplest (and probably the safest) thing, we could do: suffix order items with a unique id (1-64e747e9d362d, 2-34e747e9d362d, ...).

---

Since it's quire urgent, I've implemented a quick fix:

<?php

class CartItemPathListener implements EventSubscriberInterface
{
    public static function getSubscribedEvents(): array
    {
        return [
            DataObjectEvents::PRE_ADD => 'checkCartItem'
        ];
    }

    public function checkCartItem(DataObjectEvent $event): void
    {
        $object = $event->getObject();

        if (!$object instanceof OrderItemInterface) {
            return;
        }

        if ($object->getId() !== null) {
            return;
        }

        $fullPath = sprintf('%s/%s', $object->getParent()?->getRealFullPath(), $object->getKey());

        if ($fullPath === '/' || !Service::pathExists($fullPath)) {
            return;
        }

        $orphanedOrderItem = DataObject::getByPath($fullPath);

        $orphanedOrderItem->setKey(sprintf('%s-%s-%s', 'orphan', $orphanedOrderItem->getkey(), uniqid('', false)));
        $orphanedOrderItem->setPublished(false);
        $orphanedOrderItem->save();
    }
}

[dpfaffenbauer]

We had the transaction before but removed it due to performance issues with Concurrency:

https://github.com/coreshop/CoreShop/pull/2250

Problem was: Pimcore creates a transaction for every save on a DataObject and therefore Locks the database during it, since the Cart Saving can take a couple of milliseconds it affects other concurrent requests as well. removing this, allows us to save more carts concurrently.

I'd rather go with a try/catch and cleanup afterwards, WDYT?

[dpfaffenbauer]

@solverat ping?

[solverat]

Ah yes...

What does the "cleanup" exactly mean? Removing the old one, if an exception has been thrown? (Where we need to check it anyway, if we're dealing with a duplicate path or another (maybe valid) exception.

[dpfaffenbauer]

Thats a very good question ;). If it is a duplicate key exception, it is quite easy. Just rename it or delete the other one and re-run the process. if it's other exceptions, we might introduce a Event to handle it.

[solverat]

I would go with my suggested solution

$fullPath = sprintf('%s/%s', $object->getParent()?->getRealFullPath(), $object->getKey());

if ($fullPath === '/' || !Service::pathExists($fullPath)) {
    return;
}

$orphanedOrderItem = DataObject::getByPath($fullPath);

$orphanedOrderItem->setKey(sprintf('%s-%s-%s', 'orphan', $orphanedOrderItem->getkey(), uniqid('', false)));
$orphanedOrderItem->setPublished(false);
$orphanedOrderItem->save();

I implemented it via event listener (to fix it the fast way :)), in core there is probably a more elegant way to do it.

[dpfaffenbauer]

@solverat could you either provide me a snippet on how I can test the failing items or could you create a PR?

[dlhck]

We also stumbled over this problem. I am trying to provide something, but it happens very inconsistently without any pattern.

[dpfaffenbauer]

@davidhoeck did you find a solution?