Command Handler bindings in ServiceProvider

[qrazi]

I kind of assumed it would make sense to create a new ServiceProvider for having all the Command / Handler bindings in one place in the app. I'm using the ::register()-method to set this up.

I saw that the LocatorInterface has an ::addHandlers(array $commandClassToHandlerMap)-method, so in the ::register()-method of the service provider, I type hint for the LocatorInterface, so I can call the ::addHandlers-method.

Excerpt:

/**
     * Bootstrap the application services. Boot happens after all providers have
     * registered. So all bindings have been made and can be used.
     *
     * @return void
     */
    public function boot(LocatorInterface $handlerLocator)
    {
        $handlerLocator->addHandlers([
            GeneratePlayerCommand::class => GeneratePlayerHandler::class,
        ]);
    }

However this does not work. This does not work because for each resolve the IoC container does, it returns a new instance. I.e. the LaravelTacticianServiceProvider::register() method uses $this->app->bind() and not $this->app->singleton(). For the LocatorInterface, MethodNameInflector, CommandNameExtractor and CommandBusInterface I think it makes sense to use the singleton() method, since the same instance should be able to be reused during the request.

Or maybe I am missing a reason why that should not be the case? If I am not missing anything, I could make a PR for this?

[joselfonseca]

Hello!

First of all thanks for using the package, the reason that does not work is because the locator is the way the bus locate the handlers, if you want to use a service provider to map commands and handlers you can do it like this

<?php 

namespace Modules\Futinventory\Providers;

use Illuminate\Support\ServiceProvider;
use Joselfonseca\LaravelTactician\CommandBusInterface;
/**
 * Class CommandsServiceProvider
 * @package Modules\Futinventory\Providers
 */
class CommandsServiceProvider extends ServiceProvider
{
    /**
     * @var array
     */
    protected $commandsHandlers = [
        'Modules\FutInventory\Commands\InstallModule\InstallCommand' => 'Modules\FutInventory\Commands\InstallModule\InstallCommandHandler',
        'Modules\FutInventory\Commands\UninstallModule\UninstallCommand' => 'Modules\FutInventory\Commands\UninstallModule\UninstallCommandHandler',
        'Modules\FutInventory\Commands\UploadEquipmentsViaExcel\UploadEquipmentsCommand' => 'Modules\FutInventory\Commands\UploadEquipmentsViaExcel\UploadEquipmentsCommandHandler',
        'Modules\FutInventory\Commands\ReportSales\ReportSalesCommand' => 'Modules\FutInventory\Commands\ReportSales\ReportSalesCommandHandler'
    ];
    /**
     * Indicates if loading of the provider is deferred.
     *
     * @var bool
     */
    protected $defer = false;
    /**
     * Register the service provider.
     *
     * @return void
     */
    public function register()
    {
    }
    /**
     * Register all the commands and handlers
     */
    public function boot()
    {
        $bus = $this->app->make(CommandBusInterface::class);
        foreach($this->commandsHandlers as $command => $handler)
        {
            $bus->addHandler($command, $handler);
        }
        $this->app->instance('inventory.bus', $bus);
    }
}

Then you can just get that bus from the container like:

$bus = app('inventory.bus');

And you can then start dispatching the commands from there, inventory.bus is just a name i gave it to reference that is the bus that handles some command for an inventory module in a private project i have.

That is one way you can do it. If it does not make sense please let me know and we can discuss it further.

Thanks!

[qrazi]

But that would kind of force me to violate dependency inversion principle, since I would be calling app() inside my method.

Plus could you elaborate why the LaravelTactician\CommandBusInterface needs to be a new instance? When you call dispatch, it creates a new Tactician\CommandBus-instance. So the Tactician\CommandBus-instance will be configured differently throughout the codebase, but the LaravelTactician\CommandBusInterface will always be constructed the same way. So I still do not understand why the ServiceProvider from this package has to use bind():

public function register()
    {
        $this->registerConfig();
        $this->app->bind('Joselfonseca\LaravelTactician\Locator\LocatorInterface', config('laravel-tactician.locator'));
        $this->app->bind('League\Tactician\Handler\MethodNameInflector\MethodNameInflector', config('laravel-tactician.inflector'));
        $this->app->bind('League\Tactician\Handler\CommandNameExtractor\CommandNameExtractor', config('laravel-tactician.extractor'));
        $this->app->bind('Joselfonseca\LaravelTactician\CommandBusInterface', config('laravel-tactician.bus'));

and why it cannot be singleton() right there. Forgive if I'm being totally blind as to why.... :)

[joselfonseca]

Hello,

The reason the main bus is not a singleton is because in large applications we may have a lot of commands and handlers that are not related to each other and that becomes a huge object in time, if you have a modular application you should not share the commands and handlers between modules, the new Command bus instance returned you make a reference to, is the Tactician Bus. So say you have an application with 10 modules and each of them register 20 commands, I did not want to have an object to map 200 commands, and also the tactician bus constructed will keep that in memory.

The package use bind in case you want to change the Locator, Name inflector o command name Extractor you can do it from the config file, you can even change the hole bus if you want to.

I understand your concern about the dependency inversion principle, however you can use a service provider to map commands and handlers or do it per service classes for example, or you can create a singleton from that, it is up to you.

Example using a service class

https://gist.github.com/joselfonseca/24ee0e96666a06b16f92

Examples with a service provider

https://gist.github.com/joselfonseca/c53132658f74419060065c13846e7c06#file-busserviceprovider2-php

This are just some examples, please note that the singleton one i have not test it.

Off course we are open to PR that help us improve, i've been wanting to do an update to this package but have not had the time to do so.

Please let me know your thoughts.

[qrazi]

I understand now why the default is not singleton. Thank you for the explanation.

I am currently actually using the service class route, but I thought it's not great to scatter the command / handler binding across my different service classes. I might reconsider this, or go try you second ServiceProvider option.

If I think I can make a meaningful contribution to the code, I will make a PR.

[joselfonseca]

Sure, any idea is welcome! Thanks!

[proyectotau]

Hi again!

When I use your singleton solution, it doesn't work. Test says:

Symfony\Component\Debug\Exception\FatalThrowableError: Maximum function nesting level of '256' reached, aborting!

This my usage, quite close at yours: $this->app->singleton(CommandBus::class, function (\Illuminate\Contracts\Foundation\Application $app) use ($commandsHandlers) { $bus = $app->make(CommandBus::class); foreach($commandsHandlers as $command => $handler) { $bus->addHandler($command, $handler); } return $bus; });

And later, in the Controller:

public function __construct() { $this->commandBus = resolve(CommandBus::class); }

However, if I change singleton(CommandBus::class, ... to singleton('MyCommandBus', ... and then, in the Controller:

public function __construct() { $this->commandBus = resolve('MyCommandBus'); }

it finally works!!

PS: BTW, service provider solution does work like a charm 'as is', instead ;-)

Great work!! Regards!!

[joselfonseca]

Thanks! Hope it helps your projects in some way!

[raduungurean]

Hi!

This https://gist.github.com/joselfonseca/c53132658f74419060065c13846e7c06#file-busserviceprovider2-php does not seem to be working with Laravel 5.8

Thank you!