Add curl/swoole adapters #16
Conversation
WalkthroughAdds an Adapter interface and implements two adapters: Curl (cURL-based) and Swoole (Swoole Coroutine HTTP client). Refactors Client to accept an Adapter (defaults to Curl), centralizes request sending through adapters, and adds optional chunked streaming callbacks. Updates Response.json() error handling and blob() byte processing, applies strict types in multiple files, adds extensive PHPUnit tests for adapters and client, and introduces Docker-based CI and runtime test environment files. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 6
🤖 Fix all issues with AI agents
In `@src/Adapter/Curl.php`:
- Around line 48-52: CURLOPT_POSTFIELDS is being set unconditionally in the
$curlOptions array which can turn GET requests with an empty $body into requests
with a body; update the Curl.php code that builds $curlOptions so
CURLOPT_POSTFIELDS is only added after the initial array when ($body !== null &&
$body !== [] && $body !== '') or when the HTTP $method requires a body (e.g.,
POST, PUT, PATCH), by removing CURLOPT_POSTFIELDS from the initial $curlOptions
and conditionally assigning $curlOptions[CURLOPT_POSTFIELDS] when those checks
pass.
In `@src/Adapter/Swoole.php`:
- Around line 157-173: The code shadows the method parameter $body by assigning
$body = $client->body ?? '', causing the original request body to be lost on
redirects; rename the local variable (e.g., $responseBodyFromClient or
$responseBodyString) and use that renamed variable in the chunk creation and the
else branch (replace current uses of the overwritten $body there), leaving the
original $body parameter intact so the later check (if ($body !== null)) still
sees the original request body; update references to $chunkCallback and
$responseBody accordingly (e.g., create Chunk with data: $responseBodyFromClient
and set $responseBody = $responseBodyFromClient).
- Around line 128-132: The call to Swoole's addData currently passes form key
then value, but addData expects the data first and the field name second; in the
block handling $hasFiles inside the Swoole adapter (method using
$client->addData), swap the arguments so you call addData($value, $key) instead
of addData($key, $value), and check for any other occurrences of addData in the
Swoole adapter to apply the same argument order fix.
In `@tests/Adapter/CurlTest.php`:
- Around line 1-11: Tests in CurlTest hardcode requests to localhost:8001 and
share the production namespace; update tests/Adapter/CurlTest.php: change any
hardcoded base URL or port references in class CurlTest from 8001 to 8000 (so
they match the CI server started by php -S localhost:8000), and change the
namespace declaration from Utopia\Fetch\Adapter to a test-specific namespace
such as Utopia\Tests\Fetch\Adapter (and update any uses/imports inside the file
accordingly); ensure other test files that target the same server port are
updated similarly.
In `@tests/Adapter/SwooleTest.php`:
- Line 3: The test file currently declares the production namespace
"Utopia\Fetch\Adapter" which will conflict with source code; change the
namespace in the file to a test namespace (for example
"Utopia\Fetch\Tests\Adapter" or "Tests\Utopia\Fetch\Adapter") and ensure your
composer.json autoload-dev PSR-4 mapping matches that test namespace so the test
class is loaded correctly.
🧹 Nitpick comments (9)
src/Exception.php (1)
13-13: Consider adding type hint to constructor parameter.With
declare(strict_types=1)enabled, the$messageparameter should have astringtype hint to be fully consistent.Suggested fix
- public function __construct($message) + public function __construct(string $message)src/Adapter.php (1)
22-22: Consider documenting the chunk callback signature.The docblock could be more specific about the expected callback signature for better IDE support and developer experience.
Suggested enhancement
- * `@param` callable|null $chunkCallback Optional callback for streaming chunks + * `@param` callable(Chunk): void|null $chunkCallback Optional callback for streaming chunks, receives Chunk objectssrc/Adapter/Curl.php (2)
53-61: Unused$curlparameter is expected.The
$curlparameter inCURLOPT_HEADERFUNCTIONis required by cURL's callback signature but isn't needed here. This is a false positive from static analysis. You can suppress it with@param resource $curlin a docblock or prefix with underscore ($_curl).
62-75: Response body is empty when usingchunkCallback.When a
chunkCallbackis provided, chunks are passed to the callback but$responseBodyremains empty. The returnedResponsewill have an empty body, which may be unexpected for callers who want both streaming and the final body.Consider documenting this behavior or accumulating the body even when streaming:
📝 Option: Accumulate body alongside chunk callback
CURLOPT_WRITEFUNCTION => function ($ch, $data) use ($chunkCallback, &$responseBody, &$chunkIndex) { + $responseBody .= $data; if ($chunkCallback !== null) { $chunk = new Chunk( data: $data, size: strlen($data), timestamp: microtime(true), index: $chunkIndex++ ); $chunkCallback($chunk); - } else { - $responseBody .= $data; } return strlen($data); },Also applies to: 96-100
src/Client.php (1)
299-299: GraphQL validation with inline throw is functional but impacts readability.The inline
throwwithin a match expression works but is less readable. Consider extracting to a helper method or validating before the match.♻️ Alternative: Extract validation
if (is_array($body) && isset($this->headers['content-type'])) { + if ($this->headers['content-type'] === self::CONTENT_TYPE_GRAPHQL) { + if (!isset($body['query']) || !is_string($body['query'])) { + throw new Exception('GraphQL body must contain a "query" field with a string value'); + } + } $body = match ($this->headers['content-type']) { self::CONTENT_TYPE_APPLICATION_JSON => $this->jsonEncode($body), self::CONTENT_TYPE_APPLICATION_FORM_URLENCODED, self::CONTENT_TYPE_MULTIPART_FORM_DATA => self::flatten($body), - self::CONTENT_TYPE_GRAPHQL => isset($body['query']) && is_string($body['query']) ? $body['query'] : throw new Exception('GraphQL body must contain a "query" field with a string value'), + self::CONTENT_TYPE_GRAPHQL => $body['query'], default => $body, }; }tests/Adapter/SwooleTest.php (3)
26-28: Redundant null checks aftersetUpskip.When
markTestSkipped()is called insetUp(), PHPUnit stops test execution, so the$this->adapter === nullchecks in each test method are unnecessary. While defensive, they add noise.♻️ Consider removing redundant checks
Since
setUp()already callsmarkTestSkipped()when Swoole is unavailable, the adapter will never be null when test methods execute. You can safely remove these checks:public function testGetRequest(): void { - if ($this->adapter === null) { - $this->markTestSkipped('Swoole extension is not installed'); - } - $response = $this->adapter->send(Also applies to: 55-57, 86-88
35-41: InconsistentconnectTimeoutvalues across tests.Tests use
connectTimeout: 60000(60 seconds) while the Curl adapter and Client default to 5000ms (5 seconds). Consider aligning these values for consistency, or use a class constant for test timeouts.Also applies to: 65-71, 95-101
262-270: Test logic may produce unexpected results.If Swoole is not available,
setUp()marks the test as skipped and$this->adapterremainsnull. This test then checks if$classExistsis false and asserts$this->adapterisnull, but the test would have been skipped before reaching this point. The test only meaningfully executes when Swoole is available.♻️ Simplified availability test
public function testSwooleAvailability(): void { - $classExists = class_exists('Swoole\Coroutine\Http\Client'); - if ($classExists) { - $this->assertNotNull($this->adapter); - } else { - $this->assertNull($this->adapter); - } + // If we reach here, setUp() didn't skip, so Swoole is available + $this->assertTrue(Swoole::isAvailable()); + $this->assertNotNull($this->adapter); }tests/Adapter/CurlTest.php (1)
249-275: Consider adding assertion for file existence.The test assumes the file exists at the specified path. Adding a pre-condition check would make test failures more informative.
♻️ Optional improvement
public function testFileUpload(): void { $filePath = __DIR__ . '/../resources/logo.png'; + $this->assertFileExists($filePath, 'Test resource file not found'); + $body = [ 'file' => new \CURLFile(strval(realpath($filePath)), 'image/png', 'logo.png') ];
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Adapter.phpsrc/Adapter/Curl.phpsrc/Adapter/Swoole.phpsrc/Chunk.phpsrc/Client.phpsrc/Exception.phpsrc/Response.phptests/Adapter/CurlTest.phptests/Adapter/SwooleTest.phptests/ClientTest.phptests/ResponseTest.phptests/router.php
🧰 Additional context used
🧬 Code graph analysis (8)
src/Adapter.php (3)
src/Adapter/Curl.php (1)
send(31-104)src/Adapter/Swoole.php (1)
send(41-285)src/Response.php (1)
Response(12-115)
tests/ResponseTest.php (1)
src/Response.php (1)
blob(107-114)
tests/ClientTest.php (4)
src/Adapter/Curl.php (1)
Curl(17-105)src/Adapter/Swoole.php (1)
Swoole(17-286)src/Client.php (1)
Client(13-416)src/Response.php (1)
Response(12-115)
src/Adapter/Curl.php (4)
src/Chunk.php (1)
Chunk(12-67)src/Exception.php (1)
Exception(7-21)src/Response.php (1)
Response(12-115)src/Adapter.php (1)
send(26-33)
src/Adapter/Swoole.php (4)
src/Chunk.php (1)
Chunk(12-67)src/Exception.php (1)
Exception(7-21)src/Response.php (2)
Response(12-115)getStatusCode(72-75)src/Adapter.php (1)
send(26-33)
src/Response.php (1)
src/Exception.php (1)
Exception(7-21)
tests/Adapter/SwooleTest.php (4)
src/Chunk.php (4)
Chunk(12-67)getIndex(63-66)getSize(43-46)getData(33-36)src/Response.php (4)
Response(12-115)getStatusCode(72-75)json(91-101)getHeaders(63-66)src/Adapter/Swoole.php (2)
Swoole(17-286)send(41-285)src/Adapter.php (1)
send(26-33)
src/Client.php (5)
src/Adapter/Curl.php (2)
Curl(17-105)send(31-104)src/Chunk.php (1)
__construct(20-26)src/Exception.php (2)
__construct(13-16)Exception(7-21)src/Response.php (1)
__construct(40-48)src/Adapter.php (1)
send(26-33)
🪛 GitHub Actions: Tests
src/Adapter/Curl.php
[error] 91-91: Utopia\Fetch\Exception: Failed to connect to localhost port 8001 after 0 ms: Couldn't connect to server
tests/Adapter/CurlTest.php
[error] 24-24: PHPUnit test failed: testGetRequest could not complete due to connection to localhost:8001 being refused
[error] 50-50: PHPUnit test failed: testPostWithJsonBody could not complete due to connection to localhost:8001 being refused
[error] 76-76: PHPUnit test failed: testCustomTimeout could not complete due to connection to localhost:8001 being refused
[error] 99-99: PHPUnit test failed: testRedirectHandling could not complete due to connection to localhost:8001 being refused
[error] 124-124: PHPUnit test failed: testRedirectDisabled could not complete due to connection to localhost:8001 being refused
[error] 148-148: PHPUnit test failed: testChunkCallback could not complete due to connection to localhost:8001 being refused
[error] 183-183: PHPUnit test failed: testFormDataBody could not complete due to connection to localhost:8001 being refused
[error] 206-206: PHPUnit test failed: testResponseHeaders could not complete due to connection to localhost:8001 being refused
[error] 256-256: PHPUnit test failed: testFileUpload could not complete due to connection to localhost:8001 being refused
🪛 GitHub Check: CodeQL
src/Adapter/Swoole.php
[failure] 273-273:
Function Swoole\Coroutine\run not found.
🪛 PHPMD (2.15.0)
src/Adapter/Curl.php
53-53: Avoid unused parameters such as '$curl'. (undefined)
(UnusedFormalParameter)
🪛 PHPStan (2.1.33)
src/Adapter/Swoole.php
273-273: Function Swoole\Coroutine\run not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(function.notFound)
🔇 Additional comments (22)
src/Exception.php (1)
3-4: LGTM! Strict types declaration added.Good addition for type safety consistency across the codebase.
src/Chunk.php (1)
3-4: LGTM!Strict types declaration added. The class is well-structured with readonly properties and proper documentation.
tests/ResponseTest.php (1)
52-56: LGTM! Test logic now correctly mirrors the Response::blob() implementation.The updated test properly converts each character to its binary representation using an array, matching the implementation in
src/Response.phplines 106-113.src/Adapter.php (1)
12-34: Well-designed adapter interface.The interface provides a clean contract for HTTP adapters with appropriate flexibility for body types and optional streaming support. The documentation clearly describes all parameters and options.
tests/router.php (1)
44-46: LGTM! Port updated to 8001 for test consistency.The redirect URL port change aligns with the standardized port 8001 used consistently across all test files.
src/Response.php (2)
91-101: LGTM! Improved JSON error handling.Using
json_last_error()is the correct approach sincejson_decodecan returnnullfor valid JSON input (e.g., decoding the string"null"). This change properly distinguishes between valid null results and actual decoding errors.
107-114: LGTM! More efficient blob conversion.Array accumulation with
implode()is more efficient than repeated string concatenation, especially for larger bodies.tests/ClientTest.php (3)
14-20: LGTM! Good test for default adapter behavior.Validates that the Client correctly defaults to the Curl adapter when no adapter is explicitly provided.
317-380: LGTM! Consistent port update across data providers.All test URLs correctly updated to port 8001 for consistency with the test server configuration.
36-46: This test is correctly implemented — the Swoole adapter handles coroutine context internally.The Swoole adapter automatically manages the coroutine context requirement. It checks if already in a coroutine context using
\Swoole\Coroutine::getCid()(line 268) and, if not, wraps execution in\Swoole\Coroutine\run()(line 273). Therefore, the test does not need explicit wrapping and will function correctly as written.Likely an incorrect or invalid review comment.
src/Adapter/Curl.php (1)
87-103: LGTM! Proper error handling and resource cleanup.Good use of
try/finallyto ensurecurl_close()is always called, preventing resource leaks. The error handling correctly captures cURL errors and throws a domain-specific exception.src/Client.php (5)
43-53: LGTM! Clean adapter pattern implementation.The constructor correctly defaults to the Curl adapter when none is provided, maintaining backward compatibility while enabling adapter injection for testing and alternative implementations like Swoole.
33-33: DefaultconnectTimeoutreduced from 60s to 5s.This is a significant change that may affect users with slow network connections or services behind load balancers. Consider documenting this change in release notes.
171-173: LGTM! Correct bitwise OR reduction for JSON flags.Using
array_reducewith bitwise OR is the proper way to combine multiple JSON encode flags.
304-307: LGTM! Robust query parameter handling.Correctly detects existing query strings and uses the appropriate separator (
&vs?). Thertrimensures clean URL construction.
309-326: LGTM! Clean delegation to adapter.The options array is well-structured with proper fallbacks, and the adapter delegation is clean and maintainable.
tests/Adapter/SwooleTest.php (1)
13-19: LGTM! Proper conditional setup for optional extension.The
setUpmethod correctly skips tests when Swoole is not available, making the test suite portable across environments.src/Adapter/Swoole.php (2)
24-27: LGTM!The availability check correctly verifies the Swoole Coroutine HTTP client class exists.
267-274: Swoole coroutine context handling is correct. Add Swoole stubs to PHPStan configuration to suppress false positive warnings.The
Swoole\Coroutine\run()API exists in the Swoole extension. PHPStan and similar static analysis tools report undefined function warnings for Swoole functions because they lack stubs by default.The coroutine detection logic properly checks whether code is already running inside a coroutine using
Swoole\Coroutine::getCid()and creates a scheduler only when necessary.PHPStan configuration improvement
Add Swoole stubs to
phpstan.neon:parameters: level: 8 paths: - src - tests stubFiles: - vendor/swoole/ide-helper/output/swoole.phpOr install the Swoole IDE helper package for stubs (requires
composer require --dev swoole/ide-helper).tests/Adapter/CurlTest.php (3)
14-17: LGTM!The test setup correctly instantiates a fresh Curl adapter for each test case.
22-42: LGTM!The basic GET request test properly validates the response instance type, status code, and echoed method from the server.
145-175: Good chunk callback test coverage.The test properly validates chunk streaming behavior including sequential indices, non-zero sizes, and non-empty data. The assertion on line 167 ensures at least one chunk was received.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Fix port mismatch: update all test URLs from 8001 to 8000 to match CI server - Fix Curl adapter: only set CURLOPT_POSTFIELDS when body is non-empty - Fix Swoole adapter: correct addData parameter order (should be value, key) - Fix Swoole adapter: rename $body to $currentResponseBody to avoid shadowing - Fix phpstan.neon: add reportUnmatchedIgnoredErrors: false - Fix router.php: add Content-Type header for JSON responses - Fix ClientTest: handle missing content-type header for GET requests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/Adapter/SwooleTest.php`:
- Around line 259-270: The testSwooleAvailability() contains an unreachable else
branch because setUp() calls markTestSkipped() when Swoole is absent; update the
test to query availability directly via Swoole::isAvailable() (or equivalent
static helper) and assert that when Swoole::isAvailable() returns true
$this->adapter is not null and when false the test uses markTestSkipped() or
asserts adapter is null accordingly; specifically modify
testSwooleAvailability() to call Swoole::isAvailable() instead of
class_exists('Swoole\Coroutine\Http\Client') and remove the unreachable else
branch and any assumptions about $this->adapter being null when the test runs.
♻️ Duplicate comments (1)
tests/Adapter/SwooleTest.php (1)
3-3: Test namespace should differ from source namespace.This was flagged previously. The test file uses
Utopia\Fetch\Adapterwhich matches the source namespace rather than a test-specific namespace likeTests\Utopia\Fetch\Adapter.
🧹 Nitpick comments (3)
src/Adapter/Swoole.php (2)
205-239: Consider extracting body setup logic to reduce duplication.This block duplicates the body handling logic from lines 107-141. While both
addDatacalls now have correct parameter order, extracting this to a private helper method would improve maintainability.♻️ Proposed refactor - extract body setup helper
private function setupRequestBody(\Swoole\Coroutine\Http\Client $client, mixed $body, array $headers): void { if ($body === null) { return; } if (is_array($body)) { $hasFiles = false; $formData = []; foreach ($body as $key => $value) { if ($value instanceof \CURLFile || (is_string($value) && str_starts_with($value, '@'))) { $hasFiles = true; if ($value instanceof \CURLFile) { $client->addFile($value->getFilename(), $key, $value->getMimeType() ?: 'application/octet-stream', $value->getPostFilename() ?: basename($value->getFilename())); } elseif (str_starts_with($value, '@')) { $filePath = substr($value, 1); $client->addFile($filePath, $key); } } else { $formData[$key] = $value; } } if ($hasFiles) { foreach ($formData as $key => $value) { $client->addData($value, $key); } } elseif (isset($headers['content-type']) && $headers['content-type'] === 'application/x-www-form-urlencoded') { $client->setData(http_build_query($body)); } else { $client->setData($body); } } else { $client->setData($body); } }Then replace both occurrences (lines 107-141 and 205-239) with:
$this->setupRequestBody($client, $body, $headers);Note: Since this is within a closure, you may need to pass
$thisor make it a standalone function within the closure.
276-278: Consider preserving the original exception chain.Wrapping the exception loses the original stack trace, which can make debugging harder. Consider passing the original exception as the
previousparameter.♻️ Proposed fix
if ($exception !== null) { - throw new Exception($exception->getMessage()); + throw new Exception($exception->getMessage(), 0, $exception); }Note: This requires updating
src/Exception.phpto accept the$previousparameter in its constructor.tests/Adapter/SwooleTest.php (1)
13-19: Redundant null checks in test methods.When
markTestSkipped()is called insetUp(), PHPUnit throws aSkippedTestExceptionthat prevents the test method from executing. The null checks at the start of each test method (e.g., lines 26-28, 55-57, 86-88, etc.) are therefore unnecessary since the adapter will never be null when a test actually runs.Consider removing these redundant checks to simplify the tests:
♻️ Example refactor for testGetRequest
public function testGetRequest(): void { - if ($this->adapter === null) { - $this->markTestSkipped('Swoole extension is not installed'); - } - $response = $this->adapter->send(Apply the same pattern to all other test methods.
Also applies to: 24-28
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
phpstan.neonsrc/Adapter/Curl.phpsrc/Adapter/Swoole.phptests/Adapter/CurlTest.phptests/Adapter/SwooleTest.phptests/ClientTest.phptests/router.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Adapter/CurlTest.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Adapter/Curl.php (4)
src/Chunk.php (1)
Chunk(12-67)src/Exception.php (1)
Exception(7-21)src/Response.php (1)
Response(12-115)src/Adapter.php (1)
send(26-33)
tests/ClientTest.php (4)
src/Adapter/Curl.php (1)
Curl(17-108)src/Adapter/Swoole.php (1)
Swoole(17-286)src/Client.php (2)
Client(13-416)fetch(282-335)src/Response.php (2)
Response(12-115)getStatusCode(72-75)
🪛 PHPMD (2.15.0)
src/Adapter/Curl.php
52-52: Avoid unused parameters such as '$curl'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (16)
tests/router.php (1)
172-173: LGTM!Adding the
Content-Type: application/jsonheader for the default JSON response is correct and aligns with other response paths in this router that already set appropriate content types.phpstan.neon (1)
5-8: LGTM!The PHPStan configuration is correctly set up:
- Including
testsin analysis paths improves code quality coverage- Ignoring the Swoole coroutine function error is appropriate since Swoole is an optional extension
- Setting
reportUnmatchedIgnoredErrors: falseprevents CI failures in environments where Swoole is installedsrc/Adapter/Curl.php (3)
52-60: Unused$curlparameter is required by cURL callback signature.The static analysis tool flagged the unused
$curlparameter, but this is a false positive. TheCURLOPT_HEADERFUNCTIONcallback signature requires($ch, $header)as parameters per PHP documentation, even if$chis not used in the callback body.
82-84: LGTM!The conditional
CURLOPT_POSTFIELDSsetting correctly addresses the concern about GET requests. The body is only set when it's non-null, non-empty array, and non-empty string.
90-106: LGTM!The execution and error handling logic is correct:
- Uses
CURLOPT_WRITEFUNCTIONfor body accumulation (no need forCURLOPT_RETURNTRANSFER)- Proper error handling with
curl_error()- Resource cleanup guaranteed via
finallyblocksrc/Adapter/Swoole.php (3)
128-132: LGTM!The
addDataparameter order has been corrected to($value, $key), matching Swoole's method signature.
157-173: LGTM!The variable shadowing issue has been fixed by using
$currentResponseBodyinstead of overwriting the$bodyparameter.
267-274: LGTM!The coroutine context handling correctly detects whether execution is already within a coroutine and either executes directly or creates a new scheduler as needed.
tests/ClientTest.php (5)
6-7: LGTM!The imports for
CurlandSwooleadapters are correctly added to support the new adapter tests.
14-20: LGTM!The test correctly verifies that
Clientuses the Curl adapter by default and successfully makes a request.
25-31: LGTM!The test correctly verifies that a custom adapter (explicitly
Curl) can be injected into the Client.
36-46: LGTM!The Swoole adapter test correctly:
- Checks for Swoole availability before running
- Marks the test as skipped when Swoole is not installed
- Uses
127.0.0.1instead oflocalhost(good practice for Swoole)
108-124: LGTM!The content-type validation logic correctly handles:
- Case-insensitive header lookup (
Content-Typevscontent-type)- Only validating when headers or body were provided
- Stripping charset suffixes with
explode(';', ...)[0]- Asserting against the expected content-type based on test input
tests/Adapter/SwooleTest.php (3)
44-48: Good test coverage for adapter functionality.The tests comprehensively cover key adapter behaviors: basic requests, JSON body handling, redirect following, chunked responses, and response headers. The chunk callback test properly validates chunk properties including index, size, and data content.
Also applies to: 74-78, 131-134, 191-200
212-217: Array body handling is correctly supported with form-urlencoded encoding.The Swoole adapter's
send()method acceptsmixed $body, allowing both array and string types. When an array is passed withcontent-type: application/x-www-form-urlencoded, the adapter automatically encodes it usinghttp_build_query()before sending. The test code is correct.
31-31: No action required—URL format is intentional and functional.Both CurlTest and SwooleTest intentionally use scheme-less URLs. The Swoole adapter automatically adds
http://when a scheme is missing (line 58-61 of Swoole.php:if (!preg_match('~^https?://~i', $url)) { $url = 'http://' . $url; }). The different hosts (localhostvs127.0.0.1) reflect test environment requirements, not an inconsistency issue.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Add Dockerfile using appwrite/base:0.10.4 which includes Swoole - Add docker-compose.yml for running tests - Update tests workflow to use Docker for full Swoole test coverage Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Dockerfile`:
- Line 1: Update the Dockerfile FROM directive to use consistent keyword casing
by changing "FROM composer:2.0 as step0" to use uppercase AS ("FROM composer:2.0
AS step0"); this keeps the FROM/AS keywords consistent with the rest of the
Dockerfile (e.g., the other FROM ... AS occurrences).
- Around line 5-6: The Dockerfile currently tries to COPY ./composer.lock into
the image via the instruction COPY ./composer.lock /src/, but the file is
missing causing CI to fail; fix by either (A) removing the COPY ./composer.lock
/src/ line so the lock file is generated during build, or (B) generate and
commit a composer.lock locally (run composer install, then git add composer.lock
and git commit) so the existing COPY ./composer.lock /src/ succeeds — pick one
approach and update the Dockerfile or repository accordingly.
🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)
17-28: Update GitHub Actions to latest versions.The
docker/setup-buildx-action@v2anddocker/build-push-action@v3actions should be updated to v3 and v6 respectively to use the latest available versions and ensure compatibility with current runners.Proposed fix
- name: Set up Docker Buildx - uses: docker/setup-buildx-action@v2 + uses: docker/setup-buildx-action@v3 - name: Build image - uses: docker/build-push-action@v3 + uses: docker/build-push-action@v6 with: context: . push: false tags: fetch-dev load: true cache-from: type=gha cache-to: type=gha,mode=max
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/tests.ymlDockerfiledocker-compose.yml
🧰 Additional context used
🪛 actionlint (1.7.10)
.github/workflows/tests.yml
18-18: the runner of "docker/setup-buildx-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
21-21: the runner of "docker/build-push-action@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 GitHub Actions: Tests
Dockerfile
[error] 6-6: failed to compute cache key: failed to calculate checksum of ref uv17ojquobf1ymfstoyvdg4od::i0qtsc29jddind921kf67rrfr: "/composer.lock": not found
[warning] 1-1: FromAsCasing: 'as' and 'FROM' keywords' casing do not match (line 1)
🔇 Additional comments (2)
docker-compose.yml (1)
1-10: LGTM!The docker-compose configuration correctly builds from the local Dockerfile, maps the appropriate port, and mounts source directories for development. The volume mounts appropriately override the copied files for live development workflows.
.github/workflows/tests.yml (1)
30-36: Test execution approach looks good.The workflow correctly starts the containerized test environment and runs PHPUnit inside the container. The
sleep 5is acceptable for CI, though a health check could be more robust if startup issues arise.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| @@ -0,0 +1,26 @@ | |||
| FROM composer:2.0 as step0 | |||
There was a problem hiding this comment.
Fix FROM...AS casing inconsistency.
The as keyword should be uppercase AS to match the FROM keyword casing, consistent with line 11.
Proposed fix
-FROM composer:2.0 as step0
+FROM composer:2.0 AS step0📝 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.
| FROM composer:2.0 as step0 | |
| FROM composer:2.0 AS step0 |
🧰 Tools
🪛 GitHub Actions: Tests
[warning] 1-1: FromAsCasing: 'as' and 'FROM' keywords' casing do not match (line 1)
🤖 Prompt for AI Agents
In `@Dockerfile` at line 1, Update the Dockerfile FROM directive to use consistent
keyword casing by changing "FROM composer:2.0 as step0" to use uppercase AS
("FROM composer:2.0 AS step0"); this keeps the FROM/AS keywords consistent with
the rest of the Dockerfile (e.g., the other FROM ... AS occurrences).
| COPY ./composer.json /src/ | ||
| COPY ./composer.lock /src/ |
There was a problem hiding this comment.
Missing composer.lock file causes CI build failure.
The pipeline fails with: "/composer.lock": not found. Either generate and commit composer.lock by running composer install locally, or remove the COPY instruction if you intend to generate the lock file during the build.
Option 1: Remove composer.lock copy (generate during build)
COPY ./composer.json /src/
-COPY ./composer.lock /src/
-RUN composer install --ignore-platform-reqs --optimize-autoloader \
+RUN composer update --ignore-platform-reqs --optimize-autoloader \
--no-plugins --no-scripts --prefer-distOption 2: Generate and commit composer.lock (recommended for reproducible builds)
composer install
git add composer.lock
git commit -m "Add composer.lock"🧰 Tools
🪛 GitHub Actions: Tests
[error] 6-6: failed to compute cache key: failed to calculate checksum of ref uv17ojquobf1ymfstoyvdg4od::i0qtsc29jddind921kf67rrfr: "/composer.lock": not found
🤖 Prompt for AI Agents
In `@Dockerfile` around lines 5 - 6, The Dockerfile currently tries to COPY
./composer.lock into the image via the instruction COPY ./composer.lock /src/,
but the file is missing causing CI to fail; fix by either (A) removing the COPY
./composer.lock /src/ line so the lock file is generated during build, or (B)
generate and commit a composer.lock locally (run composer install, then git add
composer.lock and git commit) so the existing COPY ./composer.lock /src/
succeeds — pick one approach and update the Dockerfile or repository
accordingly.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.