HandlesEvents design considerations

[brendt]

With the the current implementation of v5, there are as much as four ways to configure projector and reactor handlers:

• With a magic mapping based on the argument's type hint:

class CartProjector extends Projector
{
    public function onCartInitialized(CartInitialized $event): void { /* … */ }
}

• With and explicit mapping in $handlesEvents:

class CartProjector extends Projector
{
    protected array $handlesEvents = [
        CartInitialized::class => 'onCartInitialized',
    ];

    public function onCartInitialized(CartInitialized $event): void { /* … */ }
}

• With an explicit mapping for exactly one event, making this projector only handle that one event:

class CartInitializedProjector extends Projector
{
    // Explicit mapping for one specific event
    protected string $handlesEvent = CartInitialized::class

    public function __invoke(CartInitialized $event): void { /* … */ }
}

• Using the ListensTo attribute:

class CartInitializedProjector extends Projector
{
    // Explicit mapping using attributes
    #[ListensTo(CartInitialized::class)]
    public function onCartInitialized(CartInitialized $event): void { /* … */ }
}

It's my opinion that this amount of flexibility doesn't serve better DX. It's often times the opposite:

• I often doubt what the correct spelling of handlesEvents is, which requires me to look it up in other projectors.
• The convention in aggregate roots is that applied events are mapped based on the method name, instead of the argument type, which is inconsistent and causes confusion
• While attributes offer more flexibility (map several events to the same handler), they often introduce overhead. Here's an example of when the attribute notation is useful:

class CartInitializedProjector extends Projector
{
    #[ListensTo(CartInitialized::class)]
    #[ListensTo(CartItemAdded::class)]
    #[ListensTo(CartItemRemoved::class)]
    public function __invoke(CartEvent $event): void { /* … */ }
}

This doesn't happen very often though.

I propose we pick one approach, and use it for reactors, projectors and aggregate root apply methods.

[brendt]

My personal opinion is that we go with attributes, but with a few more tweaks:

• Call them Handles instead of ListensTo
• In case of projectors and reactors, allow them both on methods and classes, if added on a class, than we'll use the __invoke method of that class as the handler.

Here are a few examples:

#[Handles(CartInitialized::class)]
#[Handles(CartItemAdded::class)]
#[Handles(CartItemRemoved::class)]
class CartInitializedProjector extends Projector
{
    public function __invoke(CartEvent $event): void { /* … */ }
}

class CartInitializedReactor extends Reactor
{
    #[Handles(CartInitialized::class)]
    public function onWhateverYouWantToCallThis(CartInitialized $event): void { /* … */ }
}

class CartAggregateRoot extends AggregateRoot
{
    #[Handles(CartInitialized::class)]
    protected function applyCartInitialized(CartInitialized event): void { /* … */ }
}

I'd suggest to drop support for everything else.

[freekmurze]

I'm ok with renaming ListensTo to Handles. and the tweaks on attributes.

I do like the several methods to configure projectors and reactors now. For our projects we'll likely only use attributes. I can image that newcomers to the package might prefers one of the other options which reflects Laravel's way of configuring event listeners.

[brendt]

I'm already going to add Handles in all places, we can decide on the alternatives later.

[aidan-casey]

I do not think you all should worry about newcomers and what they prefer. If it were me, the questions I would be asking are:

1. How does the approach we take effect performance?

2. Does the approach we take produce clean code?

I don’t think the reflection way is worth having around. It’s certainly a nice little feature, but it probably does have a (very minimal) performance hit and there’s enough of those hidden magical things in Laravel already.

I like both attributes and the array. That said, the attributes do not look as clean to me. I think that might be because of how repetitive they are with the class name Handles.

It sounds like you all are leaning in the direction of attributes, so I’d say roll with it and any users of this package will adapt if necessary.

I also like the renaming of ListensTo to Handles. 👍🏼

[brendt]

I think that might be because of how repetitive they are with the class name Handles

We could also do something like

#[Handles(EventA::class, EventB::class, …)
public function handleEvents(…)

I think that's probably a cleaner solution.

[freekmurze]

#[Handles(EventA::class, EventB::class, …)
public function handleEvents(…)

Seems like we definitely want to support that 👍

[aidan-casey]

Yeah that looks more clean. 👍🏼

[sebastiandedeyne]

I'm actually in favor of "With a magic mapping based on the argument's type hint". Specifically because we're already auto-discovering projectors and reactors.

Otherwise, setting up a projector means:

1. Create a projector class
2. Projector class is automatically discovered
3. Create an event handler method
4. Manually map the method

Why is step 2 automatic, but step 4 deemed too magical? And it's not because PHP 8 attributes are a language construct that using them to register events is less magical. I don't think the difference between argument reflection and attributes is about the magic, it's implicit vs. explicit.

I think we need to check these two boxes for a good implementation (feel free to add more):

1. It should be clear which method handles which event
2. The event payload should be one click away

public function onCartInitialized(CartInitialized $event): void { /* … */ }

This snippet checks them both.

It also reduces the chance of a certain kind of bugs. Have fun debugging this one:

class CartInitializedProjector extends Projector
{
    // Explicit mapping using attributes
    #[ListensTo(CartUninitialized::class)]
    public function onCartInitialized(CartInitialized $event): void { /* … */ }
}

Mapping an event to a listener in this context is plumbing. The less plumbing I need to do, the more energy I have left to work on things I care about. The less plumbing in my code, the less lines I need to write, read, or change.

[aidan-casey]

Yeah, you are right, it is more of an implicit vs. explicit issue. The primary issue I would have with magic mapping is that as a new developer comes into the project and digs into the code, they would not immediately see that the method is both registering the handler AND handling unless they were already familiar with the package.

That does not really matter if both are supported though, it leaves it up to the team using the package.

[freekmurze]

@brendt Let's just keep supporting the current behaviour