CommandHandler avoid endless loop #35

Merged
merged 1 commit into from Oct 13, 2014

Conversation

Projects
None yet
6 participants
@Miliooo
Contributor

Miliooo commented Sep 4, 2014

Background:
I was checking if commands needed to implement a certain interface. Even if it was only an empty one. This should at least force them to be objects.
https://github.com/qandidate-labs/broadway/blob/master/src/Broadway/CommandHandling/CommandHandlerInterface.php
Turned out that wasn't the case. So I wanted to see what would happen if I send a string as a command.

I was testing this on the command line without a framework.
I expected an exception somewhere but I ended up in an endless loop.

If you check the code the get_class command did return an warning (but no exception), neither did the explode function.
This caused me to endlessly loop the method handle.
This pull request fixed that behaviour.

@asm89

This comment has been minimized.

Show comment
Hide comment
@asm89

asm89 Sep 4, 2014

Member

@Miliooo I think you could move the check to the return value of get_class() which returns false if the argument passed was not an object. The function will also trigger an E_WARNING, but that's already the case (and not a big problem imo).

Member

asm89 commented Sep 4, 2014

@Miliooo I think you could move the check to the return value of get_class() which returns false if the argument passed was not an object. The function will also trigger an E_WARNING, but that's already the case (and not a big problem imo).

@Miliooo

This comment has been minimized.

Show comment
Hide comment
@Miliooo

Miliooo Sep 4, 2014

Contributor

Well another option would be to make sure commands are objects by implementing an empty interface Command. At the moment they can be of any type. The reason I coded it this way is that I think this really needs to be addressed.

Commands have a naming convention. They should express intent in their class name. But the current code doesn't even enforce them to be objects. Is this something you guys agree with or is this open for debate?

I could do a pull request for that one. It's not that much code to fix

Contributor

Miliooo commented Sep 4, 2014

Well another option would be to make sure commands are objects by implementing an empty interface Command. At the moment they can be of any type. The reason I coded it this way is that I think this really needs to be addressed.

Commands have a naming convention. They should express intent in their class name. But the current code doesn't even enforce them to be objects. Is this something you guys agree with or is this open for debate?

I could do a pull request for that one. It's not that much code to fix

@Ocramius

View changes

