feat: add Messenger class for multiple adapter failover support#128
feat: add Messenger class for multiple adapter failover support#128deepshekhardas wants to merge 1 commit into
Conversation
Based on PR utopia-php#115 by TorstenDittmann. Adds Messenger class that orchestrates multiple adapters with automatic failover.
Greptile SummaryThis PR introduces a
Confidence Score: 4/5Safe to merge; the failover logic is correct for the common exception path, with one minor gap worth addressing before the next iteration. The send() catch block uses \Exception rather than \Throwable, so any \Error or \TypeError thrown inside an adapter escapes the failover loop entirely. In practice adapters are unlikely to throw \Error, but the gap exists and could surprise users of third-party adapters. Everything else — validation, ordering, minimum-maxMessages aggregation, and test coverage — looks correct. src/Utopia/Messaging/Messenger.php — specifically the catch clause in send() and the single-adapter error message wording Important Files Changed
Reviews (1): Last reviewed commit: "feat: add Messenger class for multiple a..." | Re-trigger Greptile |
| if (! \is_a($message, $messageType)) { | ||
| throw new \Exception( | ||
| 'Invalid message type. Expected "' | ||
| .$messageType | ||
| .'", got "' | ||
| .\get_class($message) |
There was a problem hiding this comment.
Failover skipped on
\Error subclasses
The catch block only catches \Exception, so any \TypeError or other \Error thrown by an adapter will propagate immediately without trying the next adapter, defeating the purpose of failover. Changing to \Throwable would cover both \Exception and \Error hierarchies.
| $adapterCount = \count($this->adapters); | ||
| $adapterLabel = $adapterCount === 1 ? 'adapter' : 'adapters'; | ||
|
|
||
| throw new \Exception( | ||
| 'All ' | ||
| .$adapterCount | ||
| .' ' | ||
| .$adapterLabel | ||
| ." failed:\n" | ||
| .\implode("\n", $errors) | ||
| ); |
There was a problem hiding this comment.
"All 1 adapter failed" reads awkwardly; use a distinct message for the single-adapter case to make logs clearer.
| $adapterCount = \count($this->adapters); | |
| $adapterLabel = $adapterCount === 1 ? 'adapter' : 'adapters'; | |
| throw new \Exception( | |
| 'All ' | |
| .$adapterCount | |
| .' ' | |
| .$adapterLabel | |
| ." failed:\n" | |
| .\implode("\n", $errors) | |
| ); | |
| $adapterCount = \count($this->adapters); | |
| if ($adapterCount === 1) { | |
| throw new \Exception( | |
| 'The adapter failed: ' | |
| .\implode("\n", $errors) | |
| ); | |
| } | |
| throw new \Exception( | |
| 'All ' | |
| .$adapterCount | |
| ." adapters failed:\n" | |
| .\implode("\n", $errors) | |
| ); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Port of PR #115 by TorstenDittmann.
Adds Messenger class that orchestrates multiple messaging adapters with automatic failover. If one adapter fails, it falls back to the next in the list.
Changes: