Uh oh!
There was an error while loading. Please reload this page.
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/5
Important Files Changed
Prompt To Fix All With AI### Issue 1
tests/Messaging/MessengerTest.php:195-205
**Multiple `expectExceptionMessage` calls silently overwrite each other**
PHPUnit stores a single expected message string; each call to `expectExceptionMessage()` replaces the previous one. In `test_throws_when_all_adapters_fail` (and similarly in `test_throws_when_single_adapter_fails`), only the last call (`'SecondAdapter (adapter 2): API error'`) is actually asserted. The earlier checks for `'All 2 adapters failed'` and `'FirstAdapter (adapter 1): Connection timeout'` are silently discarded, leaving the error header and first-adapter detail completely untested. Use a manual `try/catch` with `assertStringContainsString()` calls to assert all three sub-strings.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (7): 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.
Uh oh!
There was an error while loading. Please reload this page.
| * successful adapter. | ||
| * | ||
| * @param Message $message The message to send. | ||
| * @return array{ |
There was a problem hiding this comment.
Soft-failure responses bypass failover silently
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.
Uh oh!
There was an error while loading. Please reload this page.
deepshekhardas
commented
Jul 11, 2026
Following up - this PR adds Messenger class for multiple adapter failover. Ready for review. |
Based on PR utopia-php#115 by TorstenDittmann. Adds Messenger class that orchestrates multiple adapters with automatic failover.
| } | ||
| public function test_rejects_empty_adapter_list(): void | ||
| { | ||
| $this->expectException(\InvalidArgumentException::class); | ||
| $this->expectExceptionMessage('At least one adapter must be provided'); | ||
| new Messenger([]); | ||
| } | ||
| public function test_rejects_non_adapter_array_element(): void |
There was a problem hiding this comment.
Multiple
expectExceptionMessage calls silently overwrite each other
PHPUnit stores a single expected message string; each call to expectExceptionMessage() replaces the previous one. In test_throws_when_all_adapters_fail (and similarly in test_throws_when_single_adapter_fails), only the last call ('SecondAdapter (adapter 2): API error') is actually asserted. The earlier checks for 'All 2 adapters failed' and 'FirstAdapter (adapter 1): Connection timeout' are silently discarded, leaving the error header and first-adapter detail completely untested. Use a manual try/catch with assertStringContainsString() calls to assert all three sub-strings.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/Messaging/MessengerTest.php
Line: 195-205
Comment:
**Multiple `expectExceptionMessage` calls silently overwrite each other**
PHPUnit stores a single expected message string; each call to `expectExceptionMessage()` replaces the previous one. In `test_throws_when_all_adapters_fail` (and similarly in `test_throws_when_single_adapter_fails`), only the last call (`'SecondAdapter (adapter 2): API error'`) is actually asserted. The earlier checks for `'All 2 adapters failed'` and `'FirstAdapter (adapter 1): Connection timeout'` are silently discarded, leaving the error header and first-adapter detail completely untested. Use a manual `try/catch` with `assertStringContainsString()` calls to assert all three sub-strings.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
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: