Add curl/swoole adapters #16
Conversation
WalkthroughAdds a new Adapter interface and two implementations (Curl and Swoole) to standardize HTTP requests with support for streaming chunks, redirects, SSL, proxies, timeouts, and resource cleanup. Refactors Client to accept an Adapter (defaults to Curl) and delegates request sending. Improves Response text/json/blob handling and enables strict types in several files. Adds comprehensive unit tests for both adapters and client changes, updates test router, introduces Dockerfile and docker-compose, switches CI to a Docker-based workflow, updates phpstan, and adds a dev dependency for Swoole IDE helper. 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. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/Adapter/Swoole.php (4)
🔇 Additional comments (6)
✏️ Tip: You can disable this entire section by setting 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 <[email protected]>
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 <[email protected]>
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.
- Remove COPY ./composer.lock line since file is in .gitignore - Use composer update instead of install (generates lock at build time) - Fix FROM...AS casing inconsistency Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
abnegate
force-pushed
the
feat-adapters
branch
from
8317053 to
0a98001
Compare
- Curl: Store and reuse CurlHandle with curl_reset() between requests - Swoole: Cache clients by host:port:ssl key for connection reuse - Replace FQNs with proper use statements - Extract configureBody() helper method to reduce duplication - Enable keep_alive for Swoole clients - Add __destruct() to properly close handles/clients Co-Authored-By: Claude Opus 4.5 <[email protected]>
abnegate
force-pushed
the
feat-adapters
branch
from
b5b427b to
cbacfea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Adapter/Swoole.php`:
- Around line 287-291: PHPStan is reporting Swoole\Coroutine::run as missing
because the Swoole stubs aren't included; add the swoole/ide-helper package as a
dev dependency and configure phpstan to load its stubs so calls like
Coroutine::getCid() and run($executeRequest) are recognized. Specifically, add
swoole/ide-helper to composer require --dev, then add the ide-helper stubs.php
path to phpstan.neon under parameters->stubFiles (pointing to
vendor/swoole/ide-helper/stubs.php), and ensure the stubs version matches your
installed Swoole/OpenSwoole 4.x so Coroutine::run is declared.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Adapter/Curl.phpsrc/Adapter/Swoole.php
🧰 Additional context used
🧬 Code graph analysis (1)
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)
🪛 GitHub Check: CodeQL
src/Adapter/Swoole.php
[failure] 16-16:
Used function Swoole\Coroutine\run not found.
🪛 PHPMD (2.15.0)
src/Adapter/Curl.php
71-71: Avoid unused parameters such as '$curl'. (undefined)
(UnusedFormalParameter)
🪛 PHPStan (2.1.33)
src/Adapter/Swoole.php
16-16: Used function Swoole\Coroutine\run not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(function.notFound)
🔇 Additional comments (9)
src/Adapter/Curl.php (3)
27-36: Handle reuse/reset looks solid.
Please confirm the minimum supported PHP version includes theCurlHandletype and the cURL extension is guaranteed in target runtimes.
50-121: Send path is clean and readable.
Please verifyCURLOPT_CONNECTTIMEOUT_MS/CURLOPT_TIMEOUT_MSare available in the minimum supported cURL version.
127-133: Destructor cleanup is fine.
Closing the cached handle should prevent leaks; please confirm this aligns with your long‑lived process expectations.src/Adapter/Swoole.php (6)
35-38: Availability check is straightforward.
Please confirm your support matrix (Swoole vs OpenSwoole) matches theclass_existstarget.
48-57: Client cache looks fine.
Keying by host/port/SSL is sensible; just ensure this reuse pattern matches Swoole client lifecycle guarantees.
67-75: Close/removal logic looks good.
No concerns here.
307-313: Destructor cleanup looks good.
Closing cached clients on teardown is appropriate.
85-126: Remove this concern — Swoole clears request body data automatically after each request.The Swoole HTTP client does not retain request body data between requests. According to OpenSwoole documentation, "Any data which is set is not preserved once a request is made." When
$bodyisnulland the method returns early, there is no risk of body data leakage from prior requests. The proposed fix is unnecessary.Likely an incorrect or invalid review comment.
196-205: Always reset headers when reusing cached client.
The Swoole HTTP client persists headers across requests when the connection is pooled/reused. CallingsetHeaders([])properly resets headers and is supported by Swoole/OpenSwoole. However, this fix is only necessary if the adapter actually pools/reuses client connections across requests. Verify that your implementation reuses clients; if so, apply the proposed fix to prevent credential leakage.🩹 Proposed fix
- if (!empty($allHeaders)) { - $client->setHeaders($allHeaders); - } + // Always set headers to avoid leaking previous request headers + $client->setHeaders($allHeaders);
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Curl: Accept array of CURLOPT_* options in constructor - Swoole: Accept array of client settings in constructor - Config options are merged with defaults (user config takes precedence) - Extracted buildClientSettings() helper in Swoole adapter Example usage: // Curl with custom SSL options new Curl([CURLOPT_SSL_VERIFYPEER => false]); // Swoole with long connections new Swoole(['keep_alive' => true, 'timeout' => 60]); Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Adapter/Curl.php`:
- Around line 42-48: The getHandle method in Curl.php assigns curl_init()
directly to $this->handle but curl_init() can return false in PHP 8.1+, causing
a TypeError when the method's return type is CurlHandle; change getHandle to
check the result of curl_init() and if it returns false throw a descriptive
RuntimeException (or custom exception) instead of assigning/returning false,
otherwise assign the CurlHandle to $this->handle and return it; ensure
references to curl_reset($this->handle) remain guarded by the handle being a
valid CurlHandle.
In `@src/Adapter/Swoole.php`:
- Around line 61-70: The getClient method currently returns a cached
SwooleClient from $this->clients which allows multiple coroutines to share the
same Swoole\Coroutine\Http\Client and causes unsafe concurrent use; change
getClient(SwooleClient) to always create and return a fresh SwooleClient
instance (new SwooleClient($host, $port, $ssl)) per call instead of using
$this->clients, or alternatively replace the cache with a proper pool
implementation (e.g., Swoole\ClientPool/ConnectionPool) and update callers to
checkout/return clients; update references to getClient and remove reliance on
$this->clients to ensure exclusive client usage per coroutine.
🧹 Nitpick comments (4)
src/Adapter/Curl.php (2)
86-94: Avoid mixing headers across redirects.When
CURLOPT_FOLLOWLOCATIONis enabled, the header callback is invoked per response hop. Consider clearing$responseHeaderson each new status line so the finalResponsereflects the terminal hop only.♻️ Suggested tweak
CURLOPT_HEADERFUNCTION => function ($curl, $header) use (&$responseHeaders) { $len = strlen($header); + if (preg_match('/^HTTP\/\d+(?:\.\d+)?\s+\d+/', $header) === 1) { + $responseHeaders = []; + return $len; + } $header = explode(':', $header, 2); if (count($header) < 2) { return $len; }
95-106: Streaming path returns an empty body.When
$chunkCallbackis set,$responseBodystays empty. If callers still expectResponse::getBody()to contain the full payload, consider optionally buffering (or explicitly documenting that body will be empty in streaming mode).src/Adapter/Swoole.php (2)
98-102: Verify body reset on reused clients.
When$body === null,configureBody()returns without clearing prior payload. If Swoole retains previous request data on a reused client, this can leak the prior body into the next request. Please confirm behavior; if it persists, explicitly reset.♻️ Possible fix (if Swoole retains prior data)
- if ($body === null) { - return; - } + if ($body === null) { + $client->setData(''); + return; + }
249-261: Consider keepingResponsebody when chunkCallback is used.
Right now, when$chunkCallbackis set,$responseBodyremains empty even though Swoole already buffers the full body. If callers still expectResponse->text()/json()to work, consider assigning$responseBodyregardless or documenting the behavior.💡 Optional tweak for consistency
- if ($chunkCallback !== null && !empty($currentResponseBody)) { + if ($chunkCallback !== null && $currentResponseBody !== '') { $chunk = new Chunk( data: $currentResponseBody, size: strlen($currentResponseBody), timestamp: microtime(true), index: $chunkIndex++ ); $chunkCallback($chunk); - } else { - $responseBody = $currentResponseBody; } + $responseBody = $currentResponseBody;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Adapter/Curl.phpsrc/Adapter/Swoole.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Adapter/Swoole.php (5)
src/Chunk.php (2)
Chunk(12-67)__construct(20-26)src/Exception.php (2)
Exception(7-21)__construct(13-16)src/Response.php (3)
Response(12-115)__construct(40-48)getStatusCode(72-75)src/Adapter/Curl.php (2)
__construct(32-35)send(65-140)src/Adapter.php (1)
send(26-33)
src/Adapter/Curl.php (5)
src/Chunk.php (2)
Chunk(12-67)__construct(20-26)src/Exception.php (2)
Exception(7-21)__construct(13-16)src/Response.php (2)
Response(12-115)__construct(40-48)src/Client.php (1)
__construct(50-53)src/Adapter.php (1)
send(26-33)
🪛 PHPMD (2.15.0)
src/Adapter/Curl.php
86-86: Avoid unused parameters such as '$curl'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (7)
src/Adapter/Curl.php (2)
32-35: Clean config injection.Keeps adapter-level overrides isolated; looks good.
145-149: Destructor cleanup looks good.Closing the cached handle avoids resource leaks.
src/Adapter/Swoole.php (5)
33-41: LGTM: clean config capture.
48-51: LGTM: lightweight availability check.
80-87: LGTM: cache cleanup is clear and safe.
148-158: LGTM: defaults merged cleanly with user config.
331-336: LGTM: destructor closes pooled clients.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Swoole adapter: - Add `coroutines` boolean constructor parameter (default: true) - When true: automatically wraps requests in coroutine scheduler - When false: executes directly (for use inside Swoole servers) - Refactored request logic into executeRequest() method Curl adapter: - Fix getHandle() to properly handle curl_init() returning false - Throw descriptive Exception instead of causing TypeError Co-Authored-By: Claude Opus 4.5 <[email protected]>
Both adapters now use discoverable named parameters instead of opaque arrays: Curl adapter parameters: - sslVerifyPeer, sslVerifyHost, sslCertificate, sslKey - caInfo, caPath - proxy, proxyUserPwd, proxyType - httpVersion, tcpKeepAlive, tcpKeepIdle, tcpKeepInterval - bufferSize, verbose Swoole adapter parameters: - coroutines, keepAlive, socketBufferSize, httpCompression - sslVerifyPeer, sslHostName, sslCafile, sslAllowSelfSigned - packageMaxLength, websocketMask, websocketCompression - bindAddress, bindPort, lowaterMark All parameters have sensible defaults, enabling IDE autocompletion and discoverability without requiring documentation lookup. Co-Authored-By: Claude Opus 4.5 <[email protected]>
abnegate
force-pushed
the
feat-adapters
branch
2 times, most recently
from
5d11230 to
3c51616
Compare
- coroutines=true: Uses Swoole\Coroutine\Http\Client (default) - coroutines=false: Uses Swoole\Http\Client (sync/blocking) Updated type hints to support both client types via union types. Co-Authored-By: Claude Opus 4.5 <[email protected]>
abnegate
force-pushed
the
feat-adapters
branch
from
3c51616 to
0c182a3
Compare
- Import Swoole\Http\Client as SyncClient - Remove @phpstan-ignore annotations - Use @var CoClient annotation for type compatibility Co-Authored-By: Claude Opus 4.5 <[email protected]>
abnegate
force-pushed
the
feat-adapters
branch
from
38aeb2d to
9652041
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/Adapter/Swoole.php`:
- Around line 317-339: The redirect handling reuses $allHeaders across origin
changes and may leak sensitive headers; update the redirect branch in Swoole.php
where $newHost/$newPort/$newSsl are compared to $host/$port/$ssl (inside the
block that calls $this->getClient, $client->set, $client->setMethod, and
$client->setHeaders) to filter out sensitive headers before calling
$client->setHeaders: remove Authorization, Cookie, Proxy-Authorization and Host
on cross-origin redirects (i.e. when $newHost !== $host || $newPort !== $port ||
$newSsl !== $ssl), and if you need a Host header for the new request set it
explicitly to $newHost (or omit it to let the client set it), then pass the
sanitized headers to $client->setHeaders so cookies and auth are not leaked to
the new origin.
- Around line 57-75: The constructor currently defaults to coroutines=true which
leads to a fatal error if the CoClient class isn't present; update the
constructor (or getClient) to detect at instantiation whether the coroutine
client class (Swoole\Coroutine\Http\Client or CoClient) exists and if not set
$this->coroutines = false (or otherwise fall back) so that getClient() and
send() will create the sync Swoole\Http\Client instead; ensure the logic
references __construct, isAvailable, getClient, send, CoClient and
Swoole\Http\Client so the adapter auto-falls back to the sync client when
coroutine client classes are unavailable.
- Around line 120-164: getClient() currently declares return type CoClient but
may instantiate Swoole\Http\Client when $this->coroutines is false; likewise
configureBody() type-hints CoClient but can receive Swoole\Http\Client. Fix by
changing the signatures and PHPDoc to use a union type
CoClient|Swoole\Http\Client for getClient()'s return and for configureBody()'s
$client parameter (and update any related PHPDoc annotations), ensuring both
concrete client types are accepted by methods that use them.
♻️ Duplicate comments (2)
src/Adapter/Swoole.php (2)
124-134: Avoid sharing a cached coroutine client across concurrent coroutines.The host-level cache returns the same
Swoole\Coroutine\Http\Clientfor multiple concurrent requests, which can interleave state and break requests. Prefer a per-request client or a coroutine-aware pool.
8-10: Static analysis: add Swoole stubs soSwoole\Http\Clientresolves in CI.PHPStan/CodeQL report
Swoole\Http\Clientas missing. Add the Swoole IDE stubs in dev and include them in phpstan config.#!/bin/bash # Inspect phpstan config for stubFiles/scanFiles entries fd -a -g 'phpstan*.neon*' -x sed -n '1,200p' {}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Adapter/Swoole.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Adapter/Swoole.php (4)
src/Chunk.php (2)
Chunk(12-67)__construct(20-26)src/Exception.php (2)
Exception(7-21)__construct(13-16)src/Response.php (3)
Response(12-115)__construct(40-48)getStatusCode(72-75)src/Adapter.php (1)
send(26-33)
🪛 GitHub Check: CodeQL
src/Adapter/Swoole.php
[failure] 129-129:
Instantiated class Swoole\Http\Client not found.
🪛 PHPStan (2.1.33)
src/Adapter/Swoole.php
129-129: Instantiated class Swoole\Http\Client not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| private function getClient(string $host, int $port, bool $ssl): CoClient | ||
| { | ||
| $key = "{$host}:{$port}:" . ($ssl ? '1' : '0'); | ||
|
|
||
| if (!isset($this->clients[$key])) { | ||
| if ($this->coroutines) { | ||
| $this->clients[$key] = new CoClient($host, $port, $ssl); | ||
| } else { | ||
| /** @var CoClient $client */ | ||
| $client = new SyncClient($host, $port, $ssl); | ||
| $this->clients[$key] = $client; | ||
| } | ||
| } | ||
|
|
||
| return $this->clients[$key]; | ||
| } | ||
|
|
||
| /** | ||
| * Close and remove a client from the cache | ||
| * | ||
| * @param string $host | ||
| * @param int $port | ||
| * @param bool $ssl | ||
| * @return void | ||
| */ | ||
| private function closeClient(string $host, int $port, bool $ssl): void | ||
| { | ||
| $key = "{$host}:{$port}:" . ($ssl ? '1' : '0'); | ||
|
|
||
| if (isset($this->clients[$key])) { | ||
| $this->clients[$key]->close(); | ||
| unset($this->clients[$key]); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Configure body data on the client | ||
| * | ||
| * @param CoClient $client Swoole HTTP client | ||
| * @param mixed $body | ||
| * @param array<string, string> $headers | ||
| * @return void | ||
| */ | ||
| private function configureBody(CoClient $client, mixed $body, array $headers): void | ||
| { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && find . -name "composer.json" -type f | head -5Repository: utopia-php/fetch
Length of output: 115
🏁 Script executed:
cd /repo && cat composer.json | grep -A 5 '"php"'Repository: utopia-php/fetch
Length of output: 115
🏁 Script executed:
cd /repo && wc -l src/Adapter/Swoole.phpRepository: utopia-php/fetch
Length of output: 115
🏁 Script executed:
cd /repo && cat -n src/Adapter/Swoole.php | head -50Repository: utopia-php/fetch
Length of output: 115
🏁 Script executed:
find . -name "composer.json" -type fRepository: utopia-php/fetch
Length of output: 1401
🏁 Script executed:
cat composer.json | grep -A 5 '"php"'Repository: utopia-php/fetch
Length of output: 214
🏁 Script executed:
wc -l src/Adapter/Swoole.phpRepository: utopia-php/fetch
Length of output: 86
🏁 Script executed:
head -70 src/Adapter/Swoole.phpRepository: utopia-php/fetch
Length of output: 2327
🏁 Script executed:
sed -n '1,150p' src/Adapter/Swoole.php | tail -80Repository: utopia-php/fetch
Length of output: 2500
🏁 Script executed:
sed -n '100,220p' src/Adapter/Swoole.phpRepository: utopia-php/fetch
Length of output: 3777
🏁 Script executed:
grep -n "class SyncClient\|use.*SyncClient" src/Adapter/Swoole.phpRepository: utopia-php/fetch
Length of output: 42
🏁 Script executed:
find . -name "*.php" -type f | xargs grep -l "class SyncClient"Repository: utopia-php/fetch
Length of output: 44
🏁 Script executed:
grep -n "SyncClient\|Swoole.Http.Client" src/Adapter/Swoole.phpRepository: utopia-php/fetch
Length of output: 327
🏁 Script executed:
head -15 src/Adapter/Swoole.phpRepository: utopia-php/fetch
Length of output: 328
🏁 Script executed:
sed -n '145,165p' src/Adapter/Swoole.phpRepository: utopia-php/fetch
Length of output: 670
Fix type mismatch: getClient() returns Swoole\Http\Client but declares return type CoClient.
When $coroutines is false, getClient() instantiates Swoole\Http\Client via dynamic class loading (line 128), but the method signature declares return type CoClient (line 118). This causes a type mismatch that will fail strict type checking. The configureBody() method also type-hints CoClient $client but receives instances of Swoole\Http\Client. Use a union type to correctly declare the actual return type. The project requires PHP >=8.1, which supports union types.
Proposed fix
- /**
- * `@var` array<string, CoClient>
- */
+ /**
+ * `@var` array<string, CoClient|Swoole\Http\Client>
+ */
private array $clients = [];
- private function getClient(string $host, int $port, bool $ssl): CoClient
+ private function getClient(string $host, int $port, bool $ssl): CoClient|Swoole\Http\Client
- private function configureBody(CoClient $client, mixed $body, array $headers): void
+ private function configureBody(CoClient|Swoole\Http\Client $client, mixed $body, array $headers): void🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 129-129:
Instantiated class Swoole\Http\Client not found.
🪛 PHPStan (2.1.33)
129-129: Instantiated class Swoole\Http\Client not found.
Learn more at https://phpstan.org/user-guide/discovering-symbols
(class.notFound)
🤖 Prompt for AI Agents
In `@src/Adapter/Swoole.php` around lines 120 - 164, getClient() currently
declares return type CoClient but may instantiate Swoole\Http\Client when
$this->coroutines is false; likewise configureBody() type-hints CoClient but can
receive Swoole\Http\Client. Fix by changing the signatures and PHPDoc to use a
union type CoClient|Swoole\Http\Client for getClient()'s return and for
configureBody()'s $client parameter (and update any related PHPDoc annotations),
ensuring both concrete client types are accepted by methods that use them.
| if ($allowRedirects && in_array($statusCode, [301, 302, 303, 307, 308]) && $redirectCount < $maxRedirects) { | ||
| $location = $client->headers['location'] ?? $client->headers['Location'] ?? null; | ||
| if ($location !== null) { | ||
| $redirectCount++; | ||
| if (strpos($location, 'http') === 0) { | ||
| $parsedLocation = parse_url($location); | ||
| $newHost = $parsedLocation['host'] ?? $host; | ||
| $newPort = $parsedLocation['port'] ?? (isset($parsedLocation['scheme']) && $parsedLocation['scheme'] === 'https' ? 443 : 80); | ||
| $newSsl = ($parsedLocation['scheme'] ?? 'http') === 'https'; | ||
| $path = ($parsedLocation['path'] ?? '/') . (isset($parsedLocation['query']) ? '?' . $parsedLocation['query'] : ''); | ||
|
|
||
| if ($newHost !== $host || $newPort !== $port || $newSsl !== $ssl) { | ||
| $host = $newHost; | ||
| $port = $newPort; | ||
| $ssl = $newSsl; | ||
| $client = $this->getClient($host, $port, $ssl); | ||
| $client->set($this->buildClientSettings($timeout, $connectTimeout)); | ||
| $client->setMethod($method); | ||
| if (!empty($allHeaders)) { | ||
| $client->setHeaders($allHeaders); | ||
| } | ||
| $this->configureBody($client, $body, $headers); | ||
| } |
There was a problem hiding this comment.
Strip sensitive headers on cross-origin redirects.
When the redirect changes host/port/SSL, reusing $allHeaders can leak Authorization/Cookie/Proxy-Authorization to a different origin. Drop sensitive headers (and Host) on cross-origin redirects.
🔒 Proposed fix
if ($newHost !== $host || $newPort !== $port || $newSsl !== $ssl) {
$host = $newHost;
$port = $newPort;
$ssl = $newSsl;
+ $allHeaders = array_filter(
+ $allHeaders,
+ fn($v, $k) => !in_array(strtolower($k), ['authorization', 'proxy-authorization', 'cookie', 'host'], true),
+ ARRAY_FILTER_USE_BOTH
+ );
$client = $this->getClient($host, $port, $ssl);🤖 Prompt for AI Agents
In `@src/Adapter/Swoole.php` around lines 317 - 339, The redirect handling reuses
$allHeaders across origin changes and may leak sensitive headers; update the
redirect branch in Swoole.php where $newHost/$newPort/$newSsl are compared to
$host/$port/$ssl (inside the block that calls $this->getClient, $client->set,
$client->setMethod, and $client->setHeaders) to filter out sensitive headers
before calling $client->setHeaders: remove Authorization, Cookie,
Proxy-Authorization and Host on cross-origin redirects (i.e. when $newHost !==
$host || $newPort !== $port || $newSsl !== $ssl), and if you need a Host header
for the new request set it explicitly to $newHost (or omit it to let the client
set it), then pass the sanitized headers to $client->setHeaders so cookies and
auth are not leaked to the new origin.
- Import Swoole\Http\Client as SyncClient - Use SyncClient::class instead of string names - Add PHPStan ignore pattern for missing Swoole\Http\Client stubs Co-Authored-By: Claude Opus 4.5 <[email protected]>
The Swoole extension isn't installed in CI for static analysis, causing PHPStan to not find Swoole classes. The ide-helper package provides stubs so PHPStan can analyze the code properly. Co-Authored-By: Claude Opus 4.5 <[email protected]>
abnegate
force-pushed
the
feat-adapters
branch
from
c7bf193 to
2e2cb20
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Adapter/Swoole.php`:
- Around line 192-199: The check for 'content-type' being set is case-sensitive
and can miss headers like 'Content-Type'; inside the Swoole adapter's send logic
where $headers is inspected (near variables $hasFiles, $formData and
client->setData calls), normalize header keys to lowercase (or perform a
case-insensitive lookup) before checking content-type, matching the
normalization used for response headers in this file; update the code that
branches on application/x-www-form-urlencoded to use the normalized header key
(e.g., lowercased $headers['content-type']) so the urlencoded path is reliably
detected when calling send().
♻️ Duplicate comments (4)
src/Adapter/Swoole.php (4)
57-74: Auto-disable coroutine mode when the coroutine client isn’t available.Line 73 sets
$this->coroutinesdirectly. If onlySwoole\Http\Clientexists,send()will still tryCoClient, causing a fatal error. Guard here (or ingetClient()) so the adapter gracefully falls back to sync mode.✅ Suggested fix
- $this->coroutines = $coroutines; + $this->coroutines = $coroutines && class_exists(CoClient::class);
120-164: Fix return/parameter types for sync client mode (TypeError risk).Line 120 promises
CoClient, but Line 129 constructsSyncClientwhen coroutines are off. Withstrict_types=1, this will throw.configureBody()also type-hintsCoClientonly. Use a union type (PHP ≥ 8.1) and update the cache docblock to reflect both types.✅ Suggested fix
- /** - * `@var` array<string, CoClient> - */ + /** + * `@var` array<string, CoClient|SyncClient> + */ private array $clients = []; - private function getClient(string $host, int $port, bool $ssl): CoClient + private function getClient(string $host, int $port, bool $ssl): CoClient|SyncClient - private function configureBody(CoClient $client, mixed $body, array $headers): void + private function configureBody(CoClient|SyncClient $client, mixed $body, array $headers): void
122-134: Avoid sharing one client across concurrent coroutines.The per-host cache returns a single client instance (Line 124+). Concurrent
send()calls can interleave on the sameSwoole\Coroutine\Http\Client, which is not safe for concurrent use. Prefer a client-per-request model or a coroutine-safe pool with exclusive checkout.
328-337: Strip sensitive headers on cross-origin redirects.When host/port/SSL changes,
$allHeadersis reused as-is (Line 335), which can leakAuthorization,Cookie,Proxy-Authorization, or staleHostto a new origin. Filter them beforesetHeaders()in that branch.✅ Suggested fix
if ($newHost !== $host || $newPort !== $port || $newSsl !== $ssl) { $host = $newHost; $port = $newPort; $ssl = $newSsl; + $allHeaders = array_filter( + $allHeaders, + fn($v, $k) => !in_array(strtolower($k), ['authorization', 'proxy-authorization', 'cookie', 'host'], true), + ARRAY_FILTER_USE_BOTH + ); $client = $this->getClient($host, $port, $ssl); $client->set($this->buildClientSettings($timeout, $connectTimeout)); $client->setMethod($method); if (!empty($allHeaders)) { $client->setHeaders($allHeaders); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
phpstan.neonsrc/Adapter/Swoole.php
🚧 Files skipped from review as they are similar to previous changes (1)
- phpstan.neon
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Client.php`:
- Around line 304-306: The code chooses $separator before trimming $url which
yields invalid URLs like http://example.com&foo=bar when $url ends with '?'; fix
this in the block that builds the query by first trimming trailing '?' and '&'
from $url (use rtrim($url, '?&') and assign back to $url or a new $baseUrl),
then determine $separator with str_contains($url, '?') ? '&' : '?', and finally
set $url = $url . $separator . http_build_query($query); reference the $url and
$query variables in Client.php to locate and update this logic.
♻️ Duplicate comments (4)
src/Adapter/Swoole.php (4)
133-152: Type mismatch: return type declaresCoClientbut may returnSwoole\Http\Client.When
$this->coroutinesis false, the method instantiates a sync client but the return type is declared asCoClient. This causes static analysis failures and misrepresents the actual behavior.Suggested fix - use union return type
- private function getClient(string $host, int $port, bool $ssl): CoClient + private function getClient(string $host, int $port, bool $ssl): CoClient|\Swoole\Http\Client { $key = "{$host}:{$port}:" . ($ssl ? '1' : '0'); if (!isset($this->clients[$key])) { if ($this->coroutines) { $this->clients[$key] = new CoClient($host, $port, $ssl); } else { $syncClientClass = self::getSyncClientClass(); if (!class_exists($syncClientClass)) { throw new Exception('Swoole sync HTTP client is not available'); } - /** `@var` CoClient $client */ $client = new $syncClientClass($host, $port, $ssl); $this->clients[$key] = $client; } } return $this->clients[$key]; }
180-221: Type hint should accept both client types.The
$clientparameter is typed asCoClientbut may receive a syncSwoole\Http\Clientinstance. Additionally, the header check on line 213 is case-sensitive, which may missContent-Typeor other casing variants.Suggested fix
- private function configureBody(CoClient $client, mixed $body, array $headers): void + private function configureBody(CoClient|\Swoole\Http\Client $client, mixed $body, array $headers): void { if ($body === null) { return; } + + $normalizedHeaders = array_change_key_case($headers, CASE_LOWER); if (is_array($body)) { $hasFiles = false; $formData = []; // ... file handling ... if ($hasFiles) { foreach ($formData as $key => $value) { $client->addData($value, $key); } - } elseif (isset($headers['content-type']) && $headers['content-type'] === 'application/x-www-form-urlencoded') { + } elseif (($normalizedHeaders['content-type'] ?? '') === 'application/x-www-form-urlencoded') { $client->setData(http_build_query($body)); } else { $client->setData($body); }
345-356: Security: sensitive headers are not stripped on cross-origin redirects.When a redirect changes the host, port, or SSL setting, the code reuses
$allHeaderswhich may containAuthorization,Cookie, orProxy-Authorizationheaders. These credentials could be leaked to a different origin.Proposed fix - strip sensitive headers on cross-origin redirects
if ($newHost !== $host || $newPort !== $port || $newSsl !== $ssl) { $host = $newHost; $port = $newPort; $ssl = $newSsl; + // Strip sensitive headers on cross-origin redirects + $sensitiveHeaders = ['authorization', 'proxy-authorization', 'cookie', 'host']; + $allHeaders = array_filter( + $allHeaders, + fn($key) => !in_array(strtolower($key), $sensitiveHeaders, true), + ARRAY_FILTER_USE_KEY + ); $client = $this->getClient($host, $port, $ssl); $client->set($this->buildClientSettings($timeout, $connectTimeout)); $client->setMethod($method);
133-152: Concurrent coroutines may share the same client unsafely.The
getClient()method caches and returns the sameCoClientinstance per host/port/ssl. When multiple coroutines callsend()concurrently, they will share this client instance, which violates Swoole's concurrency model where clients should not be reused across concurrent coroutines. This can cause request interleaving and failures.Consider either creating a new client per request or implementing a connection pool with proper checkout/return semantics.
🧹 Nitpick comments (2)
src/Adapter/Swoole.php (2)
23-26: PHPDoc type annotation is inaccurate.The
$clientsarray can store bothCoClient(coroutine) andSwoole\Http\Client(sync) instances depending on the$coroutinesflag, but the PHPDoc only declaresCoClient. This causes static analysis tools to report type mismatches.Suggested fix
/** - * `@var` array<string, CoClient> + * `@var` array<string, CoClient|\Swoole\Http\Client> */ private array $clients = [];
420-422: Exception re-throw loses original stack trace.Wrapping the caught exception in a new
Exceptiondiscards the original stack trace, which can make debugging more difficult. Consider passing the original exception as thepreviousparameter.Suggested improvement
if ($exception !== null) { - throw new Exception($exception->getMessage()); + throw new Exception($exception->getMessage(), 0, $exception); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
phpstan.neonsrc/Adapter/Swoole.phpsrc/Client.phpsrc/Response.phptests/Adapter/CurlTest.phptests/Adapter/SwooleTest.phptests/ClientTest.phptests/router.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Adapter/SwooleTest.php
- tests/Adapter/CurlTest.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Response.php (1)
src/Exception.php (1)
Exception(7-21)
src/Adapter/Swoole.php (4)
src/Chunk.php (2)
Chunk(12-67)__construct(20-26)src/Exception.php (2)
Exception(7-21)__construct(13-16)src/Response.php (3)
Response(12-129)__construct(40-48)getStatusCode(72-75)src/Adapter.php (1)
send(26-33)
🔇 Additional comments (23)
tests/router.php (3)
26-27: Defensive JSON decode for state loading is solid.This avoids propagating null on invalid JSON.
45-47: Redirect host normalization is consistent with test URLs.
173-173: Explicit JSON Content-Type for the default response is good.src/Client.php (4)
3-53: Adapter injection + default Curl wiring look good.Please confirm the new 5s default connect timeout is acceptable for existing consumers.
171-173: JSON encode flag aggregation looks correct.
295-301: GraphQL body guardrail is a good safety check.
309-331: Adapter send delegation is clear and consistent.src/Response.php (3)
83-95:text()now handles null/scalar bodies safely.
105-111:json()error handling is clear and explicit.
122-127:blob()byte-wise conversion aligns with updated expectations.tests/ClientTest.php (7)
6-46: Adapter coverage tests are a nice addition.Please verify CI runs both with and without the Swoole extension so the skip guard is exercised.
85-127: Stricter response-shape assertions improve signal.
138-181: File upload assertions are clearer and more robust.
194-247: Host normalization to127.0.0.1:8000looks consistent.
327-389: Data provider host updates are consistent.
433-487: Retry tests now align with the updated host.
494-626: Chunk handling tests and JSON guard look good.phpstan.neon (1)
2-9: Configuration correctly uses dev dependency.swoole/ide-helper is properly configured as a require-dev dependency and phpstan.neon correctly references its paths. PHPStan analysis will work as expected when dev dependencies are installed.
src/Adapter/Swoole.php (5)
56-103: LGTM!The constructor correctly handles the fallback when
CoClientis unavailable, and the configuration options are well-documented and properly initialized.
105-123: LGTM!The
isAvailable()check covers both client types, and the string concatenation ingetSyncClientClass()is a reasonable approach to avoid autoload issues.
162-170: LGTM!The
closeClientmethod correctly closes the connection and removes the client from the cache.
230-236: LGTM!The method cleanly merges the base configuration with request-specific timeout settings.
434-440: LGTM!The destructor properly cleans up all cached client connections.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if ($query) { | ||
| $url = rtrim($url, '?') . '?' . http_build_query($query); | ||
| $separator = str_contains($url, '?') ? '&' : '?'; | ||
| $url = rtrim($url, '?&') . $separator . http_build_query($query); |
There was a problem hiding this comment.
Avoid invalid URL when input ends with ?.
Because the separator is chosen before trimming, a URL like http://example.com? can become http://example.com&foo=bar. Consider trimming first.
🔧 Suggested fix
- $separator = str_contains($url, '?') ? '&' : '?';
- $url = rtrim($url, '?&') . $separator . http_build_query($query);
+ $trimmedUrl = rtrim($url, '?&');
+ $separator = str_contains($trimmedUrl, '?') ? '&' : '?';
+ $url = $trimmedUrl . $separator . http_build_query($query);📝 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.
| if ($query) { | |
| $url = rtrim($url, '?') . '?' . http_build_query($query); | |
| $separator = str_contains($url, '?') ? '&' : '?'; | |
| $url = rtrim($url, '?&') . $separator . http_build_query($query); | |
| if ($query) { | |
| $trimmedUrl = rtrim($url, '?&'); | |
| $separator = str_contains($trimmedUrl, '?') ? '&' : '?'; | |
| $url = $trimmedUrl . $separator . http_build_query($query); |
🤖 Prompt for AI Agents
In `@src/Client.php` around lines 304 - 306, The code chooses $separator before
trimming $url which yields invalid URLs like http://example.com&foo=bar when
$url ends with '?'; fix this in the block that builds the query by first
trimming trailing '?' and '&' from $url (use rtrim($url, '?&') and assign back
to $url or a new $baseUrl), then determine $separator with str_contains($url,
'?') ? '&' : '?', and finally set $url = $url . $separator .
http_build_query($query); reference the $url and $query variables in Client.php
to locate and update this logic.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.