RFC: Event focused extension system

[OliverSkroblin]

Description

Currently, there are discussions about moving away from decoration over services and creating a "unified" extension system.

At present, we have services, tagged services, interfaces, adapters, events, etc.

Therefore, in the entire "Better software" discussion, the idea has emerged to enable more through events.

In Shopware 5, there were various events for different situations:

• Notify => Simply an event where you could register to trigger something
• NotifyUntil => Often used for early returns or to replace services
• Filter => To filter results or extend them with data

Hence, I've been considering whether we could replace our service decoration with an event approach. In doing so, I designed this system.

In my opinion, this system has some advantages:

1. Events can be kept backwards compatible more easily than a concrete server implementation
2. In contrast to di-services, events are not callable, which also makes the backward check easier
3. There is a central way to change something via PHP
4. Extensions points are easier to find in code
5. We often do not want to keep backward compatibility for an extended period to avoid being stuck in an "old services" hell. Maintaining multiple events, however, is a low-cost factor for us, in my opinion.
6. We allow more "internal decorations" of private methods

In this context, I would also want to enforce reference documentation, in which all of these extension points must already be provided with tests + API reference documentation. (Ensured via a PHP Stan rule).

Of course, service decoration is still possible in projects, but we would only officially communicate this way. But I would like to replace interfaces and abstract classes and only define this path for PHP extensions.

I would be happy to hear your opinion on this topic.

I implemented a POC for it, which you can checkout and test.

Specification

Definition of an extension

<?php

namespace Shopware\Core\Content\Product\Extension;

use Shopware\Core\Framework\DataAbstractionLayer\Search\Criteria;
use Shopware\Core\Framework\DataAbstractionLayer\Search\IdSearchResult;
use Shopware\Core\Framework\Extensions\Extension;
use Shopware\Core\System\SalesChannel\SalesChannelContext;

/**
 * @public this class is used as type-hint for all event listeners, so the class string is "public consumable" API
 *
 * @title Determination of the listing product ids
 * @description This event allows intercepting the listing process, when the product ids should be determined for the current category page and the applied filter.
 * @example("ResolveListingIdsByYourOwnExample.php")
 */
final class ResolveListingLoaderIdsExtension extends Extension
{
    /**
     * {@inheritdoc}
     */
    public static function name(): string
    {
        return 'listing-loader.resolve-listing-ids';
    }

    /**
     * @public
     * @description The id search result acts as a DTO for the product ids. It contains the ids and the total count of the products. Also all added extensions will be transferred to the next event and to the final listing result object
     */
    public function result(): IdSearchResult
    {
        return $this->result;
    }

    /**
     * @internal shopware owns the __constructor, but the properties are public API
     */
    public function __construct(
        /**
         * @public
         * @description The criteria which should be used to load the products. Is also contains the selected customer filter
         */
        public Criteria $criteria,

        /**
         * @public
         * @description Allows you to access to the current customer/sales-channel context
         */
        public SalesChannelContext $context
    ) {
    }
}

The extension dispatcher

<?php

namespace Shopware\Core\Framework\Extensions;

use Symfony\Component\EventDispatcher\EventDispatcherInterface;

final class ExtensionDispatcher
{
    /**
     * @internal
     */
    public function __construct(
        private readonly EventDispatcherInterface $dispatcher
    ) {
    }

    public function publish(Extension $extension, callable $function): mixed
    {
        $this->dispatcher->dispatch($extension, $extension::pre());

        if (!$extension->isPropagationStopped()) {
            $extension->result = $function($extension);
        }

        $extension->resetPropagation();

        $this->dispatcher->dispatch($extension, $extension::post());

        return $extension->result();
    }
}

How we call it

class ProductListingLoader
{
    public function load(Criteria $origin, SalesChannelContext $context): EntitySearchResult
    {
        // ...

        $ids = $this->resolveIds($criteria, $context);
        
        // ...
    }

    private function resolveIds(Criteria $criteria, SalesChannelContext $context): IdSearchResult
    {
        return $this->extensions->publish(
            extension: new ResolveListingLoaderIdsExtension($criteria, $context),
            function: function(ResolveListingLoaderIdsExtension $event): IdSearchResult {
                $this->addGrouping($event->criteria);

                $this->handleAvailableStock($event->criteria, $event->context);

                return $this->productRepository->searchIds($event->criteria, $event->context);
            }
        );
    }

For more details, take a look into the linked merge request

Merry Christmas and a Happy New Year 🎄 🥳

[JoshuaBehrens]

Hi I really get the advantages regarding the compatibility. In HEPTAconnect we decided to have a payload/params object, that can be extended at any point to keep service decoration flexible. We took the inspiration from event handlers from .NET though :D

Today I just wrote a service decorator and that is why I went here to this discussion. The decorator basically looks like this:

<?php

declare(strict_types=1);

use Shopware\Core\System\SalesChannel\BaseContext;
use Shopware\Core\System\SalesChannel\Context\AbstractBaseContextFactory;
use Shopware\Core\System\SalesChannel\Context\SalesChannelContextService;
use Shopware\Core\System\SalesChannel\SalesChannelException;

final class CustomerCurrencyBaseContextFactory extends AbstractBaseContextFactory
{
    public function __construct(
        private readonly AbstractBaseContextFactory $decorated,
    ) {
    }

    public function create(string $salesChannelId, array $options = []): BaseContext
    {
        $originalOptions = $options;

        if (\array_key_exists(SalesChannelContextService::CUSTOMER_ID, $options)) {
            $customerId = $options[SalesChannelContextService::CUSTOMER_ID];
            // change $options depending on customer
        }

        try {
            return $this->decorated->create($salesChannelId, $options);
        } catch (SalesChannelException $exception) {
            if ($exception->getErrorCode() !== 'MY ERROR') {
                throw $exception;
            }

            // ╮(￣ω￣;)╭ at least we tried
            return $this->decorated->create($salesChannelId, $originalOptions);
        }
    }
}

And this is something I cannot do with events but it is something I (have to) do regularly.

[JoshuaBehrens]

Also something, that we sometimes do is to remove decorators because they are not what we need in a project. Removing a decorator is easy. Drop the service definition. Removing a single event listener is not very easy.

[OliverSkroblin]

That is easy, i implement a helper for that:

shopware/src/Core/Framework/DependencyInjection/CompilerPass/RemoveEventListener.php

Line 14 in 4ee545f

public static function remove(ContainerBuilder $builder, string $serviceId, array $remove): void

RemoveEventListener::remove($builder, 'class-string', [['event' => 'event-1', 'method' => 'method-1']]);

[JoshuaBehrens]

Stack traces. Events in shopware are in such a mess in regards of stacktraces. It is so easy to be in an event listener, that triggers other event dispatches, that will bloat your stack depth so much. I just work on the decorator mentioned above and we need 8 layers
to dispatch an event. This bloats the trace of exceptions, within debugging and performance traces. Not only humans need to look closely what they eventually need of the stacktrace, that helps them. Some tools will record performance by class name / method name.

[OliverSkroblin]

Yes, I agree with you, the stack tracing of event subscribers is not as good as that of the normal function stack.

[OliverSkroblin]

I noticed one thing today that doesn't exist in an event-based approach: Naming collisions.

I just added a function into the MigrationStep called addColumn... If someone else already has such a function, it crashes.

In itself, adding a protected (or private) function in one of our classes wouldn't be a break. And i could release this inside a patch release. However, if third-party "plugins" extend this class, collisions can occur, resulting in fatal PHP errors during the update of a patch release :/ .