Decouple stock data and prices from product to enable versioning of products

[EinShoppo]

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

Hi!

I noticed that there are two problems regarding the product stock management in case of activated versioning of DataObjects:

• If you track the inventory of products, all products of an order are saved because the values for onHand and onHold are changed. In case of activated versioning of DataObjects every order results in creation of a new version of the product. Since Pimcore saves the versions on filesystem or as blob data in database, this procedure can have a quite big impact on performance. Also, it may result in a huge amount of versions. I think Coreshop should stop creating a version of product on order creation.
• The second problem relates to the first one: CoreShop saves product's stock data directly on its DataObject. That means versioning of products is unuseable because switching to a previously scheduled version also changes the product's stock quantity. So I think CoreShop should store product stock data within a separate class.
  -- Example:
  ---We have a published product with onHand = 100
  ---We schedule a version of the product to become published 01/01/2024
  ---We sell 20 pieces of the product, so the onHand value is 80.
  ---On 01/01/2024 the scheduled version is published with onHand = 100
• We should also consider saving prices within a separate price class. If prices are updated via an import, it is unsafe to switch to a previously saved version of a product because it would restore old prices.

What do you think about my suggestions?

Best regards
EinShoppo

[dpfaffenbauer]

CoreShop could introduce a stock table where the data is stored that is disregared from versioning.

[dpfaffenbauer]

@EinShoppo Would you provide a PR?

[solverat]

"CoreShop saves product's stock data directly on its DataObject. That means versioning of products is unuseable because switching to a previously scheduled version also changes the product's stock quantity."

+1

[EinShoppo]

@dpfaffenbauer: As I have already described, the versioning feature of Pimcore is currently not (safely) usable for CoreShop products. However, separating stock data and prices from products is a pretty big change in my opinion - definitely not a minimally invasive one. Therefore, I think the implementation should better be done by the core team, also because some architectural decisions have to be made here.

[dpfaffenbauer]

@EinShoppo I honestly think it is not as big of a deal as you think ;). I would either introduce a doctrine entity to store it, or check if we can overwrite the number type and create a non versionable type.

[dpfaffenbauer]

@EinShoppo One possible solution might be this, WDYT?

<?php

declare(strict_types=1);

/*
 * CoreShop
 *
 * This source file is available under two different licenses:
 *  - GNU General Public License version 3 (GPLv3)
 *  - CoreShop Commercial License (CCL)
 * Full copyright and license information is available in
 * LICENSE.md which is distributed with this source code.
 *
 * @copyright  Copyright (c) CoreShop GmbH (https://www.coreshop.org)
 * @license    https://www.coreshop.org/license     GPLv3 and CCL
 *
 */

namespace CoreShop\Bundle\CoreBundle\EventListener;

use CoreShop\Component\Core\Model\ProductInterface;
use DeepCopy\Matcher\PropertyNameMatcher;
use Doctrine\DBAL\Connection;
use Pimcore\Event\SystemEvents;
use Pimcore\Model\DataObject\ClassDefinition\Data;
use Pimcore\Model\DataObject\Concrete;
use Pimcore\Model\Element\DeepCopy\PimcoreClassDefinitionReplaceFilter;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\EventDispatcher\GenericEvent;

class StockDeepCopyDisableListener implements EventSubscriberInterface
{
    public function __construct(private Connection $connection)
    {
    }

    public static function getSubscribedEvents(): array
    {
        return [
            SystemEvents::SERVICE_PRE_GET_DEEP_COPY => 'addDoctrineCollectionFilter',
        ];
    }

    public function addDoctrineCollectionFilter(GenericEvent $event): void
    {
        $context = $event->getArgument('context');
        $element = $event->getArgument('element');
        $copier = $event->getArgument('copier');

        if (!$element instanceof ProductInterface) {
            return;
        }

        if (!str_starts_with($context['source'], 'Pimcore\Model\Version::')) {
            return;
        }

        $copier->addFilter(
            new PimcoreClassDefinitionReplaceFilter(
                function (Concrete $object, Data $fieldDefinition, $property, $currentValue) {
                    $table = sprintf('object_store_'.$object->getClassId());
                    $result = $this->connection->fetchAssociative(
                        'SELECT onHand FROM '.$table.' WHERE oo_id = ?',
                        [$object->getId()]
                    );

                    return $result['onHand'];
                }
            ),
            new PropertyNameMatcher('onHand')
        );
        $copier->addFilter(
            new PimcoreClassDefinitionReplaceFilter(
                function (Concrete $object, Data $fieldDefinition, $property, $currentValue) {
                    $table = sprintf('object_store_'.$object->getClassId());
                    $result = $this->connection->fetchAssociative(
                        'SELECT onHold FROM '.$table.' WHERE oo_id = ?',
                        [$object->getId()]
                    );

                    return $result['onHold'];
                }
            ),
            new PropertyNameMatcher('onHold')
        );

        $event->setArgument('copier', $copier);
    }
}

[EinShoppo]

Hi @dpfaffenbauer, i will try to look at this tomorrow and test it. until then, thank you very much! the solution is really much shorter than i had thought :-)

[EinShoppo]

Hello @dpfaffenbauer,
unfortunately I didn't get to test your fix last week. As I am on holiday from this week, I have created a task for our project board. However, I can't tell you if/when my colleagues will get to test your changes.
I myself will be able to test the fix at the beginning of August at the earliest after my return.

Sorry and many greetings

[dpfaffenbauer]

@EinShoppo Sure, no worries, I'll keep it open until I have some confirmation

[dpfaffenbauer]

@EinShoppo Any Feedback?

[dpfaffenbauer]

ping @EinShoppo

[dpfaffenbauer]

ping @EinShoppo

[dpfaffenbauer]

as discussed: should still be decoupled since every order makes a save on the product that can cause heavy operations like a cache clear