feat(breaking): add Vonage Messages API adapter - #143
Conversation
|
Thanks for contributing! This repository is a read-only mirror; development for this library happens in |
Greptile SummaryAdds an SMS adapter for Vonage's Messages API.
Confidence Score: 3/5The empty-recipient crash should be fixed before this adapter is merged, and deterministic coverage should be added for its request and error paths. The shared send validation permits an empty recipient array, while the new adapter unconditionally reads its first element and passes null to a string-only function; the credential-gated tests do not exercise this or the adapter's failure handling in normal CI. Files Needing Attention: src/Utopia/Messaging/Adapter/SMS/VonageMessages.php, tests/Messaging/Adapter/SMS/VonageMessagesTest.php Important Files Changed
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/Utopia/Messaging/Adapter/SMS/VonageMessages.php:61
**Empty recipients crash processing**
When an SMS has an empty `to` array, the inherited validation permits it, but this line accesses index 0 and passes null to `ltrim()`, causing the send operation to fail with a `TypeError` instead of returning a messaging response.
### Issue 2 of 2
tests/Messaging/Adapter/SMS/VonageMessagesTest.php:18-20
**Adapter behavior lacks deterministic coverage**
These tests skip without live credentials and exercise only successful deliveries, leaving the new request construction and HTTP, transport, and malformed-response handling untested in normal CI. Add deterministic client or local-server tests that assert the outgoing request and representative error responses.
Reviews (1): Last reviewed commit: "fix: address review feedback and harden ..." | Re-trigger Greptile |
|
|
||
| protected function process(SMSMessage $message): array | ||
| { | ||
| $to = \ltrim($message->getTo()[0], '+'); |
There was a problem hiding this comment.
When an SMS has an empty to array, the inherited validation permits it, but this line accesses index 0 and passes null to ltrim(), causing the send operation to fail with a TypeError instead of returning a messaging response.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Utopia/Messaging/Adapter/SMS/VonageMessages.php
Line: 61
Comment:
**Empty recipients crash processing**
When an SMS has an empty `to` array, the inherited validation permits it, but this line accesses index 0 and passes null to `ltrim()`, causing the send operation to fail with a `TypeError` instead of returning a messaging response.
How can I resolve this? If you propose a fix, please make it concise.
| if (!$apiKey || !$apiSecret || !$to) { | ||
| $this->markTestSkipped('Vonage Messages credentials or recipient are not available.'); | ||
| } |
There was a problem hiding this comment.
These tests skip without live credentials and exercise only successful deliveries, leaving the new request construction and HTTP, transport, and malformed-response handling untested in normal CI. Add deterministic client or local-server tests that assert the outgoing request and representative error responses.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/Messaging/Adapter/SMS/VonageMessagesTest.php
Line: 18-20
Comment:
**Adapter behavior lacks deterministic coverage**
These tests skip without live credentials and exercise only successful deliveries, leaving the new request construction and HTTP, transport, and malformed-response handling untested in normal CI. Add deterministic client or local-server tests that assert the outgoing request and representative error responses.
How can I resolve this? If you propose a fix, please make it concise.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!
Rebased on latest main with conflict resolution.
Conflicts resolved: