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
Greptile SummaryThis PR introduces a
Confidence Score: 4/5Safe to merge after fixing the array key normalisation issue in the constructor. The constructor stores the raw input array and then accesses $adapters[0] directly in validateAdapters, send, getType, and getMessageType. An associative or non-zero-based array produces a TypeError from null->getType() instead of the clean InvalidArgumentException the class promises. Adding a single array_values() call resolves it. src/Utopia/Messaging/Messenger.php — the array key normalisation fix is needed before the class handles all valid array inputs correctly. Important Files Changed
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/Utopia/Messaging/Messenger.php:52-66
**Non-zero-indexed arrays silently break construction**
The constructor stores the raw input array without normalising its keys, yet both `validateAdapters` and multiple public methods access `$adapters[0]` / `$this->adapters[0]` directly. If a caller passes an associative or non-zero-based array (e.g. `new Messenger(['primary' => $twilio, 'backup' => $vonage])`), `$adapters[0]` evaluates to `null`, and `null->getType()` inside `validateAdapters` throws an unhandled `\TypeError` instead of the clean `\InvalidArgumentException` the class documents. Adding `$adapters = array_values($adapters);` after the element-type loop fixes all affected sites (`validateAdapters`, `send`, `getType`, `getMessageType`) in one place.
Reviews (3): Last reviewed commit: "feat: add Messenger class for multiple a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (! \is_a($message, $messageType)) { | ||
| throw new \Exception( | ||
| 'Invalid message type. Expected "' | ||
| .$messageType | ||
| .'", got "' | ||
| .\get_class($message) |
There was a problem hiding this comment.
\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.
deepshekhardas
force-pushed
the
fix/115-messenger-failover
branch
from
d4b60e4 to
834d575
Compare
| * successful adapter. | ||
| * | ||
| * @param Message $message The message to send. | ||
| * @return array{ |
There was a problem hiding this comment.
Messenger::send() only falls back on thrown exceptions, so an adapter that returns a response with deliveredTo: 0 and all-failure results is treated as "success" — the second adapter is never tried. This is documented by the test, but the behaviour may surprise callers who expect the Messenger to be smarter about delivery failures. At minimum, the PHPDoc and README should warn that "failure" payloads without an exception are returned as-is without attempting the next adapter, so callers must inspect deliveredTo themselves.
Based on PR utopia-php#115 by TorstenDittmann. Adds Messenger class that orchestrates multiple adapters with automatic failover.
deepshekhardas
force-pushed
the
fix/115-messenger-failover
branch
from
834d575 to
dc20a1c
Compare
| foreach ($adapters as $index => $adapter) { | ||
| if (! $adapter instanceof Adapter) { | ||
| throw new \InvalidArgumentException( | ||
| 'All elements must be instances of Adapter, but element ' | ||
| .$index | ||
| .' is ' | ||
| .\get_debug_type($adapter) | ||
| .'.' | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| $this->validateAdapters($adapters); | ||
|
|
||
| $this->adapters = $adapters; |
There was a problem hiding this comment.
The constructor stores the raw input array without normalising its keys, yet both validateAdapters and multiple public methods access $adapters[0] / $this->adapters[0] directly. If a caller passes an associative or non-zero-based array (e.g. new Messenger(['primary' => $twilio, 'backup' => $vonage])), $adapters[0] evaluates to null, and null->getType() inside validateAdapters throws an unhandled \TypeError instead of the clean \InvalidArgumentException the class documents. Adding $adapters = array_values($adapters); after the element-type loop fixes all affected sites (validateAdapters, send, getType, getMessageType) in one place.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Utopia/Messaging/Messenger.php
Line: 52-66
Comment:
**Non-zero-indexed arrays silently break construction**
The constructor stores the raw input array without normalising its keys, yet both `validateAdapters` and multiple public methods access `$adapters[0]` / `$this->adapters[0]` directly. If a caller passes an associative or non-zero-based array (e.g. `new Messenger(['primary' => $twilio, 'backup' => $vonage])`), `$adapters[0]` evaluates to `null`, and `null->getType()` inside `validateAdapters` throws an unhandled `\TypeError` instead of the clean `\InvalidArgumentException` the class documents. Adding `$adapters = array_values($adapters);` after the element-type loop fixes all affected sites (`validateAdapters`, `send`, `getType`, `getMessageType`) in one place.
How can I resolve this? If you propose a fix, please make it concise.
|
Following up - this PR adds Messenger class for multiple adapter failover. Ready for review. |
1 participant
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: