Unnecessary wrapping of exceptions

[BreiteSeite]

Currently, prooph wraps exceptions that occur in the handler itself inside 2 exceptions

See:

This has some disadvantages:

• Middlewares can not react directly on messages that the handler (or subsequent methods in that handle) throws without some additional middleware unpacking it first. This wouldn't be very performant i guess and brings its own challanges. For example you need to be sure your unpacking handler is always configured to be after the CommandMiddleware. And additionally...
• ... you don't know if that message is representing an error that actual happened in proophs service-bus or http-middleware or your own code or code from other libraries.

If you use the service-bus, every exception is wrapped into a prooph-exception, making it very hard to discern between messages originating from your business code or from the prooph library itself.

Also the exception messages are 'dispatch failed' or 'error occured during dispatch'. I don't want to create a huge discussion about naming but i think this also shows that prooph shouldn't wrap them in the first place, because in my opinion, the message dispatch is successful as soon as the handler was found and invoked. Everything after that is not part of the service-bus' dispatching. But thats just more of a side-note or an indicator for me and shouldn't be the main point of discussion.

It's hard to use the middleware approach when a library is suddenly masking every exception you throw (at least when using the command-middleware/service-bus).

I think it would make sense for the service-bus and the command middleware to get more out of the way here and let that are not originated from prooph's own code flow more freely. Prooph has no chance of handling exceptions from business logic from the user of the lib anyway because they mean anything and are out of proophs scope.

Edit: this issue's scope is prooph/service-bus (the first lib that wraps the exception) and also of prooph/http-middleware (that wraps the already wrapped exception again). If we decide to act we should create another separate issue for prooph/service-bus.

[prolic]

ping @codeliner

[codeliner]

IMHO we can change the behavior of the middleware and don't catch and wrap the exception again. It's enough that the service-bus does it.

But I won't change the behavior of the service-bus. First it would be a BC break and second reason is that a CommandDispatchFailed exception includes possibly pending commands. See https://github.com/prooph/service-bus/blob/master/src/CommandBus.php#L108

So there is a reason for this exception. If you want to throw the original exception instead, just unwrap CommandDispatchFailed

Having said this a PR to change behavior of the middleware would be nice.

[BreiteSeite]

But I won't change the behavior of the service-bus. First it would be a BC break and second reason is that a CommandDispatchFailed exception includes possibly pending commands. See https://github.com/prooph/service-bus/blob/master/src/CommandBus.php#L108

I see! My Counterpoints to this:

1. Current implementation seems to allow only 1 command in the commandQueue anyway. It gets added in the dispatch method and is removed from the array a few lines later. It's never possible to add commands from anywhere else. So something like premature optimization/dead code?
2. If this design should continue to exist: I would argue that you can still have that behaviour: f you make it possible to ask the commandQueue for the pending commands (getCommandQueue()), a caller interested in that could catch exceptions when calling dispatch and then just ask the queue what commands are still pending.

Given point 1, i would say the chance that a user of the command-bus is interested in pending commands currently is very very low vs. a very high chance that a user is interested in the exception that was thrown from his layer (or even lower).

But again, in my head a command bus implementation is something that looks for the command class to dispatch, checks for the preconditions (interface is e.g. interface is fulfilled) and then in the very last line calls the dispatch() method which isn't surrounded by any catching (because catching that is - still my opinion - not part of the responsibility of a command queue). Everything before that function call can and should be wrapped and thrown an exception class from the component because then finding that command did fail.

Having said this a PR to change behavior of the middleware would be nice.

I'll try to find the time and prepare something when i have the time. :)

[codeliner]

@BreiteSeite

It's never possible to add commands from anywhere else. So something like premature optimization/dead code?

Nope. I know it is not that easy to identify the use case for this command queue without a deeper understanding of the rest of the system. But you can trust us that we don't keep dead code in such an important component like the command bus. We work on prooph components since years. They are battle-tested and extremely stable (got this feedback from many developers around the globe)

It is actually very simple to add another command. See this workflow:

dispatch command 1 -> transaction( 
  handle -> record event -> store event -> listen on event with process maanger 
  -> call $commandBus->dispatch(command 2) { 
    put command 2 in the queue then return because we are still in the dispatching call of command 1
  }
) 
-> command 2 is in the queue, next while loop iteration -> dispatch command 2 -> transaction(...)

I would argue that you can still have that behaviour: f you make it possible to ask the commandQueue for the pending commands (getCommandQueue()), a caller interested in that could catch exceptions when calling dispatch and then just ask the queue what commands are still pending.

Yes, that would be possible. But again it is a BC break and I've learned from other libraries that it is always a good idea to throw library specific exceptions and use the feature of passing around previous exceptions so that userland code can catch library specific exceptions and deal with them in a sane way. In our case this could mean:

• implement a generic retry mechanism for (pending) commands
• log a failed dispatch along with pending commands

Having said this, I vote again against changing behavior of the command bus.