SOLAPI PHP SDK 5.1.0 #22
Conversation
- Introduced AGENTS.md for detailed SDK structure, usage, and conventions. - Added CLAUDE.md to provide guidance for code usage and architecture overview.
- Introduced "Tidy First" principles by Kent Beck to enhance code quality and maintainability. - Added guidelines for separating structural and behavioral changes, and practical techniques for tidying code. - Emphasized the importance of tidying before feature additions and bug fixes.
- Transitioned from raw CURL to PSR-18 compatible HTTP client for improved security and flexibility. - Added new dependencies: `psr/http-client`, `psr/http-message`, and `nyholm/psr7`. - Introduced `HttpClient` class for handling HTTP requests and responses. - Deprecated `CurlException` in favor of `HttpException` for better error handling. - Added documentation on migration rationale from CURL to PSR-18.
…SDK. This file outlined the transition from raw CURL to a PSR-18 compatible HTTP client, detailing the benefits and changes associated with the migration.
- Introduced new BMS message types including TEXT, IMAGE, WIDE, WIDE_ITEM_LIST, COMMERCE, CAROUSEL_FEED, CAROUSEL_COMMERCE, and PREMIUM_VIDEO. - Added models for BMS components such as BmsButton, BmsCarousel, BmsCommerce, and BmsVideo. - Implemented validation logic for BMS message requirements. - Created example scripts for sending various BMS message types. - Updated composer.json to include PHPUnit for testing and added autoloading for development. - Enhanced .gitignore to exclude PHPUnit cache files and vendor directory. - Added unit tests for BMS models and validation logic.
Summary of ChangesHello @Palbahngmiyine, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the SOLAPI PHP SDK by moving to version 5.1.0. The core change involves modernizing the HTTP client layer to adhere to PSR-18 and PSR-7 standards, which improves the flexibility and maintainability of network requests. A major feature addition is the comprehensive support for KakaoTalk's Brand Message Service (BMS), introducing new data models and validation to facilitate sending rich, free-form messages. Furthermore, the project now includes a testing setup with PHPUnit and enhanced documentation to guide developers and AI tools. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a substantial and well-executed update that significantly modernizes the SDK. The migration from a direct cURL implementation to a PSR-7/PSR-18 compliant HTTP client is a major improvement for maintainability, testability, and interoperability. It's also great to see the removal of insecure defaults like disabling SSL verification. The addition of the comprehensive Kakao BMS feature set, complete with detailed models, a validator, examples, and unit tests, is excellent. I have a few suggestions to further refine the new HTTP client implementation and ensure a smooth transition for users.
| "require": { | ||
| "php": ">=7.1", | ||
| "ext-curl": "*", | ||
| "ext-json": "*" | ||
| "ext-json": "*", | ||
| "psr/http-client": "^1.0", | ||
| "psr/http-message": "^1.0 || ^2.0", | ||
| "nyholm/psr7": "^1.5" | ||
| }, |
There was a problem hiding this comment.
This update changes the underlying HTTP transport from cURL to a file_get_contents-based stream wrapper. This introduces a new runtime requirement: allow_url_fopen must be enabled in php.ini. This is a significant change, as this setting is often disabled on production or shared hosting environments for security reasons. If allow_url_fopen is disabled, the SDK will fail. This should be clearly documented as a requirement or potential breaking change in the README or migration guide.
| public static function getInstance(string $apiKey, string $apiSecretKey): Fetcher | ||
| { | ||
| if (!isset(Fetcher::$singleton)) Fetcher::$singleton = new Fetcher($apiKey, $apiSecretKey); | ||
| if (!isset(Fetcher::$singleton)) { | ||
| Fetcher::$singleton = new Fetcher($apiKey, $apiSecretKey); | ||
| } | ||
| return Fetcher::$singleton; | ||
| } |
There was a problem hiding this comment.
The constructor for Fetcher was updated to accept a ClientInterface for dependency injection, which is a great improvement. However, the static getInstance method does not provide a way to pass this custom client, always using the default HttpClient. This undermines the benefit of the new constructor for users of the singleton pattern.
To fully support client injection, getInstance should also accept an optional ClientInterface argument. This would also require updating the SolapiMessageService constructor to pass the client along.
| } catch (ClientExceptionInterface $e) { | ||
| throw new CurlException($e->getMessage(), null, null, $e); | ||
| } |
There was a problem hiding this comment.
The Fetcher catches a ClientExceptionInterface and wraps it in a CurlException. For consistency and clarity, this should be an HttpException, as the underlying client may not be cURL-based. Using the more generic exception type makes the code more accurate and easier to understand.
} catch (ClientExceptionInterface $e) {
throw new HttpException($e->getMessage(), null, null, $e);
}| protected $timeout; | ||
| protected $connectTimeout; | ||
| protected $verifySsl; | ||
|
|
||
| public function __construct(array $options = []) | ||
| { | ||
| $this->timeout = $options['timeout'] ?? 30.0; | ||
| $this->connectTimeout = $options['connect_timeout'] ?? 10.0; | ||
| $this->verifySsl = $options['verify'] ?? true; | ||
| } |
There was a problem hiding this comment.
The $connectTimeout property is initialized in the constructor but is never used within the sendRequest method. The http stream wrapper used by file_get_contents does not have a separate connect_timeout option; the timeout option applies to the entire stream reading operation. This property is effectively dead code and could be misleading. It should be removed to avoid confusion.
| if ($responseBody === false) { | ||
| $error = error_get_last(); | ||
| throw new CurlException( | ||
| $error['message'] ?? 'HTTP request failed: ' . $url, | ||
| null, | ||
| null, | ||
| null | ||
| ); | ||
| } |
There was a problem hiding this comment.
The sendRequest method throws a CurlException on failure. However, the underlying implementation has been changed from cURL to file_get_contents. Throwing a CurlException from non-cURL code is confusing. It would be clearer to throw the more generic HttpException directly. CurlException can be kept for backward compatibility, but the library's internal code should prefer the more accurate exception type.
if ($responseBody === false) {
$error = error_get_last();
throw new HttpException(
$error['message'] ?? 'HTTP request failed: ' . $url
);
}- Updated .gitignore to exclude .env and .omc files. - Added new PHPUnit test scripts for unit and end-to-end testing of BMS message types. - Modified composer.json to include additional test scripts for unit, end-to-end, and coverage testing. - Updated phpunit.xml to define separate test suites for Unit and E2E tests. - Introduced a bootstrap file for loading environment variables and Composer autoloading.
1 participant
No description provided.