Fix OnEventStrategy #151

mablae · 2017-01-07T05:58:31Z

Closes #150

prolic

Can you also please update the copyright year to 2017 ?

prolic · 2017-01-07T06:05:12Z

src/Plugin/InvokeStrategy/OnEventStrategy.php

+            MessageBus::PRIORITY_INVOKE_HANDLER
+        );
+
+


one new line too much

coveralls · 2017-01-07T06:16:40Z

Coverage remained the same at 99.407% when pulling aa05412 on mablae:fix-invoke-event-bus into bd5dc09 on prooph:develop.

prolic · 2017-01-07T06:20:34Z

LICENSE file is missing update

prolic · 2017-01-07T06:21:22Z

TestCase?

prolic · 2017-01-07T06:24:55Z

src/Plugin/InvokeStrategy/OnEventStrategy.php

+                $handlers = $actionEvent->getParam(EventBus::EVENT_PARAM_EVENT_LISTENERS);
+
+                foreach ($handlers as $handler) {
+                    $this->invoke($handler, $message);


can be simply: $handler->onEvent($message); and method public function invoke($handler, $message): void can be removed.

can be simply: $handler->onEvent($message);

prolic · 2017-01-07T06:26:08Z

LICENSE.txt

@@ -1,4 +1,4 @@
-Copyright (c) 2013, Alexander Miertsch contact@prooph.de
+Copyright (c) 2017, Alexander Miertsch contact@prooph.de


should be:

Copyright (c) 2013-2017, prooph software GmbH
Copyright (c) 2015-2017, Sascha-Oliver Prolic

coveralls · 2017-01-07T06:26:46Z

Coverage remained the same at 99.407% when pulling a9bd5c1 on mablae:fix-invoke-event-bus into bd5dc09 on prooph:develop.

coveralls · 2017-01-07T06:42:18Z

Coverage remained the same at 99.407% when pulling c26c70d on mablae:fix-invoke-event-bus into bd5dc09 on prooph:develop.

coveralls · 2017-01-08T18:16:36Z

Coverage increased (+0.006%) to 99.413% when pulling 8cc2ad9 on mablae:fix-invoke-event-bus into bd5dc09 on prooph:develop.

oqq · 2017-01-09T13:08:25Z

src/Plugin/InvokeStrategy/OnEventStrategy.php

+use Prooph\ServiceBus\MessageBus;
+use Prooph\ServiceBus\Plugin\AbstractPlugin;
+
+class OnEventStrategy extends AbstractPlugin
 {
    /**
     * @param mixed $handler


Unnecessary doc header parameters could be removed.

oqq · 2017-01-09T13:10:48Z

src/Plugin/InvokeStrategy/OnEventStrategy.php

+use Prooph\ServiceBus\MessageBus;
+use Prooph\ServiceBus\Plugin\AbstractPlugin;
+
+class OnEventStrategy extends AbstractPlugin


This class could be final, i guess.

yes, final class please

oqq · 2017-01-09T13:29:58Z

tests/Plugin/InvokeStrategy/OnEventStrategyTest.php

+        $onEventStrategy->attachToMessageBus($bus->reveal());
+
+        $bus->attach(Argument::type('string'), Argument::type('callable'), Argument::type('integer'))->shouldHaveBeenCalled();
+        $prophet->checkPredictions();


Instead of manuell run checkPredictions(), you could run

$bus->attach(Argument::type('string'), Argument::type('callable'), Argument::type('integer')) ->shouldBeCalled() ->willReturn(new DefaultListenerHandler(function () {}));

Then checkPredictions() would be executed from PHPUnit.

@oqq Thank you, I will try it!

oqq · 2017-01-09T13:31:02Z

tests/Plugin/InvokeStrategy/OnEventStrategyTest.php

+    {
+        $onEventStrategy = new OnEventStrategy();
+
+        $prophet = new \Prophecy\Prophet();


This line is unnecessary. See comment below.

oqq · 2017-01-09T13:31:34Z

tests/Plugin/InvokeStrategy/OnEventStrategyTest.php

+
+        $prophet = new \Prophecy\Prophet();
+
+        $bus = $prophet->prophesize(EventBus::class);


Could be $this->prophesize(). See comment below.

prolic

only some last minor things

prolic · 2017-01-09T14:26:39Z

src/Plugin/InvokeStrategy/OnEventStrategy.php

+use Prooph\ServiceBus\MessageBus;
+use Prooph\ServiceBus\Plugin\AbstractPlugin;
+
+class OnEventStrategy extends AbstractPlugin


yes, final class please

prolic · 2017-01-09T14:27:20Z

src/Plugin/InvokeStrategy/OnEventStrategy.php

@@ -22,4 +27,22 @@ public function invoke($handler, $message): void
    {
        $handler->onEvent($message);


remove this method

Then I need to adjust the testcases, too and mock the handler->onEvent() call, too?

prolic · 2017-01-09T14:27:48Z

src/Plugin/InvokeStrategy/OnEventStrategy.php

+                $handlers = $actionEvent->getParam(EventBus::EVENT_PARAM_EVENT_LISTENERS);
+
+                foreach ($handlers as $handler) {
+                    $this->invoke($handler, $message);


can be simply: $handler->onEvent($message);

prolic · 2017-01-09T14:28:19Z

tests/Plugin/InvokeStrategy/OnEventStrategyTest.php

+                new DefaultListenerHandler(
+                    function () {
+
+                    }


function () {}: void

{} on the same line would be against PSR-2 (not sure whether this repository is following PSR-2).

Yeah, php-cs-fixer would complain then, been there already :)

coveralls · 2017-01-09T20:03:23Z

Coverage increased (+0.009%) to 99.416% when pulling 34e7fb6 on mablae:fix-invoke-event-bus into bd5dc09 on prooph:develop.

mablae added 2 commits January 7, 2017 06:54

Closes prooph#150

ed76d53

Missing import

f210fcf

prolic requested changes Jan 7, 2017

View reviewed changes

src/Plugin/InvokeStrategy/OnEventStrategy.php

MessageBus::PRIORITY_INVOKE_HANDLER

);

Copy link

Member

prolic Jan 7, 2017

Choose a reason for hiding this comment

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

one new line too much

prolic added the bug label Jan 7, 2017

prolic self-assigned this Jan 7, 2017

prolic added this to the 6.0 Release milestone Jan 7, 2017

mablae added 2 commits January 7, 2017 07:08

Fix CS

4cd1d7c

Update Copyright Date in LICENCE

a9bd5c1

prolic reviewed Jan 7, 2017

View reviewed changes

Fix LICENCE headers

c26c70d

mablae mentioned this pull request Jan 8, 2017

Bump service bus package prooph/service-bus-symfony-bundle#10

Merged

Add missing testcase

8cc2ad9

mablae force-pushed the fix-invoke-event-bus branch from 6844740 to 8cc2ad9 Compare January 8, 2017 18:14

oqq reviewed Jan 9, 2017

View reviewed changes

mablae changed the title ~~WIP: Fix OnEventStrategy~~ Fix OnEventStrategy Jan 9, 2017

Better Testcase, thanks @oqq

3a22bc3

prolic requested changes Jan 9, 2017

View reviewed changes

Remove OnEventStrategy::invoke() and make class final

34e7fb6

oqq approved these changes Jan 10, 2017

View reviewed changes

prolic approved these changes Jan 10, 2017

View reviewed changes

prolic merged commit 8dab4aa into prooph:develop Jan 10, 2017

mablae deleted the fix-invoke-event-bus branch January 10, 2017 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix OnEventStrategy #151

Fix OnEventStrategy #151

mablae commented Jan 7, 2017

prolic left a comment

prolic Jan 7, 2017

coveralls commented Jan 7, 2017

prolic commented Jan 7, 2017 •

edited

Loading

prolic commented Jan 7, 2017

prolic Jan 7, 2017

mablae Jan 7, 2017

prolic Jan 9, 2017

mablae Jan 9, 2017

prolic Jan 7, 2017

coveralls commented Jan 7, 2017

coveralls commented Jan 7, 2017

coveralls commented Jan 8, 2017

oqq Jan 9, 2017

oqq Jan 9, 2017

prolic Jan 9, 2017

oqq Jan 9, 2017

mablae Jan 9, 2017

oqq Jan 9, 2017

oqq Jan 9, 2017

prolic left a comment

prolic Jan 9, 2017

prolic Jan 9, 2017

mablae Jan 9, 2017

prolic Jan 9, 2017

prolic Jan 9, 2017

UFOMelkor Jan 9, 2017

mablae Jan 9, 2017

coveralls commented Jan 9, 2017

		@@ -1,4 +1,4 @@
		Copyright (c) 2013, Alexander Miertsch contact@prooph.de
		Copyright (c) 2017, Alexander Miertsch contact@prooph.de


		$prophet = new \Prophecy\Prophet();

		$bus = $prophet->prophesize(EventBus::class);

		@@ -22,4 +27,22 @@ public function invoke($handler, $message): void
		{
		$handler->onEvent($message);

Fix OnEventStrategy #151

Fix OnEventStrategy #151

Conversation

mablae commented Jan 7, 2017

prolic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 7, 2017

prolic commented Jan 7, 2017 • edited Loading

prolic commented Jan 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 7, 2017

coveralls commented Jan 7, 2017

coveralls commented Jan 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prolic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 9, 2017

prolic commented Jan 7, 2017 •

edited

Loading