mezzio / mezzio-swoole

Swoole support for Mezzio
https://docs.mezzio.dev/mezzio-swoole/
BSD 3-Clause "New" or "Revised" License
89 stars 28 forks source link

Make EventDispatcherInterface real for the sake of DI #132

Closed kirkmadera closed 4 months ago

kirkmadera commented 4 months ago

Feature Request

Make the Mezzio\Swoole\Event\EventDispatcherInterface compatible with Laminas\ServiceManager\AbstractFactory\ReflectionBasedAbstractFactory or similar concept DI factory.

Q A
New Feature no
RFC no
BC Break no

Summary

In the Message Handler example, Psr\EventDispatcher\EventDispatcherInterface is used in the constructor. The MessageHandlerFactory in the example injects an instance of Psr\EventDispatcher\EventDispatcherInterface implying that the Psr\EventDispatcher\EventDispatcherInterface service name should be overridden in DI for Mezzio Swoole. I don't think that is the intent. The factory in the example should instead get an instance of Mezzio\Swoole\Event\EventDispatcher from the container.

That said, any class using this strategy will require a factory so that the correct instance of Psr\EventDispatcher\EventDispatcherInterface can be set. The ReflectionBasedAbstractFactory cannot determine this. If you make Mezzio\Swoole\Event\EventDispatcherInterface extend Psr\EventDispatcher\EventDispatcherInterface and make Mezzio\Swoole\Event\EventDispatcher implement Mezzio\Swoole\Event\EventDispatcherInterface, then Mezzio\Swoole\Event\EventDispatcherInterface could be used in the constructor and the ReflectionBasedAbstractFactory could not target these correctly without needing to make a factory per class.

I use this strategy wherever I can to reduce boilerplate code. It convolutes the constructor slightly by not directly having Psr\EventDispatcher\EventDispatcherInterface, but to me, it is worthwhile to allow for reflection based DI strategies to handle this.

froschdesign commented 4 months ago

If I have followed your explanation correctly, then an alias should help: https://docs.laminas.dev/laminas-servicemanager/v4/configuring-the-service-manager/#aliases

kirkmadera commented 4 months ago

The alias of Mezzio\Swoole\Event\EventDispatcherInterface to Mezzio\Swoole\Event\EventDispatcherFactory is already present in Mezzio\Swoole\ConfigProvider. Which means that I can create a factory class an get Mezzio\Swoole\Event\EventDispatcherInterface from the container. But I still have to create the factory.

I can avoid creating the factory altogether, if the __constructor directly asks for Mezzio\Swoole\Event\EventDispatcherInterface instead of Psr\EventDispatcher\EventDispatcherInterface.

This is sort of a design decision. It mucks up the constructor a bit since Mezzio\Swoole\Event\EventDispatcherInterface adds no functionality and is just a marker interface. But it's helpful in reducing the boilerplate code of factories.

kirkmadera commented 4 months ago

I noticed this statement in https://mwop.net/blog/2022-01-21-openswoole-timer-cron.html that @weierophinney does alias the PSR-14 interface directly. I am guessing that is why it is like that in the docs.

To allow these "workflow" events to be separate from the application if desired, we register a Mezzio\Swoole\Event\EventDispatcherInterface service that returns a discrete PSR-14 event dispatcher implementation. I generally alias this to the PSR-14 interface, so I can use the same instance for application events.

Another design decision if you want to recommend that or not, but it should be made clear in the docs also. I don't have a strong opinion either way and could be on board with aliasing it entirely since it's really the app's event manager in this case.

It may be an anti-pattern to have an app level event manager. I like the idea of having an event manager tied to specific logic domains. I have avoided a global event manager so far and it has worked for us.

froschdesign commented 4 months ago

The alias of Mezzio\Swoole\Event\EventDispatcherInterface to Mezzio\Swoole\Event\EventDispatcherFactory is already present in Mezzio\Swoole\ConfigProvider.

This not an alias:

https://github.com/mezzio/mezzio-swoole/blob/3ab78a398e95c80ffd55f39e6f98b55da6a09b7b/src/ConfigProvider.php#L154-L159

Which means that I can create a factory class an get Mezzio\Swoole\Event\EventDispatcherInterface from the container. But I still have to create the factory.

Create an alias for Psr\EventDispatcher\EventDispatcherInterface which maps to Mezzio\Swoole\Event\EventDispatcherInterface. Compare with:

https://github.com/mezzio/mezzio-swoole/blob/3ab78a398e95c80ffd55f39e6f98b55da6a09b7b/src/ConfigProvider.php#L182-L187


But please note, I don't use Swoole, so I can only answer in general or be wrong.

kirkmadera commented 4 months ago

If the recommendation is to override EventDispatcherInterface directly, this is not relevant. Closing

froschdesign commented 4 months ago

@kirkmadera No, nothing needs to be overwritten, add another alias to your configuration:

return [
    'dependencies' => [
        'aliases' => [
            Psr\EventDispatcher\EventDispatcherInterface => Mezzio\Swoole\Event\EventDispatcherInterface,
        ],
        // …
    ],
];

Then the ReflectionBasedAbstractFactory from laminas-servicemanager can resolve the service under the name Psr\EventDispatcher\EventDispatcherInterface.

See the link from my first comment above: https://docs.laminas.dev/laminas-servicemanager/v4/configuring-the-service-manager/#aliases