Conversation
Greptile SummaryThis PR reverts Swoole cookie handling on Key points:
Confidence Score: 5/5Safe to merge — all remaining findings are P2 style/test-coverage suggestions that do not block correctness. The functional change (removing
|
| Filename | Overview |
|---|---|
| src/Http/Adapter/Swoole/Request.php | Reverts getCookie() to read from $swooleRequest->cookie (Swoole's native parsed map) with strtolower() key normalization; creates a case-sensitivity behavioral difference with the FPM adapter. |
| src/Http/Adapter/Swoole/Server.php | Removes the forced http_parse_cookie => false setting, restoring Swoole's default native cookie-parsing behavior. Change is clean. |
| src/Http/Adapter/SwooleCoroutine/Server.php | Same as Swoole/Server.php — removes forced http_parse_cookie => false. Clean removal. |
| tests/e2e/BaseTest.php | Cookie test removed from the shared trait, since FPM and Swoole now have distinct cookie-access semantics. Makes sense. |
| tests/e2e/ResponseFPMTest.php | Adds FPM-specific testCookie() covering raw Cookie header echo with good edge-case coverage (equals in values, no space after semicolon, case-distinct names). |
| tests/e2e/ResponseSwooleTest.php | Adds Swoole-specific testCookie() covering individual getCookie() lookups, but lacks edge-case tests for cookies with = in values, no-space semicolons, and duplicate names with different casing. |
| tests/e2e/init.php | Adds /cookie/:key endpoint for per-cookie lookup alongside the existing /cookies raw-header endpoint. Correct split for adapter-specific tests. |
Reviews (1): Last reviewed commit: "Remove Swoole adapter unit test" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| public function getCookie(string $key, string $default = ''): string | ||
| { | ||
| $key = \strtolower($key); | ||
|
|
||
| $cookies = \explode(';', $this->getHeader('cookie', '')); | ||
| foreach ($cookies as $cookie) { | ||
| $cookie = \trim($cookie); | ||
| if ($cookie === '') { | ||
| continue; | ||
| } | ||
| $parts = \explode('=', $cookie, 2); | ||
| $cookieKey = \trim($parts[0]); | ||
| $cookieValue = isset($parts[1]) ? \trim($parts[1]) : ''; | ||
| if ($cookieKey === $key) { | ||
| return $cookieValue; | ||
| } | ||
| } | ||
| $key = strtolower($key); | ||
|
|
||
| return $default; | ||
| return $this->swoole->cookie[$key] ?? $default; | ||
| } |
There was a problem hiding this comment.
getCookie() key normalisation differs from FPM adapter
Swoole's native cookie parser stores cookie names in lowercase (all HTTP header keys are lowercased by Swoole), so the strtolower($key) here correctly normalises lookup. However, FPM\Request::getCookie() performs no such normalisation — it looks up $_COOKIE[$key] with the key exactly as passed.
This means getCookie('MyToken') returns the cookie on Swoole (lowercased lookup matches mytoken) but will miss it on FPM when the cookie was sent as mytoken. The reverse is also true: FPM is case-sensitive while Swoole is case-insensitive. Any application that calls getCookie() with a mixed-case cookie name will behave differently depending on the adapter in use.
This is a pre-existing 0.33.x characteristic being restored, but it may be worth documenting the difference or adding a note to getCookie()'s docblock so callers know to use lowercase keys for portability across adapters.
| { | ||
| $headers = ['Cookie: cookie1=value1; cookie2=value2']; | ||
|
|
||
| $response = $this->client->call(Client::METHOD_GET, '/cookie/cookie1', $headers); | ||
| $this->assertEquals(200, $response['headers']['status-code']); | ||
| $this->assertEquals('value1', $response['body']); | ||
|
|
||
| $response = $this->client->call(Client::METHOD_GET, '/cookie/cookie2', $headers); | ||
| $this->assertEquals(200, $response['headers']['status-code']); | ||
| $this->assertEquals('value2', $response['body']); | ||
| } |
There was a problem hiding this comment.
ResponseFPMTest::testCookie() tests several edge cases: cookies with = in the value (cookie1=value1=value2), cookies with no space after the semicolon separator (cookie1=value1;cookie2=value2), and two cookies that differ only in casing (cookie1=v1; Cookie1=v2). None of these are exercised in the Swoole equivalent.
Since the whole point of this PR is to restore Swoole native cookie parsing, it would be valuable to confirm Swoole handles at least the value=with=equals case correctly (i.e. getCookie('cookie1') returns value1=value2), because different cookie parsers handle trailing = splits differently.
Consider adding a test case such as:
$response = $this->client->call(
Client::METHOD_GET,
'/cookie/cookie1',
['Cookie: cookie1=value1=value2']
);
$this->assertEquals('value1=value2', $response['body']);2 participants
Summary
This reverts Swoole cookie handling on
0.34.xback to the0.33.xbehavior.What Changed
http_parse_cookie => falsesetting from both Swoole server adaptersUtopia\Http\Adapter\Swoole\Request::getCookie()back to Swoole's parsed cookie mapcookieheader from Swoole request stateCookieheader echoing, while Swoole assertsRequest::getCookie()behaviorWhy
0.34.xchanged Swoole to preserve the rawCookieheader and parse cookies manually. That differs from0.33.x, where Swoole parsed cookies natively andgetCookie()read from$swooleRequest->cookie.This PR restores the older compatibility model for servers upgrading from
0.33.xto0.34.x.Impact
Cookieheader preservation in Swoole will no longer see the0.34.xbehaviorValidation
vendor/bin/phpunit tests/RequestTest.php tests/ResponseTest.phpphp -l src/Http/Adapter/Swoole/Request.phpphp -l src/Http/Adapter/Swoole/Server.phpphp -l src/Http/Adapter/SwooleCoroutine/Server.phpphp -l tests/e2e/BaseTest.phpphp -l tests/e2e/ResponseFPMTest.phpphp -l tests/e2e/ResponseSwooleTest.phpphp -l tests/e2e/init.php