feat: add Alibaba Cloud SMS adapter (#6307)#109
feat: add Alibaba Cloud SMS adapter (#6307)#109deepshekhardas wants to merge 1 commit intoutopia-php:mainfrom
Conversation
WalkthroughThis pull request introduces a new Alibaba Cloud SMS adapter. The implementation includes a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/Utopia/Messaging/Adapter/SMS/AlibabaCloud.php (3)
56-56: Consider making RegionId configurable.The
RegionIdis hardcoded to'cn-hangzhou'. Alibaba Cloud SMS is available in multiple regions. Consider accepting this as a constructor parameter with a default value for flexibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Utopia/Messaging/Adapter/SMS/AlibabaCloud.php` at line 56, The RegionId is hardcoded; update the AlibabaCloud class to accept a configurable region by adding a constructor parameter (e.g., $region = 'cn-hangzhou') that assigns to a private property (e.g., $this->region) and replace the hardcoded 'cn-hangzhou' in the request payload with that property; ensure any existing instantiations of AlibabaCloud continue to work by keeping the default value and update references to the class constructor if tests or other code create the object with a custom region.
62-62: Hardcoded template parameter key limits flexibility.The
TemplateParamis hardcoded to use a'code'key, which assumes all SMS templates use this exact parameter name. Alibaba Cloud SMS templates can have arbitrary parameter names defined by the user. This also creates a mismatch with the test, which passes JSON as content that gets wrapped again.Consider allowing the message content to be the full
TemplateParamJSON directly:♻️ Proposed fix
- 'TemplateParam' => \json_encode(['code' => $message->getContent()]), + 'TemplateParam' => $message->getContent(),This would require the caller to pass properly formatted JSON for
TemplateParam, giving flexibility for any template structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Utopia/Messaging/Adapter/SMS/AlibabaCloud.php` at line 62, The TemplateParam is currently hardcoded to ['code' => $message->getContent()] in AlibabaCloud.php which prevents arbitrary template keys and double-wraps JSON; change the call that sets 'TemplateParam' to accept the message content as the full TemplateParam JSON/object directly by using $message->getContent() (ensuring it is passed through as JSON if it's an array/object or left as-is if already a JSON string) so callers supply the exact template parameter structure expected by Alibaba Cloud; update any tests to pass a full JSON TemplateParam string or array to message->getContent() accordingly.
59-59: Useuniqidwith more entropy for better uniqueness.
uniqid()without themore_entropyparameter may generate duplicate nonces in high-concurrency scenarios, which could cause signature collisions and API request failures.♻️ Proposed fix
- 'SignatureNonce' => \uniqid(), + 'SignatureNonce' => \uniqid('', true),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Utopia/Messaging/Adapter/SMS/AlibabaCloud.php` at line 59, Replace the current use of \uniqid() for the 'SignatureNonce' value with a higher-entropy call to avoid collisions under concurrency; specifically, in AlibabaCloud.php where the request payload/headers are assembled (the entry that sets 'SignatureNonce' => \uniqid()), change it to use \uniqid('', true) so the nonce includes more entropy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Utopia/Messaging/Adapter/SMS/AlibabaCloud.php`:
- Around line 106-113: The percentEncode function uses preg_replace('/%7E/i',
'~', $res) which can return null on error but percentEncode is typed to return
string; update percentEncode to handle a null result from preg_replace (in
function percentEncode) by checking the return value and falling back to the
original $res or casting to string before returning so the method always returns
a string, ensuring any preg_replace failure is safely handled.
In `@tests/Messaging/Adapter/SMS/AlibabaCloudTest.php`:
- Around line 23-26: The test is passing a JSON string as SMS content which
causes double-encoding when the adapter wraps it again; update the test in
AlibabaCloudTest so SMS is constructed with content as an associative array
(e.g., ['code' => '123456']) or otherwise match the adapter’s expected
TemplateParam shape instead of passing json_encode(...), and adjust any
assertions that inspect the resulting TemplateParam payload to expect the
single-layer JSON (or raw array) produced by the adapter; update references to
the SMS constructor call (symbol: SMS) in the test to pass the direct array
content.
---
Nitpick comments:
In `@src/Utopia/Messaging/Adapter/SMS/AlibabaCloud.php`:
- Line 56: The RegionId is hardcoded; update the AlibabaCloud class to accept a
configurable region by adding a constructor parameter (e.g., $region =
'cn-hangzhou') that assigns to a private property (e.g., $this->region) and
replace the hardcoded 'cn-hangzhou' in the request payload with that property;
ensure any existing instantiations of AlibabaCloud continue to work by keeping
the default value and update references to the class constructor if tests or
other code create the object with a custom region.
- Line 62: The TemplateParam is currently hardcoded to ['code' =>
$message->getContent()] in AlibabaCloud.php which prevents arbitrary template
keys and double-wraps JSON; change the call that sets 'TemplateParam' to accept
the message content as the full TemplateParam JSON/object directly by using
$message->getContent() (ensuring it is passed through as JSON if it's an
array/object or left as-is if already a JSON string) so callers supply the exact
template parameter structure expected by Alibaba Cloud; update any tests to pass
a full JSON TemplateParam string or array to message->getContent() accordingly.
- Line 59: Replace the current use of \uniqid() for the 'SignatureNonce' value
with a higher-entropy call to avoid collisions under concurrency; specifically,
in AlibabaCloud.php where the request payload/headers are assembled (the entry
that sets 'SignatureNonce' => \uniqid()), change it to use \uniqid('', true) so
the nonce includes more entropy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1202d194-634e-41a4-b399-8d5ff5644271
📒 Files selected for processing (2)
src/Utopia/Messaging/Adapter/SMS/AlibabaCloud.phptests/Messaging/Adapter/SMS/AlibabaCloudTest.php
| private function percentEncode(string $str): string | ||
| { | ||
| $res = \urlencode($str); | ||
| $res = \str_replace(['+', '*'], ['%20', '%2A'], $res); | ||
| $res = \preg_replace('/%7E/i', '~', $res); | ||
|
|
||
| return $res; | ||
| } |
There was a problem hiding this comment.
preg_replace can return null on error.
If preg_replace encounters an error, it returns null, but the function is typed to return string. This could cause a type error.
🛡️ Proposed fix
private function percentEncode(string $str): string
{
$res = \urlencode($str);
$res = \str_replace(['+', '*'], ['%20', '%2A'], $res);
- $res = \preg_replace('/%7E/i', '~', $res);
+ $res = \preg_replace('/%7E/i', '~', $res) ?? $res;
return $res;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private function percentEncode(string $str): string | |
| { | |
| $res = \urlencode($str); | |
| $res = \str_replace(['+', '*'], ['%20', '%2A'], $res); | |
| $res = \preg_replace('/%7E/i', '~', $res); | |
| return $res; | |
| } | |
| private function percentEncode(string $str): string | |
| { | |
| $res = \urlencode($str); | |
| $res = \str_replace(['+', '*'], ['%20', '%2A'], $res); | |
| $res = \preg_replace('/%7E/i', '~', $res) ?? $res; | |
| return $res; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Utopia/Messaging/Adapter/SMS/AlibabaCloud.php` around lines 106 - 113,
The percentEncode function uses preg_replace('/%7E/i', '~', $res) which can
return null on error but percentEncode is typed to return string; update
percentEncode to handle a null result from preg_replace (in function
percentEncode) by checking the return value and falling back to the original
$res or casting to string before returning so the method always returns a
string, ensuring any preg_replace failure is safely handled.
| $message = new SMS( | ||
| to: [\getenv('ALIBABA_CLOUD_TO')], | ||
| content: \json_encode(['code' => '123456']), | ||
| ); |
There was a problem hiding this comment.
Message content format may cause double JSON encoding.
The test passes \json_encode(['code' => '123456']) as content, but the adapter wraps this in another JSON object with the 'code' key (line 62 of the adapter). This results in double encoding: {"code":"{\"code\":\"123456\"}"}.
If the adapter fix to pass content directly as TemplateParam is applied, update this test accordingly:
🐛 Proposed fix (if adapter is updated to use content directly)
$message = new SMS(
to: [\getenv('ALIBABA_CLOUD_TO')],
- content: \json_encode(['code' => '123456']),
+ content: '{"code":"123456"}',
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $message = new SMS( | |
| to: [\getenv('ALIBABA_CLOUD_TO')], | |
| content: \json_encode(['code' => '123456']), | |
| ); | |
| $message = new SMS( | |
| to: [\getenv('ALIBABA_CLOUD_TO')], | |
| content: '{"code":"123456"}', | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Messaging/Adapter/SMS/AlibabaCloudTest.php` around lines 23 - 26, The
test is passing a JSON string as SMS content which causes double-encoding when
the adapter wraps it again; update the test in AlibabaCloudTest so SMS is
constructed with content as an associative array (e.g., ['code' => '123456']) or
otherwise match the adapter’s expected TemplateParam shape instead of passing
json_encode(...), and adjust any assertions that inspect the resulting
TemplateParam payload to expect the single-layer JSON (or raw array) produced by
the adapter; update references to the SMS constructor call (symbol: SMS) in the
test to pass the direct array content.
1 participant
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit
New Features
Tests