Normalize array-valued Swoole request headers#235
Conversation
Greptile SummaryThis PR fixes a production Confidence Score: 4/5Safe to merge for the primary Swoole header normalization fix, but the The core TypeError fix is correct and well-targeted. However, the new
Important Files Changed
|
| for ($i = count($value) - 1; $i >= 0; $i--) { | ||
| $candidate = $value[$i]; | ||
|
|
||
| if (is_scalar($candidate) || $candidate instanceof Stringable) { | ||
| return (string) $candidate; | ||
| } |
There was a problem hiding this comment.
Numeric-index traversal assumes sequential array
The loop accesses $value[$i] where $i counts down from count($value)-1. If Swoole ever returns an associative (non-sequential) array for a multi-valued header, these accesses will produce Undefined array key warnings in PHP 8 and return null, causing the loop to silently fall through to $default. Using array_values() to re-index first makes this robust:
| for ($i = count($value) - 1; $i >= 0; $i--) { | |
| $candidate = $value[$i]; | |
| if (is_scalar($candidate) || $candidate instanceof Stringable) { | |
| return (string) $candidate; | |
| } | |
| foreach (array_reverse(array_values($value)) as $candidate) { | |
| if (is_scalar($candidate) || $candidate instanceof Stringable) { | |
| return (string) $candidate; | |
| } | |
| } |
| public function testCanGetScalarHeaders(): void | ||
| { | ||
| $this->request?->getSwooleRequest()->header = [ | ||
| 'x-replaced-path' => '/gateway', | ||
| ]; | ||
|
|
||
| $this->assertEquals('/gateway', $this->request?->getHeader('x-replaced-path')); | ||
| } | ||
|
|
||
| public function testCanNormalizeArrayHeaders(): void | ||
| { | ||
| $this->request?->getSwooleRequest()->header = [ | ||
| 'x-replaced-path' => ['/client', '/gateway'], | ||
| ]; | ||
|
|
||
| $this->assertEquals('/gateway', $this->request?->getHeader('x-replaced-path')); | ||
| } |
There was a problem hiding this comment.
Missing test cases for default value and case-insensitive lookup
The new tests cover scalar and array values, but two additional cases are worth adding:
- A missing header should return the provided
$default(not''). getHeadershould be case-insensitive on the key (getHeader('X-Replaced-Path')vsgetHeader('x-replaced-path')).
Without the case-insensitive test, the new strtolower call in getHeader has no coverage.
Summary
Request::getHeader()getHeader()aligned with its declaredstringreturn type even when the underlying Swoole request stores repeated header values as arraysProblem
This was hit in production by Appwrite Edge while handling gateway-rewritten requests:
The failure mode is:
TypeError: Utopia\Http\Adapter\Swoole\Request::getHeader(): Return value must be of type string, array returnedThe concrete trigger was Traefik's
replacePathmiddleware. It stores the original path inX-Replaced-PathviaHeader.Add(...). If an inbound request already contains the same header, Swoole exposes the combined value as an array.getHeader()currently returns that raw array even though its contract isstring.Solution
Returning the last value matches append-style middleware behavior, including the Traefik
replacePathcase where the middleware-added value is appended after any client-supplied value.Why 0.34.x
This issue is currently affecting a consumer pinned to
0.34.*, so the fix is proposed against0.34.xfirst to make it releasable for that dependency line.Testing
vendor/bin/pint src/Http/Adapter/Swoole/Request.php tests/SwooleRequestTest.phpvendor/bin/phpunit tests/RequestTest.php tests/SwooleRequestTest.php