Proposal: Add interfaces for classes when using an InvokeStrategy
Closed this issue · 24 comments
If we have a EventRouter with a mapping e.g.
and we use for example the OnEventStrategy wants to call the onEvent() method, even if the class just uses __invoke() as callable.
So currently it is not possible to use more than one InvokeStrategy. If we use a specific InvokeStrategy we are bound to this InvokeStrategy
I propose to provide an interface for each InvokeStrategy (for the given methods e.g.)
interface UseHandleCommandStrategy {
public interface handle(Message $message) : void;
}
interface UseOnEventStrategy {
public interface onEvent(Message $message) : void;
}
interface UseFinderInvokeStrategy {
public interface find(Message $message, $deferred) : void;
}So we can build in a check if the handler wants to use a specific strategy:
foreach ($handlers as $handler) {
if ($handler instanceof UseOnEventStrategy) {
$handler->onEvent($message);
}
}Handler not implementing the interface will just be skipped, so multiple strategies are possible then.
I don't think it's necessary to have this in the core library. The message bus is flexible enough so that you can achieve those things in userland code. So to me, it would be a nice blog post about how you can build stuff around it, but nothing for prooph/service-bus itself.
But that's just my opinion, let's see what others have to say about this.
i feel it is justified. The strategies can use them with instanceof to make sure it's target is valid.
This is a bit defensive. I have noticed configuration problems aren't handled throughout the framework. I feel that is a deliberate descision. I understand that checks everywhere will have a slight performance impact. Is that the main rational?
The problem I have with it is then when everything works you don't want it. But as soon as you start refactoring or upgrading it it really helpful to have proper error messages for configuration issues.
How about http://php.net/manual/en/function.assert.php which are zero cost in php7
@codeliner your thoughts on this?
good for beginners/adoption
Needs a new minor tag though (7.1), change would be a BC break. But we are not bound to have the same minor version as event-store i hope / guess
In the past those strategies tried some more options like handle<shortCommandName>() so type hinting via interfaces was not possible. But now interfaces are possible and I agree that it could be useful for beginners.
It is comparable with the "Command was not handled" check, that we've added in v6 or so. This check is really useful so I think the interfaces + checks in the strategies will be useful, too.
Found one more need for interfaces
Normally one would use the Servicelocator plugin.
So, if your CommandHandler is a __invoke-able class, it will be called by the command class since it detects it as callable (https://github.com/prooph/service-bus/blob/master/src/CommandBus.php#L45)
If you have any InvokeStrategy enabled, e.g. the HandleCommandStrategy will be called (https://github.com/prooph/service-bus/blob/master/src/Plugin/InvokeStrategy/HandleCommandStrategy.php#L29). Handle method of course does not exist -> boom.
So that brings also one question up. for commands / query, as soon as a handler "handled" it, it sets handled to true. Should an Invoker check for that flag at all?
If I want to use e.g. the HandleCommandStrategy and a UseHandleCommand interface must be implemented therfor, I won't be able to type hint against my command object.
interface UseHandleCommand
{
public function handle($message): void;
}
class RegisterUserHandler implements UseHandleCommand
{
public function handle(RegisterUserCommand $message): void
{
}
}
// PHP Fatal error: Declaration of RegisterUserHandler::handle(RegisterUserCommand $message): void must be compatible with UseHandleCommand::handle($message): void So if I want to type hint I would be forced to use the callables.
@lunetics Afaik a BC break requires a new major version.
You are missing the void return type
Updated :-)
@UFOMelkor
does your RegisterUserCommand implement extend Prooph\Common\Messaging\Command ? The interface typehint is set to Command, so that should be no problem (see https://github.com/prooph/service-bus/pull/158/files#diff-f13902d8210c753033a504442038c2e9R11)
You are totally right, we need to typehint Message (as shown in first post)
If I want to use e.g. the HandleCommandStrategy and a UseHandleCommand interface must be implemented therfor, I won't be able to type hint against my command object.
That's true, we should omit the method signature.
We could do a check via reflectionclass in the handler to check if the method typehint implements "Message", e.g. But i think that is maybe overkill?
$this->listenerHandlers[] = $messageBus->attach(
MessageBus::EVENT_DISPATCH,
function (ActionEvent $actionEvent): void {
$message = $actionEvent->getParam(MessageBus::EVENT_PARAM_MESSAGE);
$handler = $actionEvent->getParam(MessageBus::EVENT_PARAM_MESSAGE_HANDLER);
$implementsMessaging = (new \ReflectionMethod($handler, 'handle'))->getParameters()[0]->getClass();
$implementsMessaging->implementsInterface(Message::class);
if ($handler instanceof UseHandleCommandStrategy && $implementsMessaging) {
$handler->handle($message);
$actionEvent->setParam(MessageBus::EVENT_PARAM_MESSAGE_HANDLED, true);
}
},
MessageBus::PRIORITY_INVOKE_HANDLER
);Service-Bus v6.0 was released Feb 16th. It's only 2 months old. I am against releasing a new major release at this time. This change is currently a BC, that's why it's not good to implement it as described here (and as implemented in #158).
If we want this change to happen, we need to find a way to implement this without having a BC, or we add new classes instead of changing the existing ones, because we can still add features, but not break BC.
@prolic Depends if the current behaviour is seen as Bug or configuration error:
When we use the ServiceLocator plugin AND the class XY is callable (__invoke), we can't use any Strategy, since the class XY is called via Bus e.g. (https://github.com/prooph/service-bus/blob/master/src/CommandBus.php#L45). After that, the InvokeStrategy will be called and can't find the "handle" method. We could implement an empty handle method, but that feels "foobar".
About callables: This can be fixed easily:
if (is_callable($handler )) {
$handler($event);
return;
}
if (is_object($handler)) {
// use invoke strategy
return;
}
But adding instanceof checks for interfaces that are new to the invoke strategy is clearly a BC break.
But doesn't that again break BC if we now ignore the __invoke strategy?
...But using the Servicelocator plugin makes all _invokable classes callable...
Nope, actually that is the behaviour of the provided bus implementations. For example command bus: https://github.com/prooph/service-bus/blob/master/src/CommandBus.php#L45-L48.
So if a handler is a callable, it will get called immediately. The OnEventStrategy is attached with same priority (but later), so it will get ignored. I cannot even see the described bug. Can you provide a unit tests that currently fails (but shouldn't)?
Btw: If you want to have a handler, that is an object and that implements the __invoke() method, BUT is not supposed to handle the message with that method, but via onEvent() method (which makes me think first: why the hell you wanna do that???, but whatever...), you can attach a custom plugin with higher priority that will do this for you.
@prolic so desired behaviour is:
If EventBus finds a callable, it is executed. No more InvokeStrategies should be called?
For now I see nothing stop calling the invoke strategy.
to the second: That's true and doesn't make sense. The current problem is IF you have a class with __invoke(Message $message) method, the later invoke strategy still wants to call the (non-existing) onEvent() method
I see, that's a bug in the OnEventStrategy:
We need to add this:
if ($actionEvent->getParam(MessageBus::EVENT_PARAM_MESSAGE_HANDLED, false)) {
return;
}
yes
We can reconsider this when we start planning on v7. I leave the ticket open, so we will not forget.