fix: Phase::Handle event hooks cause side-effects to original Event::Handle return value

[tsterker]

This PR is a proposal to resolve #213.

Symptom:
Listeners for the Phase::Handle cause an event's handle return value to be altered.

Root cause:
The Dispatcher::handle methods returns all return values by registered lifecycle hooks. This includes the original event's handle return value as well as the return values of any listeners on Phase::Handle.

Proposed solution:
Ensure that Dispatcher::handle will only consider the original event's return value.

[tsterker]

ℹ️ Update

It looks like the root cause is an interplay between Broker::commit setting the "last results" (plural) from the return value of Dispatcher::handle, which in turn considers any return value of the listener methods to be a valid "result".

[tsterker]

ℹ️ Extented existing OutsideListenersTest tests to cover replays

I wasn't sure if this addition is in line with your test conventions, but it seems like a sensible scenario that should be covered somewhere.

[inxilpro]

I don't see this as a bug… calling Verbs::commit(...) will return one result if there is only one result (the 90% case) and all results if there are more than one. I get that it makes it a little harder to code around if you don't know for sure which it will be, and maybe we could make that configurable somewhere. But Verbs doesn't treat the handle method on the event as any more special than other handle listeners…

[tsterker]

I don't see this as a bug… calling Verbs::commit(...) will return one result if there is only one result (the 90% case) and all results if there are more than one. I get that it makes it a little harder to code around if you don't know for sure which it will be, and maybe we could make that configurable somewhere. But Verbs doesn't treat the handle method on the event as any more special than other handle listeners…

Thanks for the reply! And indeed, "bug" might be the wrong classification.

But with the premise of the event handle return not being "special", the commit return value would effectively be a wildcard that you cannot "trust" unless you have e.g. test coverage for your use case of the return value. So, while not bug, I'd claim that if there is no guarantee/defined behavior, the utility of this return behavior would be affected. Because there no longer would be a contract that you can rely on.

Or to turn it around: what would be the benefit of offering the (current) flexibility of being able to hook into the return value and alter what is being returned? Is there a use case that I'm missing where the ability to affect the data and shape of the return value would bring value?

For me, the wonderful self-contained-ness of the verbs event classes suffers a bit of a hit if one loses control of what the event "intends" to return by the mere existence of a potentially rogue event listener. It feels a bit like mutating an assumed-to-be private property.

TL;DR: Is there a benefit of providing the flexibility for event listeners to add arbitrary return values vs. the guarantee that you can safely use the return value of your handle method?

Looking at the code, I understand the idea of not treating the "original" handle method any different - it's all just registered hooks. But factually, a differentiation seems like a benefit to me that provides extra guarantees when working with my events.

I guess I could simply always only consider the first return value, but I would at least need to either take the singular value or the first element of a collection, in case some listener got it's hand on my event.

PS: Please don't see this "rant" as a critique or complaining :)
I'm currently just contemplating how our assumptions we currently make about how we can leverage handle returns might need to be updated.

[tsterker]

Quick update for anyone facing this or interested in a safe(er) way to tackle this.

In general, I would suggest not relying on the return value of your commit calls.
With the current behavior, anybody (including 3rd party packages) could alter the shape of your commit return value.

If you still want to use the feature, you should take control over the return value as much as possible by handling the possibility that a collection will be returned. Below are two example approaches, with some limitations.

⚠️ Caution:

• This workaround relies on the current implementation detail that the handle return would always be the first element._
• If your handle method would already return a Collection, the above workaround would break._

If anybody has ideas for a more robust solution, I'm all ears :)

A) Per-case basis

When you want to obtain the commit return value, simply ensure to always grab the first element in case of a returned collection.

$value = Collection::wrap(MyEvent::commit())->first();

B) Event Base Class

For a more generalized solution, one could introduce a base class for events.

Note that I would probably not override the base commit method to avoid confusion as to what is verbs and custom behavior.

<?php

namespace App\Events;

use Illuminate\Support\Collection;
use Thunk\Verbs\Event;

class VerbsBaseEvent extends Event
{
    /** @see https://github.com/hirethunk/verbs/pull/218 */
    public static function commitAndExclusivelyReturnOriginalHandleReturnValue(...$args): mixed
    {
        return Collection::wrap(static::commit(...$args))->first();
    }
}

[tsterker]

ℹ️ Follow-up: Verbs Docs

Apologies for being pedantic, but it would probably be worth extending the Verbs docs with some clarifications.

Here a screenshot from the current docs, with some snarky comments 😅