src/Broadway/CommandHandling/CommandHandler.php
@@ -37,8 +37,18 @@ public function handle($command)
private function getHandleMethod($command)
{
+ if (false === is_object($command)) {

This comment has been minimized.

@Ocramius

Ocramius Sep 4, 2014

if (! is_object($command)) {

@Ocramius

Ocramius Sep 4, 2014

if (! is_object($command)) {

@Ocramius

View changes

src/Broadway/CommandHandling/CommandHandler.php
@@ -37,8 +37,18 @@ public function handle($command)
private function getHandleMethod($command)
{
+ if (false === is_object($command)) {
+ throw new \InvalidArgumentException('The convenience based command handler only supports object commands');

This comment has been minimized.

@Ocramius

Ocramius Sep 4, 2014

if possible, throw a specialized exception here. \InvalidArgumentException is not specific enough

@Ocramius

Ocramius Sep 4, 2014

if possible, throw a specialized exception here. \InvalidArgumentException is not specific enough

This comment has been minimized.

@Miliooo

Miliooo Sep 4, 2014

Contributor

Totally agree, up for naming suggestions.

@Miliooo

Miliooo Sep 4, 2014

Contributor

Totally agree, up for naming suggestions.

This comment has been minimized.

@sstok

sstok Sep 4, 2014

http://nl1.php.net/manual/en/class.badmethodcallexception.php ? but not the actual SPL.

Or UnexpectedTypeException?

@sstok

sstok Sep 4, 2014

http://nl1.php.net/manual/en/class.badmethodcallexception.php ? but not the actual SPL.

Or UnexpectedTypeException?

This comment has been minimized.

@Ocramius

Ocramius Sep 4, 2014

@sstok no, exceptions under the Broadway namespace

@Ocramius

Ocramius Sep 4, 2014

@sstok no, exceptions under the Broadway namespace

This comment has been minimized.

@sstok

sstok Sep 4, 2014

True :) but is BadMethodCallException or UnexpectedTypeException a good name?

Other names I think of CommandNotAnObject or WrongCommandObject which seem to most descriptive InvalidCommand .

@sstok

sstok Sep 4, 2014

True :) but is BadMethodCallException or UnexpectedTypeException a good name?

Other names I think of CommandNotAnObject or WrongCommandObject which seem to most descriptive InvalidCommand .

This comment has been minimized.

@Ocramius

Ocramius Sep 4, 2014

namespace Broadway\CommandHandling\Exception { class InvalidArgumentException extends \InvalidArgumentException {} } is also fine

@Ocramius

Ocramius Sep 4, 2014

namespace Broadway\CommandHandling\Exception { class InvalidArgumentException extends \InvalidArgumentException {} } is also fine

This comment has been minimized.

@Miliooo

Miliooo Sep 4, 2014

Contributor

@sstok CommandNotAnObject so this means that our command bus was able to send a command as a string. But only if our system crashes we would prevent that.

So now every listener of a command should check if it's an object. But commands have rules, they HAVE to be objects. The problem is that this code was allowed in the first place.

That's why i believe this system is really fragile . It allows the users to send whatever data they want through the system. That's why I truly believe that we need to implement interfaces for commands and events. They should be empty because the only thing we care about is their intent and that they are objects.

There's never a reason to let a command be an array or a string. Commands are objects and if you don't want to follow that rule. Then write your own CQRS system because well it's just wrong.

Symfony2 security system also demands us that our user model implements a certain securityInterface. How much harm would it do to make users have to implement an empty interface for their commands and events. They are using the system right?

Sorry for my bad english but I hope I make sense. There's no reason to merge this PR if the reason why it happened can easily be solved. Because if we allow all data to pass through, there will be strange errors in many places.

@Miliooo

Miliooo Sep 4, 2014

Contributor

@sstok CommandNotAnObject so this means that our command bus was able to send a command as a string. But only if our system crashes we would prevent that.

So now every listener of a command should check if it's an object. But commands have rules, they HAVE to be objects. The problem is that this code was allowed in the first place.

That's why i believe this system is really fragile . It allows the users to send whatever data they want through the system. That's why I truly believe that we need to implement interfaces for commands and events. They should be empty because the only thing we care about is their intent and that they are objects.

There's never a reason to let a command be an array or a string. Commands are objects and if you don't want to follow that rule. Then write your own CQRS system because well it's just wrong.

Symfony2 security system also demands us that our user model implements a certain securityInterface. How much harm would it do to make users have to implement an empty interface for their commands and events. They are using the system right?

Sorry for my bad english but I hope I make sense. There's no reason to merge this PR if the reason why it happened can easily be solved. Because if we allow all data to pass through, there will be strange errors in many places.

This comment has been minimized.

@sstok

sstok Sep 5, 2014

👍 on using interfaces.

@sstok

sstok Sep 5, 2014

👍 on using interfaces.

@Ocramius

View changes

src/Broadway/CommandHandling/CommandHandler.php
+ $convenience = end($classParts);
+
+ if (empty($convenience) || !is_string($convenience)) {
+ throw new \Exception('Avoid endless loop');

This comment has been minimized.

@Ocramius

Ocramius Sep 4, 2014

Same as above

@Ocramius

Ocramius Sep 4, 2014

Same as above

@Ocramius

View changes

src/Broadway/CommandHandling/CommandHandler.php
- return 'handle' . end($classParts);
+ $convenience = end($classParts);
+
+ if (empty($convenience) || !is_string($convenience)) {

This comment has been minimized.

@Ocramius

Ocramius Sep 4, 2014

can't really be empty, unless someone has used class_alias(), in which case he deserves a slow and painful death by bugs.

@Ocramius

Ocramius Sep 4, 2014

can't really be empty, unless someone has used class_alias(), in which case he deserves a slow and painful death by bugs.

This comment has been minimized.

@Miliooo

Miliooo Sep 4, 2014

Contributor

Well i agree maybe I overdid the chechs a little, I can't write an unit test to get to that check. But endless loops are nasty

@Miliooo

Miliooo Sep 4, 2014

Contributor

Well i agree maybe I overdid the chechs a little, I can't write an unit test to get to that check. But endless loops are nasty

@Ocramius

View changes

test/Broadway/CommandHandling/CommandHandlerTest.php
+ * @test
+ *
+ * @dataProvider unresolvableCommands
+ * @expectedException \InvalidArgumentException

This comment has been minimized.

@Ocramius

Ocramius Sep 4, 2014

Should probably use setExpectedException() after line 39 instead (assuming the command constructor may throw an exception for any reason)

@Ocramius

Ocramius Sep 4, 2014

Should probably use setExpectedException() after line 39 instead (assuming the command constructor may throw an exception for any reason)

@asm89

This comment has been minimized.

Show comment
Hide comment
@asm89

asm89 Sep 5, 2014

Member

👎 on adding an interface at this point in time.

Member

asm89 commented Sep 5, 2014

👎 on adding an interface at this point in time.

@Miliooo Miliooo referenced this pull request Sep 5, 2014

Closed

CommandBusCommandAware #38

@Miliooo

This comment has been minimized.

Show comment
Hide comment
@Miliooo

Miliooo Sep 7, 2014

Contributor

Updated pull request.

  • Now it uses a custom exception.
  • But maybe it's still better not to merge this. Introducing checks if commands are objects everywhere because you never check the type will create some really stupid checks around simple code. And this is one of them.
Contributor

Miliooo commented Sep 7, 2014

Updated pull request.

  • Now it uses a custom exception.
  • But maybe it's still better not to merge this. Introducing checks if commands are objects everywhere because you never check the type will create some really stupid checks around simple code. And this is one of them.
@asm89

View changes

src/Broadway/CommandHandling/Exceptions/CommandNotAnObjectException.php
+ * file that was distributed with this source code.
+ */
+
+namespace Broadway\CommandHandling\Exceptions;

This comment has been minimized.

@asm89

asm89 Sep 10, 2014

Member

Exception?

@asm89

asm89 Sep 10, 2014

Member

Exception?

@Miliooo

This comment has been minimized.

Show comment
Hide comment
@Miliooo

Miliooo Sep 23, 2014

Contributor

Do I need to update this pull request or do we agree that we should not care about this since commands are by convention objects? @asm89 @wjzijderveld

Contributor

Miliooo commented Sep 23, 2014

Do I need to update this pull request or do we agree that we should not care about this since commands are by convention objects? @asm89 @wjzijderveld

@wjzijderveld

This comment has been minimized.

Show comment
Hide comment
@wjzijderveld

wjzijderveld Oct 10, 2014

Member

@Miliooo As we discussed on IRC a while back (at least, I thought we did... 😕) Can you update the Exception namespace to singular?
After that its okay to merge in my opinion; ping @qandidate-labs/broadway

Member

wjzijderveld commented Oct 10, 2014

@Miliooo As we discussed on IRC a while back (at least, I thought we did... 😕) Can you update the Exception namespace to singular?
After that its okay to merge in my opinion; ping @qandidate-labs/broadway

@Miliooo

This comment has been minimized.

Show comment
Hide comment
Contributor

Miliooo commented Oct 10, 2014

@wjzijderveld

This comment has been minimized.

Show comment
Hide comment
Member

wjzijderveld commented Oct 13, 2014

👍

@fritsjanb

This comment has been minimized.

Show comment
Hide comment
@fritsjanb

fritsjanb Oct 13, 2014

Contributor

👍

Contributor

fritsjanb commented Oct 13, 2014

👍

fritsjanb added a commit that referenced this pull request Oct 13, 2014

Merge pull request #35 from Miliooo/loop
CommandHandler avoid endless loop

@fritsjanb fritsjanb merged commit 983ea97 into broadway:master Oct 13, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@fritsjanb

This comment has been minimized.

Show comment
Hide comment
@fritsjanb

fritsjanb Oct 13, 2014

Contributor

Thanks @Miliooo

Contributor

fritsjanb commented Oct 13, 2014

Thanks @Miliooo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment