Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing a bug when "async_switch" option is used. #65

Merged
merged 8 commits into from
Mar 22, 2018

Conversation

s-code
Copy link
Contributor

@s-code s-code commented Mar 7, 2018

When "async_switch" option is used "$routerArguments" should be passed to the decorated router, but not to the decorator.

@coveralls
Copy link

coveralls commented Mar 7, 2018

Pull Request Test Coverage Report for Build 212

  • 0 of 4 (0.0%) changed or added relevant lines in 1 file are covered.
  • 27 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.05%) to 22.151%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/DependencyInjection/Compiler/RoutePass.php 0 4 0.0%
Files with Coverage Reduction New Missed Lines %
src/DependencyInjection/Compiler/RoutePass.php 27 0.0%
Totals Coverage Status
Change from base Build 200: -0.05%
Covered Lines: 103
Relevant Lines: 465

💛 - Coveralls

@UFOMelkor
Copy link
Member

UFOMelkor commented Mar 12, 2018

I did not use the async switch, but it looks good to me 👍

Do you think you can add a test case for it?
Travis notes about a coding style. Please take a look at the order of the use statements.

The other error is not related to this PR. Feel free to look at it if you want, otherwise I will do.

Also: Is there anything that prevents the usage of has to check whether the async switch is enabled instead of using try-catch?

Sergii Shostak added 2 commits March 20, 2018 16:19
@s-code
Copy link
Contributor Author

s-code commented Mar 20, 2018

@UFOMelkor i've added test cases and replaced try-catch by has checking.

@UFOMelkor
Copy link
Member

The failures in CI have other reasons, will be fixed with #67 .
Unfortunately your test does not cover the issue you mentioned (remove your fix and the tests will still pass).

The bug just appears if the handlers are registered with tags.

I had a look at it and modified as following.
Does this reflect the setup in your application?

prooph_service_bus:
  command_buses:
    command_bus_async:
      router:
        async_switch: async_message_producer

services:
  Acme\RegisterUserHandler:
     class: ProophTest\Bundle\ServiceBus\DependencyInjection\Fixture\Model\AcmeRegisterUserHandler
     public: true
     tags: [{name: 'prooph_service_bus.command_bus_async.route_target', message_detection: true }]
  async_message_producer:
     class: ProophTest\Bundle\ServiceBus\DependencyInjection\Fixture\Model\AsyncMessageProducer
     public: true
    /**
     * @test
     */
    public function it_creates_a_command_bus_with_async_switch_message_router()
    {
        $container = $this->loadContainer('command_bus_async', new PluginsPass(), new RoutePass());

        /** @var AsyncMessageProducer $asyncMessageProducer */
        $asyncMessageProducer = $container->get('async_message_producer');

        /** @var CommandBus $commandBus */
        $commandBus = $container->get('prooph_service_bus.command_bus_async');

        /** @var AcmeRegisterUserHandler $handler */
        $handler = $container->get('Acme\RegisterUserHandler');

        // dispatch an async command
        $commandBus->dispatch(new AsyncAcmeRegisterUserCommand([]));
        self::assertInstanceOf(AsyncAcmeRegisterUserCommand::class, $asyncMessageProducer->lastMessage);

        // dispatch an sync command
        $commandBus->dispatch(new AcmeRegisterUserCommand([]));
        self::assertInstanceOf(AcmeRegisterUserCommand::class, $handler->lastCommand());
    }

@s-code
Copy link
Contributor Author

s-code commented Mar 22, 2018

@UFOMelkor I've made a mistake in fixtures for event bus. Now it covers the issue.
Your modification reflects the setup of our app. Should i apply this changes in my pull request and remove my event bus fixtures?

@s-code
Copy link
Contributor Author

s-code commented Mar 22, 2018

@UFOMelkor i've decided to remove event_bus_async fixtures, so now only command_bus_async fixture is changed

@UFOMelkor
Copy link
Member

Your modification reflects the setup of our app. Should i apply this changes in my pull request and remove my event bus fixtures?

Would be great 👍

@s-code
Copy link
Contributor Author

s-code commented Mar 22, 2018

@UFOMelkor Done, could you check?

Copy link
Member

@UFOMelkor UFOMelkor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one space missing, then we are ready :-)

@@ -42,7 +42,12 @@ public function process(ContainerBuilder $container)
$buses = $container->getParameter('prooph_service_bus.' . $type . '_buses');

foreach ($buses as $name => $bus) {
$router = $container->findDefinition(sprintf('prooph_service_bus.%s.router', $name));
$routerServiceId = sprintf('prooph_service_bus.%s.decorated_router', $name);
if (!$container->has($routerServiceId)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space after !

@s-code
Copy link
Contributor Author

s-code commented Mar 22, 2018

done, thx

@UFOMelkor
Copy link
Member

👍

@UFOMelkor UFOMelkor merged commit 0cbb462 into prooph:master Mar 22, 2018
@UFOMelkor
Copy link
Member

Thank you! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants