fix: adapt event listeners and dispatch to framework#1541 event API - #150
Merged
Merged
Conversation
… (framework#1541)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SendShipmentNotificationand the feature suite'sintegrationListenersatisfy the newevent.Listenercontract (Handle(eventName string, args ...any) error), so the bundledOrderShipped/OrderCanceleddemo events dispatch again.facades.Event().Dispatch(...)/Listen(...)and the returnedResultinstead of the deprecatedJob(...).Dispatch()/Register(...)flow; the dispatch tests assert the new semantics — an unlistened event is a silent success, and every registered listener runs with its failures aggregated into theResultrather than aborting the dispatch.Job/Register/GetEventspath while it remains available, locking in its legacy semantics: an unbound job fails withEventListenerNotBind, a job is fail-fast and deliversHandle-transformed payloads to sync or queued listeners, andGetEventsreturns a defensive copy.Closes https://github.com/goravel/goravel/issues/1541
Why
framework#1541 reworked the event module:
event.Listener.Handlenow receives the canonical event name first (Handle(eventName string, args ...any) error), andListen/Dispatchreplaced the now-deprecatedRegister/Jobflow with aResultthat joins every listener error. Once the example pinnedgoravel/frameworkpast that merge, the demo listener and the suite'sintegrationListenerno longer satisfiedevent.Listener, so the example would not build. This PR adapts both listeners to the new contract and migrates the example service and the event feature suite to the currentDispatch/ListenAPI.Dispatch semantics changed with it: the framework now runs every matching listener and collects the failures, and an event with no listeners is a no-op rather than an error. The migrated dispatch tests assert those semantics — each registers under its own event type because
Listenappends listeners by the event's type name rather than overwriting them, and the payload argument positions passed toHandleare unchanged.The deprecated
Register/Job/GetEventspath is still shipped and will only be removed in a future version, so the feature suite keeps it under test while it exists. The newJob-based tests pin down its legacy semantics — an unbound job fails withEventListenerNotBind, the event'sHandleerror short-circuits the job, a job stops at the first listener error, and it still deliversHandle-transformed arguments to both sync and queued listeners — whileGetEventsis verified to hand back a defensive copy so callers cannot mutate the live registry. Keeping these tests guards the deprecated path from regressions and gives the eventual removal a defined, tested contract to migrate against.