Default bus setup seems broken ?

[tyx]

Hello guys,

it looks we broke something in latest release.

Default bus is no longer setup if no configuration is defined.

Here is the Integration test that should pass:

    public function testHandleCommandOnDefaultBus()
    {
        // We use default config
        $this->registerService('tactician.test.handler', \League\Tactician\Bundle\Tests\EchoTextHandler::class, [
            ['name' => 'tactician.handler', 'command' => 'League\Tactician\Bundle\Tests\EchoText'],
        ]);

        $this->expectOutputString('Hello world');
        $this->handleCommand('default', \League\Tactician\Bundle\Tests\EchoText::class, ['Hello world']);
    }

and it fails with stack trace

League\Tactician\Bundle\DependencyInjection\InvalidCommandBusId: Could not find a command bus with id 'default'. Valid buses are: 

/home/tyx/www/tactician-bundle/src/DependencyInjection/InvalidCommandBusId.php:10
/home/tyx/www/tactician-bundle/src/DependencyInjection/Compiler/BusBuilder/BusBuilders.php:61
/home/tyx/www/tactician-bundle/src/DependencyInjection/Compiler/BusBuilder/BusBuilders.php:29
/home/tyx/www/tactician-bundle/src/DependencyInjection/Compiler/BusBuilder/BusBuildersFromConfig.php:25
/home/tyx/www/tactician-bundle/src/DependencyInjection/Compiler/CommandHandlerPass.php:27
/home/tyx/www/tactician-bundle/vendor/symfony/dependency-injection/Compiler/Compiler.php:141
/home/tyx/www/tactician-bundle/vendor/symfony/dependency-injection/ContainerBuilder.php:757
/home/tyx/www/tactician-bundle/vendor/symfony/http-kernel/Kernel.php:624
/home/tyx/www/tactician-bundle/vendor/symfony/http-kernel/Kernel.php:137
/home/tyx/www/tactician-bundle/tests/Integration/IntegrationTest.php:64
/home/tyx/www/tactician-bundle/tests/Integration/BasicCommandAndBusMappingTest.php:20

I guess it is because of getExtensionConfig that does not merge extension config with default config but I have no idea how to fix that.

I'm ready to make a PR if anyone else have an idea !

edit: Here is a detailled issue with this behavior (expected, not a bug) : https://github.com/symfony/symfony/issues/19149

[tyx]

From what I read, the best solution would be to register command buses in TacticianExtension with empty locator and fill its locator in the CompilerPass.

In order to do that we will need to get back tactician.commandbus.ids

[chalasr]

I would either use an intermediate parameter (created from the extension, removed from the pass) or store the result of BusBuildersFromConfig::convert() as a synthetic service to be reused in the compiler pass.

[tyx]

both solutions look fine for me.

Synthetic service would be the easiest change

[chalasr]

@tyx Still up for a PR?

[tyx]

Yes ! I'm so ready but don't know the best solution to choose. Synthetic ?

[rosstuck]

Sorry for delay, I'm swamped this week and can't really dig into it until tomorrow at least.

I thought usually the Configuration object was returning default values that prevented this from happening in actual installs though it failed in bundle tests? Either way, I'd rather not bring back tactician.commandbus.ids and passing intermediate state around through container parameters.

Would it make sense to modify BusBuildersFromConfig to always return a default bus builder if none are set in the config, i.e. give it a default state to return?

[tyx]

No problem ;)

Would it make sense to modify BusBuildersFromConfig to always return a default bus builder if none are set in the config, i.e. give it a default state to return?

We could do that also. It should works. I just have the feeling we would do extra work and bring "complexity" over the classic symfony way.

Synthetic service looks more symfony compliant and will allow to remove all default value from BusBuildersFromConfig and let Configuration be the only truth

[rosstuck]

That's okay too. I'll leave it to you and Robin to decide, you're actually acting on this so you've got priority :)

On Nov 9, 2017 11:04 AM, "Timothée Barray" notifications@github.com wrote:

No problem ;)

Would it make sense to modify BusBuildersFromConfig to always return a default bus builder if none are set in the config, i.e. give it a default state to return?

We could do that also. It should works. I just have the feeling we would do extra work and bring "complexity" over the classic symfony way.

Synthetic service looks more symfony compliant and will allow to remove all default value from BusBuildersFromConfig

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/thephpleague/tactician-bundle/issues/80#issuecomment-343106355, or mute the thread https://github.com/notifications/unsubscribe-auth/AAI9TlEh7GvD9V-Kg6tM2q158ZaiwWrlks5s0s47gaJpZM4QTEvq .

[chalasr]

let Configuration be the only truth

👍

@tyx the synthetic approach won't work, using a synthetic service during compilation is not supported (only definitions are managed, see https://github.com/symfony/symfony/pull/19715). We need a parameter containing the merged config, to be removed once used by the pass (via $container->getParameterBag()->remove('...');. Some existing tests will probably break due they don't register the bundle extension and the pass will now require it. I suggest to keep the current value as fallback (i.e. if the param exists then use it, otherwise use the raw config as today).