Skip to content

fix: rebase PR #99 - SMSGateApp SMS adapter - #140

Closed
deepshekhardas wants to merge 1 commit into
utopia-php:mainfrom
deepshekhardas:fix/pr-99-rebase
Closed

fix: rebase PR #99 - SMSGateApp SMS adapter#140
deepshekhardas wants to merge 1 commit into
utopia-php:mainfrom
deepshekhardas:fix/pr-99-rebase

Conversation

@deepshekhardas

Copy link
Copy Markdown

Replacement for #99. Original by @capcom6.

Rebased on latest main. Fixed:

  • Added parent::__construct() call
  • Made properties private readonly
  • Moved apiEndpoint default to constructor parameter default
  • Aligned with v2.0.0 conventions

Original by @capcom6.

Fixed:
- Added parent::__construct() call
- Made properties private readonly
- Moved apiEndpoint default to constructor parameter default
- Aligned with v2.0.0 conventions
@github-actions

Copy link
Copy Markdown

Thanks for contributing! This repository is a read-only mirror; development for this library happens in packages/messaging in the utopia-php monorepo. Please open this pull request there instead.

@github-actions github-actions Bot closed this Jul 23, 2026
@greptile-apps

greptile-apps Bot commented Jul 23, 2026

Copy link
Copy Markdown

Greptile Summary

Adds an SMSGateApp SMS adapter and its credential-gated integration test.

  • Sends JSON requests using HTTP Basic authentication to the configurable SMSGateApp endpoint.
  • Maps accepted API responses into per-recipient delivery results.
  • Limits each request to ten recipients.

Confidence Score: 4/5

The endpoint handling in the integration test needs to be fixed before merging because the normal default-endpoint configuration throws a TypeError.

When credentials are configured without an endpoint override, the test converts the missing value to null and explicitly supplies it to a constructor parameter that only accepts strings.

tests/Messaging/Adapter/SMS/SMSGateAppTest.php

Important Files Changed

Filename Overview
src/Utopia/Messaging/Adapter/SMS/SMSGateApp.php Adds the SMSGateApp adapter with authentication, request construction, recipient-result mapping, and a ten-recipient limit.
tests/Messaging/Adapter/SMS/SMSGateAppTest.php Adds an integration test, but its absent-endpoint path passes null to a non-nullable constructor parameter and fails before sending.

Fix All in Claude Code Fix All in Codex

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
tests/Messaging/Adapter/SMS/SMSGateAppTest.php:21-23
**Null endpoint bypasses default**

When the credentials and recipient are configured but `SMSGATEAPP_ENDPOINT` is unset, the test explicitly passes `null` to the non-nullable `string` constructor parameter, causing a `TypeError` instead of using `DEFAULT_API_ENDPOINT` and sending the SMS.

Reviews (1): Last reviewed commit: "fix: rebase PR #99 - SMSGateApp SMS adap..." | Re-trigger Greptile

Comment on lines +21 to +23
$endpoint = \getenv('SMSGATEAPP_ENDPOINT') ?: null;

$sender = new SMSGateApp($username, $password, $endpoint);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Null endpoint bypasses default

When the credentials and recipient are configured but SMSGATEAPP_ENDPOINT is unset, the test explicitly passes null to the non-nullable string constructor parameter, causing a TypeError instead of using DEFAULT_API_ENDPOINT and sending the SMS.

Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/Messaging/Adapter/SMS/SMSGateAppTest.php
Line: 21-23

Comment:
**Null endpoint bypasses default**

When the credentials and recipient are configured but `SMSGATEAPP_ENDPOINT` is unset, the test explicitly passes `null` to the non-nullable `string` constructor parameter, causing a `TypeError` instead of using `DEFAULT_API_ENDPOINT` and sending the SMS.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code Fix in Codex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant