Feat sync with 33 34 #189
Feat sync with 33 34 #189
Conversation
Add 451 error code
Feat default param callable
Adding the Tests namespace causes conflicts downstream.
fix: remove tests from autoload
chore: fix default param
Added AnyOf Validator
Feat: AnyOf validators export
WalkthroughAdds telemetry and optional response compression, expands header and status-code handling, scopes route parameters by matched path, moves header generation to adapters, raises PHP requirement to >=8.1, updates CI/Docker images, and adds cookie endpoints/tests and domain restriction validator. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant App as Http::run
participant Router as Router
participant Handler as Route Handler
participant Resp as Response
participant Telemetry as Telemetry
rect rgb(245,250,255)
App->>Telemetry: activeRequests.add(+1)
App->>Telemetry: start requestDuration
end
Client->>App: HTTP request
App->>Router: match(request)
Router-->>App: route + matchedPath
App->>Handler: invoke(route handler)
Handler->>Resp: write headers/body
alt compression enabled & isCompressible
Resp->>Resp: compress body
Resp->>Client: headers (Content-Encoding, Vary), body (compressed)
else
Resp->>Client: headers, body
end
rect rgb(245,250,255)
App->>Telemetry: record sizes + route attr
App->>Telemetry: stop requestDuration
App->>Telemetry: activeRequests.add(-1)
end
sequenceDiagram
autonumber
participant SW as Swoole\HTTPRequest
participant Adapter as Swoole Request Adapter
participant App as Application
SW->>Adapter: raw headers (Cookie)
Adapter->>Adapter: generateHeaders() -> lowercase keys & reconstruct Cookie header
Adapter->>Adapter: getCookie(key) -> exact key or lowercase fallback
Adapter-->>App: cookie value (or default)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/Http/Http.php (4)
🔇 Additional comments (4)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Dockerfile.fpm (1)
16-24: Mismatch between base image andPHP_VERSIONbreaks the build.Base image is
php:8.1-cli-alpine, yet we setPHP_VERSION=82, soapk add php$PHP_VERSIONbecomesapk add php82 …. Those packages don’t exist in the Alpine repos shipped with the 8.1 image, causing the Docker build to fail. Please align the variable with the base image:-FROM php:8.1-cli-alpine as final -ENV DEBIAN_FRONTEND=noninteractive \ - PHP_VERSION=82 +FROM php:8.1-cli-alpine as final +ENV DEBIAN_FRONTEND=noninteractive \ + PHP_VERSION=81src/Http/Response.php (1)
640-669: Drop the stale pre-compression send branch.This block still calls
array_key_exists(..., $this->compressed)even though$this->compressedno longer exists, which now throws aTypeError. It also sends the body (and mutates$this->size) before the new compression logic can run, leading to double writes and incorrect accounting. Remove this leftover branch sosend()flows through the new compression-aware path only.Apply this diff:
- if (!$this->disablePayload) { - $length = strlen($body); - - $headersSize = 0; - foreach ($this->headers as $name => $values) { - if (\is_array($values)) { - foreach ($values as $value) { - $headersSize += \strlen($name . ': ' . $value); - } - $headersSize += (\count($values) - 1) * 2; // linebreaks - } else { - $headersSize += \strlen($name . ': ' . $values); - } - } - $headersSize += (\count($this->headers) - 1) * 2; // linebreaks - $this->size = $this->size + $headersSize + $length; - - if (array_key_exists( - $this->contentType, - $this->compressed - ) && ($length <= self::CHUNK_SIZE)) { // Dont compress with GZIP / Brotli if header is not listed and size is bigger than 2mb - $this->end($body); - } else { - for ($i = 0; $i < ceil($length / self::CHUNK_SIZE); $i++) { - $this->write(substr($body, ($i * self::CHUNK_SIZE), min(self::CHUNK_SIZE, $length - ($i * self::CHUNK_SIZE)))); - } - - $this->end(); - } - }
📜 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 (23)
.github/workflows/bench.yml(1 hunks).github/workflows/codeql-analysis.yml(1 hunks)Dockerfile.fpm(1 hunks)README.md(1 hunks)composer.json(1 hunks)phpunit.xml(2 hunks)src/Http/Adapter/FPM/Response.php(2 hunks)src/Http/Adapter/Swoole/Request.php(2 hunks)src/Http/Adapter/Swoole/Response.php(2 hunks)src/Http/Adapter/Swoole/Server.php(2 hunks)src/Http/Http.php(7 hunks)src/Http/Response.php(18 hunks)src/Http/Route.php(4 hunks)src/Http/Router.php(6 hunks)src/Http/Validator/Domain.php(2 hunks)tests/HttpTest.php(1 hunks)tests/Validator/DomainTest.php(2 hunks)tests/Validator/HostTest.php(1 hunks)tests/Validator/IPTest.php(1 hunks)tests/Validator/URLTest.php(1 hunks)tests/e2e/BaseTest.php(1 hunks)tests/e2e/Client.php(3 hunks)tests/e2e/init.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
tests/HttpTest.php (4)
src/Http/Route.php (1)
Route(5-185)src/Http/Http.php (1)
execute(473-476)src/Http/Adapter/FPM/Request.php (1)
Request(7-401)src/Http/Request.php (1)
Request(5-644)
src/Http/Adapter/Swoole/Response.php (3)
src/Http/Adapter/FPM/Response.php (2)
end(31-36)sendHeader(60-69)src/Http/Response.php (2)
end(747-747)sendHeader(828-828)tests/MockRequest.php (1)
sendHeader(62-64)
src/Http/Adapter/Swoole/Server.php (4)
src/Http/Http.php (2)
run(559-588)start(384-448)src/Http/Adapter/SwooleCoroutine/Server.php (1)
start(34-39)src/Http/Adapter/FPM/Server.php (1)
start(26-29)src/Http/Adapter.php (1)
start(9-9)
src/Http/Validator/Domain.php (4)
src/Http/Validator/AllOf.php (1)
__construct(19-21)src/Http/Validator/ArrayList.php (1)
__construct(32-36)src/Http/Validator/Host.php (1)
__construct(21-24)src/Http/Validator/Multiple.php (1)
__construct(33-40)
src/Http/Route.php (3)
src/Http/Adapter/Swoole/Request.php (2)
Request(8-403)getURI(162-165)src/Http/Adapter/FPM/Request.php (2)
Request(7-401)getURI(148-151)src/Http/Request.php (2)
Request(5-644)getURI(219-219)
src/Http/Adapter/FPM/Response.php (3)
src/Http/Adapter/Swoole/Response.php (2)
end(43-46)sendHeader(67-70)src/Http/Response.php (2)
end(747-747)sendHeader(828-828)tests/MockRequest.php (1)
sendHeader(62-64)
src/Http/Router.php (2)
src/Http/Route.php (5)
setPathParam(155-158)path(85-90)alias(98-103)getMethod(123-126)setMatchedPath(58-62)src/Http/Http.php (1)
match(458-470)
tests/e2e/init.php (3)
src/Http/Adapter/Swoole/Request.php (2)
Request(8-403)addHeader(320-325)src/Http/Request.php (3)
Request(5-644)getHeaders(329-332)addHeader(343-343)src/Http/Response.php (3)
Response(7-1002)getHeaders(552-555)addHeader(509-527)
src/Http/Adapter/Swoole/Request.php (2)
src/Http/Adapter/FPM/Request.php (1)
getHeader(283-288)src/Http/Request.php (1)
getHeader(320-320)
tests/e2e/BaseTest.php (1)
tests/e2e/Client.php (2)
call(55-139)Client(7-155)
tests/Validator/DomainTest.php (1)
src/Http/Validator/Domain.php (3)
Domain(14-136)createRestriction(25-32)isValid(63-111)
src/Http/Response.php (4)
src/Http/Http.php (2)
setCompressionMinSize(103-106)setCompressionSupported(111-114)src/Http/Request.php (1)
addHeader(343-343)src/Http/Adapter/FPM/Response.php (3)
end(31-36)write(17-21)sendHeader(60-69)src/Http/Adapter/Swoole/Response.php (3)
end(43-46)write(32-35)sendHeader(67-70)
src/Http/Http.php (5)
src/Http/Route.php (3)
Route(5-185)getMethod(123-126)getPath(133-136)src/Http/Response.php (5)
setCompressionMinSize(397-401)setCompressionSupported(408-412)getStatusCode(463-466)getSize(475-478)setAcceptEncoding(384-388)src/Http/Adapter/Swoole/Request.php (3)
getMethod(135-138)getProtocol(90-102)getHeader(308-311)src/Http/Adapter/FPM/Request.php (3)
getMethod(121-124)getProtocol(85-88)getHeader(283-288)src/Http/Request.php (4)
getMethod(200-200)getProtocol(173-173)getSize(362-365)getHeader(320-320)
🪛 GitHub Actions: Tests
src/Http/Response.php
[error] 51-51: Fatal error: Cannot redefine class constant Utopia\Http\Response::STATUS_CODE_PROCESSING in /usr/share/nginx/html/src/Http/Response.php on line 51.
🪛 PHPMD (2.15.0)
tests/e2e/Client.php
58-58: Avoid unused local variables such as '$url'. (undefined)
(UnusedLocalVariable)
96-96: Avoid unused parameters such as '$curl'. (undefined)
(UnusedFormalParameter)
src/Http/Response.php
673-673: Avoid unused local variables such as '$hasContentEncoding'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (1)
README.md (1)
278-278: Branding update reads wellThe PHP 8.1 requirement statement still makes sense with the updated framework name.
| if (empty($path)) { | ||
| $pathParams = $this->pathParams[$path] ?? \array_values($this->pathParams)[0] ?? []; | ||
| } else { | ||
| $pathParams = $this->pathParams[$path] ?? []; | ||
| } | ||
|
|
||
| foreach ($pathParams as $key => $index) { | ||
| if (array_key_exists($index, $parts)) { |
There was a problem hiding this comment.
Fix fallback when no path params exist
When $path is empty we hit \array_values($this->pathParams)[0] ?? []. If the route has no captured params, array_values([])[0] raises an “Undefined array key 0” warning. With PHPUnit configured to convert warnings to exceptions, this breaks request handling for routes without params. Guard the fallback before indexing, e.g.:
- $pathParams = $this->pathParams[$path] ?? \array_values($this->pathParams)[0] ?? [];
+ $pathParams = $this->pathParams[$path] ?? [];
+ if ($pathParams === []) {
+ $values = \array_values($this->pathParams);
+ $pathParams = $values[0] ?? [];
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Http/Route.php around lines 171 to 178, the fallback when $path is empty
indexes array_values($this->pathParams)[0] even when pathParams is empty which
triggers an "Undefined array key 0" warning; change the logic to check if
$this->pathParams is non-empty before accessing the first element (or use a safe
function like reset() with a presence check) and fall back to an empty array
when there are no captured params so no indexing occurs on an empty array.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/UtopiaFPMRequestTest.php (1)
73-76: Use strict comparison in null check.The comparison
self::$params != nulluses loose equality, which can have unexpected behavior with empty arrays. Use strict comparison for clearer intent.Apply this diff:
public static function _hasParams(): bool { - return self::$params != null; + return self::$params !== null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/codeql-analysis.yml(1 hunks)src/Http/Adapter/Swoole/Server.php(2 hunks)src/Http/Http.php(7 hunks)src/Http/Response.php(18 hunks)tests/HttpTest.php(2 hunks)tests/UtopiaFPMRequestTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/HttpTest.php (2)
tests/UtopiaFPMRequestTest.php (2)
UtopiaFPMRequestTest(7-77)_setParams(53-56)src/Http/Request.php (1)
Request(5-644)
src/Http/Http.php (3)
src/Http/Route.php (3)
Route(5-185)getMethod(123-126)getPath(133-136)src/Http/Response.php (5)
setCompressionMinSize(377-381)setCompressionSupported(388-392)getStatusCode(443-446)getSize(455-458)setAcceptEncoding(364-368)src/Http/Request.php (4)
getMethod(200-200)getProtocol(173-173)getSize(362-365)getHeader(320-320)
tests/UtopiaFPMRequestTest.php (2)
src/Http/Http.php (1)
Http(13-718)src/Http/Adapter.php (1)
Adapter(5-10)
src/Http/Adapter/Swoole/Server.php (5)
src/Http/Http.php (3)
Http(13-718)run(534-563)start(359-423)src/Http/Adapter.php (4)
Adapter(5-10)onRequest(8-8)onStart(7-7)start(9-9)src/Http/Adapter/SwooleCoroutine/Server.php (4)
Server(8-40)onRequest(20-27)onStart(29-32)start(34-39)src/Http/Adapter/Swoole/Request.php (1)
Request(8-403)src/Http/Adapter/Swoole/Response.php (1)
Response(8-95)
src/Http/Response.php (3)
src/Http/Http.php (3)
Http(13-718)setCompressionMinSize(103-106)setCompressionSupported(111-114)src/Http/Adapter/FPM/Response.php (2)
end(31-36)sendHeader(60-69)src/Http/Adapter/Swoole/Response.php (2)
end(43-46)sendHeader(67-70)
🔇 Additional comments (10)
.github/workflows/codeql-analysis.yml (1)
15-16: composer:2.8 Docker image tag verified. CI job can proceed as expected.src/Http/Http.php (3)
122-138: LGTM! Telemetry integration follows OpenTelemetry conventions.The metric definitions correctly follow the OpenTelemetry semantic conventions for HTTP server metrics, including appropriate histogram bucket boundaries and proper attribute naming.
534-563: LGTM! Clean separation of concerns with telemetry wrapper.The refactoring cleanly separates telemetry tracking (in
run) from core request processing (inrunInternal), making the codebase more maintainable. The use of$this->matchedRoutefor thehttp.routeattribute ensures accurate route attribution in metrics.
581-585: LGTM! Compression configuration properly initialized.The compression settings are correctly passed from
HttptoResponsewhen compression is enabled, including accept-encoding header, minimum size, and supported algorithms.tests/HttpTest.php (2)
810-810: Past issue resolved: Container is now correctly passed.The previous critical issue where the string
'1'was passed instead of aContainerinstance has been fixed. All three calls now correctly pass$context.Also applies to: 829-829, 848-848
795-853: LGTM! Comprehensive test coverage for callable string safety.The test properly verifies that callable strings (function names like
'phpinfo'and'system') are treated as literal string values rather than being executed, which is critical for security. The three scenarios cover default parameters, request parameters, and closures effectively.src/Http/Response.php (4)
49-114: Past issue appears addressed: No duplicate status code constants visible.The duplicate status code constants mentioned in the previous review are not present in the current annotated code. If they were causing fatal errors, they appear to have been removed.
330-340: LGTM! Clean content-type parsing for compression check.The
isCompressiblemethod correctly strips charset parameters (e.g.,;charset=utf-8) before checking against the compressible MIME types array, ensuring proper compression decisions.
489-506: LGTM! Robust multi-value header handling.The
addHeadermethod with theoverrideparameter properly handles both single-value and multi-value headers. Whenoverrideis false, it correctly converts a single value to an array before appending additional values.
625-637: LGTM! Accurate header size calculation for metrics.The header size calculation correctly accounts for both single-value and array-value headers, including the proper count of line breaks for multi-value headers. This provides accurate response size metrics for telemetry.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Dockerfile.swoole (1)
44-44: Xdebug removal path likely wrong for newer PHP (leftover xdebug in non‑debug builds).Hard-coded ABI dir
no-debug-non-zts-20220829(PHP 8.2). Ifappwrite/base:0.11.3ships PHP 8.3 (20230831), xdebug.so won’t be removed. Remove by discovery instead of fixed path.Apply this to make removal robust:
-RUN if [ "$DEBUG" = "false" ]; then rm -f /usr/local/lib/php/extensions/no-debug-non-zts-20220829/xdebug.so; fi +RUN if [ "$DEBUG" = "false" ]; then find /usr/local/lib/php/extensions -type f -name xdebug.so -delete || true; fiDockerfile.swoole_coroutines (1)
44-44: Xdebug removal path likely wrong for newer PHP (leftover xdebug in non‑debug builds).Same hard-coded ABI dir issue. Make removal robust.
-RUN if [ "$DEBUG" = "false" ]; then rm -f /usr/local/lib/php/extensions/no-debug-non-zts-20220829/xdebug.so; fi +RUN if [ "$DEBUG" = "false" ]; then find /usr/local/lib/php/extensions -type f -name xdebug.so -delete || true; fi
🧹 Nitpick comments (10)
tests/e2e/ResponseFPMTest.php (1)
22-57: Consider aligning the case sensitivity test input with BaseTest.The test logic correctly validates FPM's cookie preservation behavior. However, the case sensitivity test (lines 52-56) sends input with a space (
'cookie1=v1; Cookie1=v2'), while the corresponding BaseTest case sends input without a space ('cookie1=v1;Cookie1=v2'). Using the same input would better demonstrate the behavioral difference: Swoole normalizes by adding a space, while FPM preserves the original format.Apply this diff to align the test with BaseTest:
- // Case sensitivity for cookie names - $cookie = 'cookie1=v1; Cookie1=v2'; + // Case sensitivity for cookie names (FPM preserves original format) + $cookie = 'cookie1=v1;Cookie1=v2'; $response = $this->client->call(Client::METHOD_GET, '/cookies', [ 'Cookie' => $cookie ]); $this->assertEquals(200, $response['headers']['status-code']); - $this->assertEquals($cookie, $response['body']); + $this->assertEquals('cookie1=v1;Cookie1=v2', $response['body']);src/Http/Request.php (1)
507-507: Consider documenting caching expectations for implementers.The
FPM/Request.phpadapter caches headers in$this->headers(leveraging the property defined at line 47), while theSwoole/Request.phpadapter regenerates headers on each call. For methods likegetSize()(line 364) that callgenerateHeaders()multiple times, the lack of caching in Swoole could cause redundant work.Consider documenting in the method's docblock whether subclasses should cache results for performance, or consider adding a final wrapper method in the base class that handles caching uniformly:
/** * Generate headers * * Parse request headers as an array for easy querying using the getHeader method. * Subclasses should implement this method and may cache results in $this->headers * for performance if header generation is expensive. * * @return array */ abstract protected function generateHeaders(): array;docker-compose.yml (1)
11-11: Make phpunit.xml bind read‑only to avoid accidental in‑container edits.Mount config files as :ro.
Apply:
- - ./phpunit.xml:/usr/share/nginx/html/phpunit.xml + - ./phpunit.xml:/usr/share/nginx/html/phpunit.xml:roDockerfile.swoole (3)
27-31: Shell test consistency and portability.Use '=' consistently in
[ ... ]for POSIX shells; some environments don’t accept '=='.- if [ "$DEBUG" == "true" ]; then \ + if [ "$DEBUG" = "true" ]; then \-RUN if [ "$DEBUG" == "true" ]; then cp /usr/src/code/dev/xdebug.ini /usr/local/etc/php/conf.d/xdebug.ini; fi +RUN if [ "$DEBUG" = "true" ]; then cp /usr/src/code/dev/xdebug.ini /usr/local/etc/php/conf.d/xdebug.ini; fiAlso applies to: 42-44
1-1: Image pinning for reproducibility.Consider pinning images to immutable digests (or full patch tags) to avoid surprise rebuild drift.
Example:
- FROM composer:2.8@sha256:...
- FROM appwrite/base:0.11.3@sha256:...
Also applies to: 17-17
13-15: Composer ergonomics in CI builds.Consider adding
--no-interaction --no-progressto reduce noise and flakiness in non-interactive environments.-RUN composer install --ignore-platform-reqs --optimize-autoloader \ +RUN composer install --ignore-platform-reqs --no-interaction --no-progress --optimize-autoloader \ --no-plugins --no-scripts --prefer-dist \ `if [ "$TESTING" != "true" ]; then echo "--no-dev"; fi`Dockerfile.swoole_coroutines (3)
27-31: Shell test consistency and portability.Use '=' consistently in
[ ... ].- if [ "$DEBUG" == "true" ]; then \ + if [ "$DEBUG" = "true" ]; then \-RUN if [ "$DEBUG" == "true" ]; then cp /usr/src/code/dev/xdebug.ini /usr/local/etc/php/conf.d/xdebug.ini; fi +RUN if [ "$DEBUG" = "true" ]; then cp /usr/src/code/dev/xdebug.ini /usr/local/etc/php/conf.d/xdebug.ini; fiAlso applies to: 42-44
1-1: Image pinning for reproducibility.Pin by digest (or full patch tags) to avoid rebuild drift.
Also applies to: 17-17
13-15: Composer ergonomics in CI builds.Add
--no-interaction --no-progressfor non-interactive builds..github/workflows/test.yml (1)
27-37: Use-Twithdocker compose execin CI and fix a typo.Without
-T, CI can fail with TTY errors; also fix "Corotuine" typo.- - name: Run unit Tests - run: docker compose exec swoole vendor/bin/phpunit --configuration phpunit.xml --testsuite=unit + - name: Run unit Tests + run: docker compose exec -T swoole vendor/bin/phpunit --configuration phpunit.xml --testsuite=unit - - name: Run FPM Tests - run: docker compose exec fpm vendor/bin/phpunit --configuration phpunit.xml --group=fpm + - name: Run FPM Tests + run: docker compose exec -T fpm vendor/bin/phpunit --configuration phpunit.xml --group=fpm - - name: Run Swoole Tests - run: docker compose exec swoole vendor/bin/phpunit --configuration phpunit.xml --group=swoole + - name: Run Swoole Tests + run: docker compose exec -T swoole vendor/bin/phpunit --configuration phpunit.xml --group=swoole - - name: Run Swoole Corotuine Tests - run: docker compose exec swoole-coroutine vendor/bin/phpunit --configuration phpunit.xml --group=swoole-coroutine + - name: Run Swoole Coroutine Tests + run: docker compose exec -T swoole-coroutine vendor/bin/phpunit --configuration phpunit.xml --group=swoole-coroutineAdditionally, consider replacing the fixed
sleep 10with a simple readiness probe (curl loop) to reduce flakiness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/test.yml(1 hunks)Dockerfile.fpm(2 hunks)Dockerfile.swoole(2 hunks)Dockerfile.swoole_coroutines(2 hunks)docker-compose.yml(3 hunks)phpunit.xml(1 hunks)src/Http/Adapter/Swoole/Request.php(2 hunks)src/Http/Adapter/Swoole/Server.php(0 hunks)src/Http/Request.php(1 hunks)tests/UtopiaFPMRequestTest.php(1 hunks)tests/e2e/BaseTest.php(1 hunks)tests/e2e/ResponseFPMTest.php(2 hunks)tests/e2e/ResponseSwooleCoroutineTest.php(1 hunks)tests/e2e/ResponseSwooleTest.php(1 hunks)tests/e2e/init.php(2 hunks)
💤 Files with no reviewable changes (1)
- src/Http/Adapter/Swoole/Server.php
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/UtopiaFPMRequestTest.php
- phpunit.xml
- Dockerfile.fpm
- src/Http/Adapter/Swoole/Request.php
- tests/e2e/init.php
- tests/e2e/BaseTest.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Http/Request.php (2)
src/Http/Adapter/Swoole/Request.php (1)
generateHeaders(393-414)src/Http/Adapter/FPM/Request.php (1)
generateHeaders(375-400)
tests/e2e/ResponseFPMTest.php (2)
tests/e2e/BaseTest.php (1)
testCookie(84-120)tests/e2e/Client.php (2)
call(55-139)Client(7-155)
🔇 Additional comments (3)
tests/e2e/ResponseSwooleTest.php (1)
8-11: LGTM!The PHPDoc group annotations are a good addition, enabling targeted test execution via
--groupflags.tests/e2e/ResponseSwooleCoroutineTest.php (1)
8-11: LGTM!The PHPDoc annotations follow the same pattern as other test classes and support organized test execution.
tests/e2e/ResponseFPMTest.php (1)
8-11: LGTM!The PHPDoc annotations are consistent with the other test classes.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Http/Http.php(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Http/Http.php (3)
src/Http/Route.php (3)
Route(5-185)getMethod(123-126)getPath(133-136)src/Http/Response.php (5)
setCompressionMinSize(377-381)setCompressionSupported(388-392)getStatusCode(443-446)getSize(455-458)setAcceptEncoding(364-368)src/Http/Request.php (4)
getMethod(200-200)getProtocol(173-173)getSize(362-365)getHeader(320-320)
🔇 Additional comments (4)
src/Http/Http.php (4)
8-11: LGTM: Telemetry and compression infrastructure.The telemetry imports, constant, properties, and constructor initialization are well-structured. Using
NoTelemetryas the default adapter is appropriate, and thematchedRouteproperty documentation clearly explains its purpose in preserving the originally matched route.Also applies to: 15-15, 49-59, 69-75, 89-89
95-117: LGTM: Compression setters implemented correctly.The compression setter methods now include explicit
staticreturn type declarations, addressing the concern from the previous review. The fluent interface pattern is properly implemented for method chaining.
125-141: LGTM: Telemetry metrics follow OpenTelemetry conventions.The metric definitions align with OpenTelemetry semantic conventions for HTTP servers. The histogram bucket boundaries for
request.durationare appropriate for typical HTTP response times (5ms to 10s).
584-588: LGTM: Compression configuration applied correctly.The compression setup properly passes the client's
Accept-Encodingheader and configuration parameters to the response object when compression is enabled.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Http/Http.php (2)
593-599: Comment contradicts TTL (2 years vs 45 days).The computed value is 2 years. Update the comment for clarity.
- $time = (60 * 60 * 24 * 365 * 2); // 45 days cache + $time = (60 * 60 * 24 * 365 * 2); // 2 years cache
604-607: Instance-level matchedRoute may be unnecessary now.You persist
$this->matchedRoutebut telemetry now pullsroutefrom the request-scoped context. If no other consumer reads$this->matchedRoute, consider removing it to avoid shared-state drift in coroutine servers. If it is still needed elsewhere, ignore.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Http/Http.php(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Http/Http.php (3)
src/Http/Route.php (3)
Route(5-185)getMethod(123-126)getPath(133-136)src/Http/Response.php (5)
setCompressionMinSize(377-381)setCompressionSupported(388-392)getStatusCode(443-446)getSize(455-458)setAcceptEncoding(364-368)src/Http/Request.php (4)
getMethod(200-200)getProtocol(173-173)getSize(362-365)getHeader(320-320)
🔇 Additional comments (4)
src/Http/Http.php (4)
586-590: Compression wiring looks good; confirm header key handling.LGTM on passing Accept-Encoding and compression settings to Response. Ensure Request::getHeader() is case-insensitive so 'accept-encoding' works across adapters; otherwise use 'Accept-Encoding'.
119-141: Telemetry instrument initialization LGTM.Metric names, units, and bucket boundaries align with HTTP semconv goals.
92-117: Compression setters now fluent with static return type — LGTM.
49-55: Compression options state — LGTM.Sane defaults and types.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Http/Http.php (2)
69-75: Consider removing or documenting the usage ofmatchedRoute.The property is set at line 606 but never read in the provided code. If it's intended for extension points or external usage, document that in the comment; otherwise, it may be dead code.
606-606: Consider removing this assignment if unused.This assignment to
$this->matchedRouteappears to be unused (see comment on lines 69-75). If it serves an extension point or is accessed via reflection, document that; otherwise, remove it as dead code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Http/Http.php(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Http/Http.php (4)
src/Http/Route.php (3)
Route(5-185)getMethod(123-126)getPath(133-136)src/Http/Response.php (5)
setCompressionMinSize(377-381)setCompressionSupported(388-392)getStatusCode(443-446)getSize(455-458)setAcceptEncoding(364-368)src/Http/Router.php (1)
match(124-175)src/Http/Request.php (4)
getMethod(200-200)getProtocol(173-173)getSize(362-365)getHeader(320-320)
🔇 Additional comments (8)
src/Http/Http.php (8)
8-11: LGTM: Telemetry imports.The telemetry adapter imports follow a clean adapter pattern with a no-op fallback (NoTelemetry), which is appropriate for optional observability.
15-15: LGTM: Compression threshold constant.1024 bytes is a sensible default; smaller payloads often don't benefit from compression due to overhead.
49-54: LGTM: Compression configuration properties.The properties are correctly typed and provide the necessary configuration surface for enabling and tuning response compression.
56-59: LGTM: Telemetry instrument properties.The instrument types (Histogram for durations/sizes, UpDownCounter for active requests) align with OpenTelemetry semantic conventions for HTTP metrics.
89-89: LGTM: Default no-op telemetry adapter.Initializing with
NoTelemetry()ensures the framework can operate without observability overhead when not explicitly configured.
92-117: LGTM: Compression setter methods.The return type declarations (
static) have been correctly added, addressing the previous review feedback. The fluent interface pattern is now consistent across all setters.
119-141: LGTM: Telemetry instrument initialization.The instrument setup correctly follows OpenTelemetry semantic conventions for HTTP server metrics:
- Duration buckets (5ms to 10s) cover typical HTTP latency ranges
- Instrument types match the conventions (Histogram for sizes/durations, UpDownCounter for active requests)
- Units follow the spec (seconds, bytes, request count)
586-590: LGTM: Compression configuration.The compression setup correctly delegates to the Response object before processing begins. The methods exist in Response.php and follow the appropriate pattern.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests