AutoRegister all public methods

[ruudk]

When a handler or subscriber is tagged and has an __invoke method it will be automatically registered. This is awesome.

But what if you want to have 1 subscriber that handles 2 or more events?

Then you need to do manual work. Not nice!

This PR changes the AutoRegister compiler pass to search for all public methods and just register them as handlers/subscribers.

Tests are failing because of SimpleBus/message-bus#76 (comment)

[ruudk]

I see there was already some discussion about this in #43. It was mentioned that if you need to handle more events, you create an abstract subscriber and then use that. I don't think this bridge should enforce this. I know SRP, sometimes there are just 2 or more events that need the same handling :)

[cmodijk]

The reason why I primary don't like to add al public methods is that for command handlers i don't think you should ever add two commands to one handler. For events i think it should be viable to do so.

The other objection I have is that i have no controle over which methods gets registered in this case all public methods. But if i look at the Symfony event listener i can do two things (as far as i know).

1. Add a tag by hand for every method i want a listener on
2. add the EventSubscriberInterface interface and define which methods i want to register

The current AutoRegister creates a third option and implies the use of the __invoke method which makes sure you can only register one method.

So we currently support method 1. to manual add tags and the 3. option for the __invoke i think if we want to do something like this we could add option 2 and have a trait that just registers everything example below @ruudk what do you think?

interface MessageSubscriberInterface
{
    public function getSubscribedMessages();
}

class MyCommandHandler implements MessageSubscriberInterface
{
    public function getSubscribedMessages()
    {
        return [
            MyFirstCommand::class => 'handleThisOne',
            MySecondCommand::class => 'handleThisTo',
        ];
    }

    public function handleThisOne(MyFirstCommand $command)
    {
        // Do stuff
    }

    public function handleThisTo(MySecondCommand $command)
    {
        // Do stuff
    }
}

With trait

trait SubscribeAllPublicMethods
{
    public function getSubscribedMessages()
    {
        // Logic to extract all public methods
    }
}

class MyCommandHandler implements MessageSubscriberInterface
{
    use SubscribeAllPublicMethods;

    public function handleThisOne(MyFirstCommand $command)
    {
        // Do stuff
    }

    public function handleThisTo(MySecondCommand $command)
    {
        // Do stuff
    }
}

[ruudk]

I like your idea. But I still don't like the fact that I have to implement an interface or use a trait. Currently, Commands, Events, Handlers and Subscribers don't need to extend any SimpleBus interface. You just map everything together with DIC.

What if we go with the approach from this PR, but only if the services are tagged with a special parameter? Like auto_register_public_methods: true? It would be disabled by default and you can enable it per bundle or for the whole project.
Another option would be to add this to the bundle configuration so that you can either enable or disable it for the whole project.

[cmodijk]

@ruudk With the AutoWire feature in mind can we add different parameters from the services.yml to tag services differently on a namespace basis?

services:
    Infrastructure\Handler\AllPulic\:
        resource: '../../src/AppBundle/Controller'
        public: true
        tags: ['command_handler']
        ???: 
            auto_register_public_methods: true

    Infrastructure\Handler\OnlyInvoke\:
        resource: '../../src/AppBundle/Controller'
        public: true
        tags: ['command_handler']
        ???: 
            auto_register_public_methods: false

[cmodijk]

Does Adding Additional Attributes on Tags also work with auto wire?

[ruudk]

Yes, I will make a POC :)

[cmodijk]

I was thinking to allow 3 strategies

1. The current implementation with __invoke
2. Choose your method with tags: [{ name: 'command_handler', method: 'handle'}]
3. All public tags: [{ name: 'command_handler', allPublic: true}]

[ruudk]

@cmodijk I like allPublic: true. I will try to make this AutoRegister support scenario 1 + 3. Scenario 2 is already handled.

Can you please create v3.0.1 for MessageBus so that my tests will work? :)

[cmodijk]

A complete different note;

We know have two classes to wire commands (events) to handlers (subscribers) AutoRegister which reeds the tag command_handler tag and then based on the __invoke method it adds the command_bus tag with the handles argument.

But the RegisterHandlers know has the requirement to have the handles inside it to be able to handle a command. But why don't we try to merge these 2 compilers into 1 and based on the tag arguments decide what to do with it.

# Current methods of 'RegisterHandlers'
{ name: 'command_handler', handles: 'My\Command\Name' } # Uses __invoke method
{ name: 'command_handler', handles: 'My\Command\Name', method: 'handle' } # Uses handles method
 
# Current method of 'AutoRegister'
{ name: 'command_handler' } # Adds new tag { name: 'command_handler', handles: 'My\Command\Name' }

# New suggestion
{ name: 'command_handler', register_public_methods: true }

[ruudk]

@cmodijk Could you please tag a new MessageBus release? Tests are still failing.

I added support for tag attribute register_public_methods

See dd7b5ce

[cmodijk]

I see that my other comment was never posted. Do we create a v3.0.1 version or a v3.1.0 version? What do you think?

[ruudk]

I'm fine with 3.0.1 😊

[ruudk]

@cmodijk I went ahead and tagged it myself, see https://github.com/SimpleBus/MessageBus/releases/tag/v3.0.1

[cmodijk]

I See thats fine with me

[ruudk]

@cmodijk It's green! What do you think?

[garak]

Please merge this! I can't wait to remove all that ugly _invoke methods from my handlers

[cmodijk]

@ruudk Did you read my earlier comment about merging this class with the current register compiler class #53 (comment)? Should we improve this now or create a separate issue to implement this later down the line any thoughts about this?

[ruudk]

@cmodijk I think it's better to do that later. First ship and release this :)

[ruudk]

Ready to merge?

[cmodijk]

Merged! Thanks @ruudk I wil tag the latest version in a moment.

[cmodijk]

Released the latest version @ruudk thanks for the work!
https://github.com/SimpleBus/SymfonyBridge/releases/tag/v5.1.0

[garak]

Just tried to remove __invoke from my handler and got

Array([0] => Foo\BarHandler[1] => __invoke) could not be resolved to a valid callable

😞

Edit: after a cache clear, the error is

Could not find a callable for name "BarCommand"

Final edit: added register_public_methods option to my tag and now everything works like a charm! 👏

[ruudk]

🎉🎉🎉 I will update the docs soon ✌️

[ruudk]

PR for docs: SimpleBus/docs#46