Fix: array-like compatibility #35
Conversation
WalkthroughAdds Composer allow-plugins entries, relaxes several method parameter types to nullable/mixed, enhances Swoole Response to accept multi-value headers and nullable body, extends E2E client to parse and return cookies, adds a /set-cookie test route and an E2E test for Set-Cookie handling, updates MIME expectations in a unit test, and replaces the final Docker image/staging that removed Swoole build steps. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant E2E_Client as "E2E Client"
participant Test_Server as "Test Server"
participant App_Route as "App Route (/set-cookie)"
participant Swoole_Response as "Swoole Response"
rect rgb(242,255,242)
note right of Tester: /set-cookie flow (new)
Tester->>E2E_Client: call("/set-cookie")
E2E_Client->>Test_Server: HTTP GET /set-cookie
Test_Server->>App_Route: dispatch handler
App_Route->>Swoole_Response: sendHeader("Set-Cookie", "key1=value1")
App_Route->>Swoole_Response: sendHeader("Set-Cookie", "key2=value2")
App_Route->>Swoole_Response: end("OK")
Test_Server-->>E2E_Client: 200 OK + Set-Cookie headers
E2E_Client->>E2E_Client: parseCookie() for each Set-Cookie
E2E_Client-->>Tester: return { body, headers, cookies }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the ✨ Finishing Touches
🧪 Generate unit tests
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Swoole/Response.php (1)
188-191: Fix PHP 8.4 deprecation: make parameter explicitly nullable.The default null with string type is deprecated in 8.4. Update signature.
-protected function end(string $content = null): void +protected function end(?string $content = null): void
🧹 Nitpick comments (5)
composer.json (1)
26-32: Allow-plugins looks fine; consider prefer-stable and re-verify plugin necessity.To avoid pulling dev builds for a library, add prefer-stable and only keep minimum-stability=dev if strictly required by dependencies.
Apply:
"minimum-stability": "dev", + "prefer-stable": true, "config": { "allow-plugins": { "php-http/discovery": true, "tbachert/spi": true } }Optionally confirm these are actual Composer plugins enabled at install time.
#!/bin/bash # Verify both packages register a composer-plugin composer show -a php-http/discovery | rg -n 'type|plugin' composer show -a tbachert/spi | rg -n 'type|plugin'tests/e2e/Client.php (2)
72-85: Cookie capture works; consider accumulating duplicate header values generally.Current logic overwrites $responseHeaders['set-cookie'] on repeats; you already store cookies separately, which is fine. If you want parity for other multi-valued headers, store arrays on duplicate keys.
- $responseHeaders[strtolower(trim($header[0]))] = trim($header[1]); + $key = strtolower(trim($header[0])); + $val = trim($header[1]); + if (isset($responseHeaders[$key])) { + $responseHeaders[$key] = (array) $responseHeaders[$key]; + $responseHeaders[$key][] = $val; + } else { + $responseHeaders[$key] = $val; + }
113-126: Cookie parser is pragmatic; minor hardening suggested.parse_str treats flags (e.g., HttpOnly) as empty strings, which is fine since you only take the first name/value. Optionally trim surrounding quotes from the primary value.
- parse_str(strtr($cookie, ['&' => '%26', '+' => '%2B', ';' => '&']), $cookies); - return $cookies; + parse_str(strtr($cookie, ['&' => '%26', '+' => '%2B', ';' => '&']), $cookies); + $name = array_key_first($cookies); + if ($name !== null && isset($cookies[$name])) { + $cookies[$name] = trim($cookies[$name], "\"'"); + } + return $cookies;tests/e2e/Swoole/ResponseTest.php (2)
90-122: Solid coverage of Cookie header variants; tiny typo.Rename “Two cookiees” to “Two cookies” for clarity.
- // Two cookiees + // Two cookies
123-129: LGTM: validates multiple Set-Cookie handling via parsed cookies.As an enhancement, you might also assert the raw Set-Cookie headers count if you decide to accumulate headers in the client.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
composer.json(1 hunks)src/Swoole/Response.php(1 hunks)tests/e2e/Client.php(2 hunks)tests/e2e/Swoole/ResponseTest.php(1 hunks)tests/e2e/server.php(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CodeQL
src/Swoole/Response.php
[error] 188-188: PHPStan: Deprecated in PHP 8.4: Parameter #1 $content (string) is implicitly nullable via default value null.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit & E2E
🔇 Additional comments (4)
tests/e2e/Client.php (2)
63-64: LGTM: initialize cookie collection for response parsing.
109-110: Breaking change in return shape; confirm all callers handle 'cookies'.Downstream assertions seem updated; ensure any helpers consuming Client::call() out of E2E also handle the new key.
tests/e2e/server.php (2)
70-76: LGTM: echo cookie header endpoint is simple and effective for tests.
77-85: LGTM: multi Set-Cookie route aligns with client parsing.This validates multiple Set-Cookie emissions; relies on Response::sendHeader handling arrays correctly (see Response.php comment).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Swoole/Response.php (1)
230-233: Bug: arrays passed to SwooleResponse->header() won’t emit multiple headers. Iterate values.Without iterating, Set-Cookie array values won’t be sent as separate headers, breaking cookie handling/tests.
- /** - * @param string|array<string> $values - */ - protected function sendHeader(string $key, mixed $values): void - { - $this->swoole->header($key, $values); - } + /** + * @param string|array<int,string> $values + */ + protected function sendHeader(string $key, mixed $values): void + { + if (\is_array($values)) { + foreach ($values as $value) { + $this->swoole->header($key, (string) $value); + } + return; + } + $this->swoole->header($key, (string) $values); + }
🧹 Nitpick comments (3)
tests/e2e/Swoole/ResponseTest.php (1)
90-96: Test depends on multi-value header emission (Set-Cookie).This will only pass if Response::sendHeader() iterates arrays to emit multiple header lines. See my comment in src/Swoole/Response.php. Consider also asserting the cookie count to harden the test.
$response = $this->client->call(Client::METHOD_GET, '/set-cookie'); $this->assertEquals(200, $response['headers']['status-code']); + $this->assertCount(2, $response['cookies']); $this->assertEquals('value1', $response['cookies']['key1']); $this->assertEquals('value2', $response['cookies']['key2']);src/Swoole/Files.php (1)
86-86: Guard directory handle and stabilize MIME detection (optional).
- Add an opendir() failure check to avoid warnings.
- Consider mapping woff/woff2 in EXTENSIONS to avoid environment-dependent mime_content_type() results.
Example (outside this hunk):
// EXTENSIONS 'woff' => 'font/woff', 'woff2' => 'font/woff2',$handle = opendir((string) $directory); if ($handle === false) { throw new Exception("Failed to open directory: {$directory}"); }src/Swoole/Response.php (1)
218-221: Type: pass int to status(), not string (minor).Swoole expects int code; drop the string cast to avoid relying on implicit conversion.
- $this->swoole->status((string) $statusCode, $this->getStatusCodeReason($statusCode)); + $this->swoole->status($statusCode, $this->getStatusCodeReason($statusCode));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Dockerfile(1 hunks)src/Swoole/Files.php(1 hunks)src/Swoole/Request.php(1 hunks)src/Swoole/Response.php(2 hunks)tests/Swoole/FilesTest.php(1 hunks)tests/e2e/Swoole/ResponseTest.php(1 hunks)tests/e2e/server.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/server.php
🔇 Additional comments (5)
src/Swoole/Request.php (1)
46-46: Signature loosening to nullable default looks good.Contravariant param type is fine vs parent; keeps behavior unchanged.
Please verify Utopia\Request::getServer() signature is compatible in the version targeted by utopia-php/http 0.33.25.
src/Swoole/Files.php (1)
86-86: Nullable $root: LGTM.No functional change; improves API clarity.
tests/Swoole/FilesTest.php (1)
52-53: Updated MIME expectations: OK.Matches common MIME registrations for WOFF/WOFF2.
src/Swoole/Response.php (1)
188-191: Accepting nullable content in end(): OK.Matches Swoole’s ability to end without a body.
Dockerfile (1)
11-11: Verify base image has Swoole and Composer plugin choice.
- Confirm appwrite/base:0.10.4 ships the Swoole extension required to run the server.
- You install with --no-plugins while composer.json enables allow-plugins; ensure no plugin-required steps are skipped.
Would you like me to open a follow-up PR if verification fails?
Support for https://github.com/utopia-php/http/releases/tag/0.33.25
Summary by CodeRabbit
New Features
Tests
Chores