Conversation
Extract per-request state from the Http singleton into a Dispatcher and
immutable RouteMatch. Under Swoole the Http, Router, and Route objects
are shared across every concurrent coroutine; previously matched-route
state lived on those shared objects and could be clobbered by a peer
coroutine between match and execute.
- Http no longer holds \$this->route; the matched route flows through
the per-request DI container.
- Route is frozen after registration: setMatchedPath/getMatchedPath are
gone, and the wildcard route is no longer mutated with the request
path on each hit.
- Router::matchRoute() returns an immutable RouteMatch (final readonly)
instead of writing back onto the shared Route.
- RequestContext provides a per-request overlay for label overrides so
consumers that tagged the current route (e.g. \$route->label('router',
true)) have a safe migration target.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
k6 benchmark
|
Adds three test files (28 cases, 67 assertions) focused on the no-mutation guarantees that motivate the refactor. - DispatcherTest: sequential FPM dispatch verifies request resources (route/routeMatch/context) are populated, RequestContext is fresh per request, the wildcard Route is never mutated, parameterized routes don't bleed state between requests, and hook/error/OPTIONS/HEAD lifecycle flows. - RouteMatchTest: field access, readonly-class reflection check, and a runtime assertion that fields cannot be reassigned. - RouterMatchRouteTest: matchRoute return shape, exact/param/wildcard branches, explicit no-mutation snapshot across repeated matches, distinct RouteMatch instance per call, and the deprecated match() shim still returns a Route. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR extracts per-request mutable state out of the One breaking-change gap needs attention before merge:
Confidence Score: 4/5Safe to merge after addressing the Router::match() missing shim / matchRoute() visibility gap. The concurrency fix itself is correct and well-tested. The single P1 finding is a public-API breakage: Router::match() was removed without a shim and Router::matchRoute() is private despite being documented as the replacement, leaving callers with a bare method+path string with no public migration path. src/Http/Router.php — missing Router::match() deprecation shim and matchRoute() visibility. Important Files Changed
Reviews (6): Last reviewed commit: "Collapse Router surface and fold wildcar..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The per-request label-override overlay isn't needed. Route labels should be set at registration time; any per-request mutable state a consumer needs can go in their own DI-container resource. Keeping a dedicated class for this was speculative scope. - Delete src/Http/RequestContext.php and its test file. - Dispatcher no longer constructs a RequestContext or registers a `context` request resource. - Http::execute shim drops the same wiring. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The hook arrays (init/shutdown/options/error/start/request) lived as protected statics on Http, and the previous refactor added six public getInitHooks()/getShutdownHooks()/etc. accessors purely so Dispatcher could read them. That's a leaky API — internal process-global state promoted to public surface for the sake of one internal collaborator. Move the registries to a dedicated Hooks class (mirrors how Router owns its own static state). Http::init/shutdown/options/error/onStart/ onRequest remain as thin delegators for backwards compat; the six getXxxHooks accessors are gone. Dispatcher reads Hooks::\$init etc. directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Dispatcher::execute hoisted \$this->request->getParams() into a local before any init hooks fired, then passed that stale array to every subsequent getArguments() call. That broke the init-hook-mutates-params contract: if an init hook applied filter/rewrite via \$request->setQueryString(), the route action and shutdown hooks would still see the pre-mutation view. The old Http::execute called \$request->getParams() inline at each call site specifically to preserve this ordering. Restore that behaviour. Adds two regression tests: init-hook mutation visible to the route action, and init-hook mutation visible to a shutdown hook's params. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follows utopia-php/http#249 migration guide to drop usage of the shims kept for one-minor-release compat. Affected call sites switch to: - Handler / hook context: inject 'route' (Route|null from the per-request DI container) instead of $utopia->getRoute(). - Resource factories (init/resources/request.php): Router::matchRoute() returning ?RouteMatch instead of $utopia->match($request). - Error path in app/http.php: $app->getResource('routeMatch')?->route, guarded with try/catch for the pre-dispatch error case. - GraphQL resolver inner-request pattern (Resolvers.php): capture getResource('routeMatch') as the original, Router::matchRoute() for the inner URL, rebind 'route' / 'routeMatch' on the resolver container in finally instead of setRoute($original). Http::execute() shim is retained — the guide explicitly keeps it for this hand-built-route case. Redundant re-match in shared/api.php storage-cache branch removed; the injected $route is already the current one. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Router::matchRoute() takes (method, path) strings, forcing every caller to hand-roll URL parsing and HEAD-to-GET normalisation. The library already does this internally in Dispatcher::handle; the matching boilerplate should not be the caller's problem. Router::matchRequest(Request) wraps matchRoute and handles both. The deprecated Http::match() shim now delegates to it; Dispatcher continues to call matchRoute directly because it needs the parsed URL in scope for the wildcard fallback anyway. Adds three regression tests covering URL extraction, root-URL fallback when the URI has no path, and HEAD normalisation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two public matching entry points (matchRoute + matchRequest) were a mild smell; the method-agnostic wildcard living as a static singleton on Http was an outright special case leaking into the Dispatcher. Address both at once: - Make Router::matchRoute(string, string) private. Router::matchRequest is now the sole public matching API. External callers that used to hand-roll parse_url + HEAD normalisation to call matchRoute now just pass a Request. - Delete the deprecated Router::match(string, string): ?Route shim. - Move wildcard registration into Router via new Router::setWildcard; delete Http::\$wildcardRoute and Http::getWildcardRoute. Http::wildcard continues to return a Route, just registered on Router. - Dispatcher's match section collapses from the \$url/\$matchMethod/ wildcard-fallback block to a single Router::matchRequest call. - RouterTest/RouterMatchRouteTest migrate to matchRequest via a private test helper that sets \$_SERVER and builds an FPM Request. - Three new tests cover Router::setWildcard semantics (unknown path, unknown method, and method-specific precedence over the wildcard). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughRefactors the HTTP request lifecycle by introducing a per-request Changes
Sequence DiagramsequenceDiagram
participant Client
participant Http
participant Dispatcher
participant Router
participant Hooks
participant Action
Client->>Http: run()
Http->>Dispatcher: new + handle()
Dispatcher->>Router: matchRequest(request)
Router-->>Dispatcher: RouteMatch|null
alt Route Found
Dispatcher->>Hooks: invoke init hooks
Hooks-->>Dispatcher: (results)
Dispatcher->>Action: execute action
Action-->>Dispatcher: result
Dispatcher->>Hooks: invoke shutdown hooks
Hooks-->>Dispatcher: (results)
else No Route / Error
Dispatcher->>Hooks: invoke error hooks
Hooks-->>Dispatcher: (results)
end
Dispatcher-->>Http: (lifecycle complete)
Http-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
| * call {@see self::matchRequest()} so URL parsing and HEAD normalisation | ||
| * are handled consistently. | ||
| */ | ||
| private static function matchRoute(string $method, string $path): ?RouteMatch |
There was a problem hiding this comment.
Router::matchRoute is private but documented as a public replacement
The PR's migration guide explicitly tells callers to use Router::matchRoute($method, $path) as the low-level alternative when no Request object is available, and the breaking-changes table lists Router::match() as having a "deprecation shim" pointing at Router::matchRoute(). But matchRoute is private, so:
- Any existing caller of
Router::match($method, $path)gets an immediate fatal call-to-undefined-method error (no shim was added toRouter). - Developers following the migration guide and trying
Router::matchRoute($method, $path)directly will hit a fatal error calling a private method.
The only public entry point is Router::matchRequest(Request $request), which requires wrapping a bare method+path pair in a synthetic Request object. Either add a deprecation shim for Router::match() or make matchRoute public so the migration guide is accurate.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/Http/Router.php (2)
182-188: Minor:$currentis implicitly scoped to the enclosing function, not the loop.
$current = ($current ?? '') . "{$part}/";relies on$currentbeing undefined on entry, but because PHP variables leak out of blocks, ifmatchRoute()were ever refactored to reuse$currentearlier in the method, the coalesce would silently carry stale state. An explicit$current = '';immediately above theforeachmakes the intent clearer and is defensive against future edits.♻️ Suggested tweak
/** * Match wildcard for path segments (e.g. GET /foo/*). */ + $current = ''; foreach ($parts as $part) { - $current = ($current ?? '') . "{$part}/"; + $current .= "{$part}/"; $match = $current . self::WILDCARD_TOKEN; if (array_key_exists($match, self::$routes[$method])) { return new RouteMatch(self::$routes[$method][$match], $path, $match, $match); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/Router.php` around lines 182 - 188, In matchRoute (src/Http/Router.php) the loop uses $current = ($current ?? '') . "{$part}/"; which relies on $current being undefined and can carry stale state; explicitly initialize $current = ''; immediately before the foreach over $parts to make the intent clear and prevent variable leakage, leaving the rest of the loop and the RouteMatch return logic (new RouteMatch(self::$routes[$method][$match], $path, $match, $match)) unchanged.
149-200:routeKeyandpreparedPathare always passed the same value — consider consolidating or clarifying intent.In every
new RouteMatch(...)construction insidematchRoute()(and in the wildcard fallback), the 3rd and 4th arguments are the same$match/WILDCARD_TOKENstring. If these fields are semantically equivalent today, carrying both inRouteMatchis redundant surface area that callers have to reason about. If they are intended to diverge (e.g.,routeKey= canonical router-table key vspreparedPath= actual prepared path that matched, which differs for aliases), then passing the same value here silently loses that distinction.Worth either collapsing to a single field, or documenting on
RouteMatchwhat each is meant to represent so future callers don't treat them as interchangeable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/Router.php` around lines 149 - 200, The code currently passes the same value for the 3rd and 4th RouteMatch constructor args (routeKey and preparedPath) in matchRoute(), which either hides intended semantics or is redundant; fix by choosing one of two approaches: (A) collapse the two fields into one in the RouteMatch class (remove preparedPath or routeKey from the constructor and update all usages) and update matchRoute() calls (new RouteMatch(..., $path, $match)), or (B) preserve both semantics by computing and passing distinct values: keep routeKey as the canonical router table key ($match or self::WILDCARD_TOKEN) and set preparedPath to the actual normalized/prepared request path (e.g., the $path after trimming/normalizing or an alias-resolved value) when constructing RouteMatch in matchRoute(), ensuring all other call sites handle the new meaning; update RouteMatch docblocks to describe routeKey vs preparedPath accordingly.src/Http/Hooks.php (2)
22-40: Public static registries expose mutable state that the docblock says must not be mutated.The class comment states hooks "must not be mutated after the server starts accepting requests", but declaring the arrays
public staticmeans any caller — including handler code running mid-request — can append to or wholesale reassign them. That's effectively the same class of shared-mutable-state hazard this refactor is trying to close onHttp/Route.Given registration already goes through the static factory methods (
init(),shutdown(), …), making the arraysprotectedand exposing read-only accessors forDispatcherwould make the invariant enforceable. The "six getXxx accessors" cost noted in the docblock seems worth paying for a concurrency-safety class.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/Hooks.php` around lines 22 - 40, Change the public static hook registries in class Hooks (currently public static arrays $init, $shutdown, $options, $errors, $start, $request) to protected static so external code cannot mutate them directly, and add six read-only accessor methods (e.g. getInit(), getShutdown(), getOptions(), getErrors(), getStart(), getRequest()) that return the arrays for use by Dispatcher; update any call sites that read the registries (such as Dispatcher) to use these new getters and keep existing static factory registration methods (init(), shutdown(), etc.) as the only mutation entrypoints.
93-111: Asymmetric group defaults between lifecycle hooks andonStart/onRequestis non-obvious.
init/shutdown/options/errorall default togroups(['*'])butonStart/onRequestdo not. If this is intentional (e.g., start/request fire unconditionally regardless of group matching), a one-line comment inside the factories would make it explicit and prevent a well-meaning "fix" later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/Hooks.php` around lines 93 - 111, The onStart() and onRequest() factory methods (which create Hook instances and append to self::$start and self::$request) differ from init/shutdown/options/error which default to groups(['*']); if this is intentional, add a concise one-line comment inside the onStart and onRequest factories stating that these hooks run unconditionally (do not use group matching) so future maintainers don't change behavior — alternatively, if they should follow the same group-default behavior, set the created Hook to groups(['*']) before returning; update the Hook creation in onStart and onRequest accordingly and document the chosen behavior in that one-line comment.tests/RouterTest.php (1)
12-28: Consider saving/restoring$_SERVERglobals for test isolation.
match()mutates$_SERVER['REQUEST_METHOD']and$_SERVER['REQUEST_URI']buttearDown()only resets theRouter.HttpTestandDispatcherTestsave/restore these in setUp/tearDown — worth doing the same here for consistency so leftover globals don't leak into other suites that happen to run afterwards in the same process.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/RouterTest.php` around lines 12 - 28, Tests mutate $_SERVER['REQUEST_METHOD'] and $_SERVER['REQUEST_URI'] in the private match(string $method, string $path): ?Route method but tearDown() only calls Router::reset(), so restore the original globals to avoid leaking state; add a setUp() that saves the current $_SERVER entries (or a snapshot of $_SERVER) to a private property (e.g. $originalServer) and update tearDown() to restore those saved values before/after calling Router::reset(); reference the match(), setUp(), tearDown(), Router::reset(), and the $_SERVER['REQUEST_METHOD']/$_SERVER['REQUEST_URI'] keys when making the changes.src/Http/Dispatcher.php (1)
51-51: Catch\Throwablefor consistency with the rest of this class.Every other error-catching site in this file (Lines 59, 106, 179, 187, 198) uses
\Throwable. Only the onRequest hook block here narrows to\Exception, so aTypeError/AssertionErrorthrown from a user hook silently bypasses the wildcard error handlers and escapes the dispatcher.♻️ Proposed change
- } catch (\Exception $e) { + } catch (\Throwable $e) { $this->http->setRequestResource('error', fn() => $e);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/Dispatcher.php` at line 51, Change the catch in the onRequest hook from catching \Exception to catching \Throwable so errors like TypeError/AssertionError don't bypass the dispatcher's wildcard handlers; locate the catch (\Exception $e) in the onRequest hook inside Dispatcher.php and replace it with catch (\Throwable $e) while keeping the existing error handling logic and variable name ($e) intact to match the other catch sites in this class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Http/Dispatcher.php`:
- Around line 46-76: After running onRequest hooks and then wildcard error
handlers in the catch block (Hooks::$request / Hooks::$errors) the code
continues into static file serving and route dispatch, which can cause double
responses; modify Dispatcher::handle() to abort further processing if an error
handler already sent a response—either return early from the catch block after
running error handlers or add a guard before static-file serving and route
execution using $this->response->isSent() (same check used in execute()) so that
isFileLoaded(), ->send(), and subsequent route init/execute/shutdown are skipped
when a response is already sent.
- Around line 66-89: The Dispatcher currently checks for static files before
normalizing HEAD requests, so HEAD requests to preloaded files still send
bodies; to fix, obtain and normalize the request method early (call
$this->request->getMethod() and if it's Http::REQUEST_METHOD_HEAD set $method =
Http::REQUEST_METHOD_GET and call $this->response->disablePayload()) before the
isFileLoaded($this->request->getURI()) branch so that when isFileLoaded(...)
triggers the subsequent ->send(...) it will have payloads disabled for HEAD;
update references to $method and keep existing use of isFileLoaded,
getFileContents, send, matchRequest, setRequestResource, etc., unchanged except
for moving the normalization logic earlier in Dispatcher.
---
Nitpick comments:
In `@src/Http/Dispatcher.php`:
- Line 51: Change the catch in the onRequest hook from catching \Exception to
catching \Throwable so errors like TypeError/AssertionError don't bypass the
dispatcher's wildcard handlers; locate the catch (\Exception $e) in the
onRequest hook inside Dispatcher.php and replace it with catch (\Throwable $e)
while keeping the existing error handling logic and variable name ($e) intact to
match the other catch sites in this class.
In `@src/Http/Hooks.php`:
- Around line 22-40: Change the public static hook registries in class Hooks
(currently public static arrays $init, $shutdown, $options, $errors, $start,
$request) to protected static so external code cannot mutate them directly, and
add six read-only accessor methods (e.g. getInit(), getShutdown(), getOptions(),
getErrors(), getStart(), getRequest()) that return the arrays for use by
Dispatcher; update any call sites that read the registries (such as Dispatcher)
to use these new getters and keep existing static factory registration methods
(init(), shutdown(), etc.) as the only mutation entrypoints.
- Around line 93-111: The onStart() and onRequest() factory methods (which
create Hook instances and append to self::$start and self::$request) differ from
init/shutdown/options/error which default to groups(['*']); if this is
intentional, add a concise one-line comment inside the onStart and onRequest
factories stating that these hooks run unconditionally (do not use group
matching) so future maintainers don't change behavior — alternatively, if they
should follow the same group-default behavior, set the created Hook to
groups(['*']) before returning; update the Hook creation in onStart and
onRequest accordingly and document the chosen behavior in that one-line comment.
In `@src/Http/Router.php`:
- Around line 182-188: In matchRoute (src/Http/Router.php) the loop uses
$current = ($current ?? '') . "{$part}/"; which relies on $current being
undefined and can carry stale state; explicitly initialize $current = '';
immediately before the foreach over $parts to make the intent clear and prevent
variable leakage, leaving the rest of the loop and the RouteMatch return logic
(new RouteMatch(self::$routes[$method][$match], $path, $match, $match))
unchanged.
- Around line 149-200: The code currently passes the same value for the 3rd and
4th RouteMatch constructor args (routeKey and preparedPath) in matchRoute(),
which either hides intended semantics or is redundant; fix by choosing one of
two approaches: (A) collapse the two fields into one in the RouteMatch class
(remove preparedPath or routeKey from the constructor and update all usages) and
update matchRoute() calls (new RouteMatch(..., $path, $match)), or (B) preserve
both semantics by computing and passing distinct values: keep routeKey as the
canonical router table key ($match or self::WILDCARD_TOKEN) and set preparedPath
to the actual normalized/prepared request path (e.g., the $path after
trimming/normalizing or an alias-resolved value) when constructing RouteMatch in
matchRoute(), ensuring all other call sites handle the new meaning; update
RouteMatch docblocks to describe routeKey vs preparedPath accordingly.
In `@tests/RouterTest.php`:
- Around line 12-28: Tests mutate $_SERVER['REQUEST_METHOD'] and
$_SERVER['REQUEST_URI'] in the private match(string $method, string $path):
?Route method but tearDown() only calls Router::reset(), so restore the original
globals to avoid leaking state; add a setUp() that saves the current $_SERVER
entries (or a snapshot of $_SERVER) to a private property (e.g. $originalServer)
and update tearDown() to restore those saved values before/after calling
Router::reset(); reference the match(), setUp(), tearDown(), Router::reset(),
and the $_SERVER['REQUEST_METHOD']/$_SERVER['REQUEST_URI'] keys when making the
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6fd1da5-889c-4090-9151-410702968ab9
📒 Files selected for processing (11)
src/Http/Dispatcher.phpsrc/Http/Hooks.phpsrc/Http/Http.phpsrc/Http/Route.phpsrc/Http/RouteMatch.phpsrc/Http/Router.phptests/DispatcherTest.phptests/HttpTest.phptests/RouteMatchTest.phptests/RouterMatchRouteTest.phptests/RouterTest.php
💤 Files with no reviewable changes (1)
- src/Http/Route.php
| try { | ||
| foreach (Hooks::$request as $hook) { | ||
| $arguments = $this->http->getArguments($hook, [], []); | ||
| \call_user_func_array($hook->getAction(), $arguments); | ||
| } | ||
| } catch (\Exception $e) { | ||
| $this->http->setRequestResource('error', fn() => $e); | ||
|
|
||
| foreach (Hooks::$errors as $error) { | ||
| if (\in_array('*', $error->getGroups())) { | ||
| try { | ||
| $arguments = $this->http->getArguments($error, [], []); | ||
| \call_user_func_array($error->getAction(), $arguments); | ||
| } catch (\Throwable $e) { | ||
| throw new Exception('Error handler had an error: ' . $e->getMessage(), 500, $e); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if ($this->http->isFileLoaded($this->request->getURI())) { | ||
| $time = (60 * 60 * 24 * 365 * 2); | ||
|
|
||
| $this->response | ||
| ->setContentType($this->http->getFileMimeType($this->request->getURI())) | ||
| ->addHeader('Cache-Control', 'public, max-age=' . $time) | ||
| ->addHeader('Expires', \date('D, d M Y H:i:s', \time() + $time) . ' GMT') | ||
| ->send($this->http->getFileContents($this->request->getURI())); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
Fall-through after onRequest error handling can double-send / dispatch a route.
After catching an onRequest hook error and running wildcard error handlers (which almost always write a response), control continues into static-file serving (Lines 66–76) and then into route matching + execute(). That means the matched route action and its init/shutdown hooks can still run after the request was already rejected, and ->send() can be invoked twice. Consider returning early from the catch block, or guarding the rest of handle() with if ($this->response->isSent()) { return; } — similar to the isSent() check already used at Line 160 inside execute().
🔧 Proposed fix
} catch (\Exception $e) {
$this->http->setRequestResource('error', fn() => $e);
foreach (Hooks::$errors as $error) {
if (\in_array('*', $error->getGroups())) {
try {
$arguments = $this->http->getArguments($error, [], []);
\call_user_func_array($error->getAction(), $arguments);
} catch (\Throwable $e) {
throw new Exception('Error handler had an error: ' . $e->getMessage(), 500, $e);
}
}
}
+
+ return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Http/Dispatcher.php` around lines 46 - 76, After running onRequest hooks
and then wildcard error handlers in the catch block (Hooks::$request /
Hooks::$errors) the code continues into static file serving and route dispatch,
which can cause double responses; modify Dispatcher::handle() to abort further
processing if an error handler already sent a response—either return early from
the catch block after running error handlers or add a guard before static-file
serving and route execution using $this->response->isSent() (same check used in
execute()) so that isFileLoaded(), ->send(), and subsequent route
init/execute/shutdown are skipped when a response is already sent.
| if ($this->http->isFileLoaded($this->request->getURI())) { | ||
| $time = (60 * 60 * 24 * 365 * 2); | ||
|
|
||
| $this->response | ||
| ->setContentType($this->http->getFileMimeType($this->request->getURI())) | ||
| ->addHeader('Cache-Control', 'public, max-age=' . $time) | ||
| ->addHeader('Expires', \date('D, d M Y H:i:s', \time() + $time) . ' GMT') | ||
| ->send($this->http->getFileContents($this->request->getURI())); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| $method = $this->request->getMethod(); | ||
| $this->match = Router::matchRequest($this->request); | ||
|
|
||
| $this->http->setRequestResource('route', fn() => $this->match?->route); | ||
| $this->http->setRequestResource('routeMatch', fn() => $this->match); | ||
|
|
||
| $groups = $this->match?->route->getGroups() ?? []; | ||
|
|
||
| if (Http::REQUEST_METHOD_HEAD === $method) { | ||
| $method = Http::REQUEST_METHOD_GET; | ||
| $this->response->disablePayload(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether Response::send honors disablePayload via an internal flag
# and whether any existing test covers HEAD-against-static-file to document
# pre-existing vs regression behavior.
rg -nP --type=php -C3 '\bdisablePayload\s*\('
rg -nP --type=php -C3 'function\s+send\s*\(' -g 'src/Http/Response*.php'
rg -nP --type=php -C3 'REQUEST_METHOD_HEAD|isFileLoaded' -g 'tests/**'Repository: utopia-php/http
Length of output: 1850
🏁 Script executed:
sed -n '537,620p' src/Http/Response.phpRepository: utopia-php/http
Length of output: 2812
🏁 Script executed:
sed -n '640,680p' src/Http/Response.phpRepository: utopia-php/http
Length of output: 1057
🏁 Script executed:
sed -n '60,95p' src/Http/Http/Dispatcher.phpRepository: utopia-php/http
Length of output: 131
🏁 Script executed:
# Search for any test that uses HEAD method or tests static files
rg -i "head.*static|static.*head" tests/ --type=php
rg "REQUEST_METHOD_HEAD" tests/ --type=php -B2 -A2
rg "loadFiles|isFileLoaded" tests/ --type=php -B2 -A2Repository: utopia-php/http
Length of output: 41
🏁 Script executed:
fd -type f "Dispatcher.php" | head -5Repository: utopia-php/http
Length of output: 230
🏁 Script executed:
# Find correct path and read the file
DISP_PATH=$(fd -type f "Dispatcher.php" | head -1)
sed -n '60,95p' "$DISP_PATH"Repository: utopia-php/http
Length of output: 274
🏁 Script executed:
# Search for tests that exercise Dispatcher with static files or HEAD
rg "Dispatcher|loadFiles" tests/ --type=php -l | head -10Repository: utopia-php/http
Length of output: 84
🏁 Script executed:
fd "Dispatcher.php" --type f | grep -v testRepository: utopia-php/http
Length of output: 83
🏁 Script executed:
# Once we find it, read the dispatcher code
DISP=$(fd "Dispatcher.php" --type f | grep -v test)
sed -n '60,95p' "$DISP"Repository: utopia-php/http
Length of output: 1471
🏁 Script executed:
# Check DispatcherTest.php for HEAD or static file tests
head -100 tests/DispatcherTest.phpRepository: utopia-php/http
Length of output: 3219
🏁 Script executed:
# Search for HEAD method tests in DispatcherTest
rg -A10 -B5 "HEAD|head|loadFile" tests/DispatcherTest.phpRepository: utopia-php/http
Length of output: 739
🏁 Script executed:
# Check if Response::send has any other HEAD handling besides disablePayload
rg -C5 "REQUEST_METHOD_HEAD" src/Http/Response.phpRepository: utopia-php/http
Length of output: 41
🏁 Script executed:
# Double-check: does Response have any HEAD-specific logic in send/write methods?
rg "HEAD|head" src/Http/Response.php | grep -i "method\|header"Repository: utopia-php/http
Length of output: 2886
HEAD requests to preloaded static files still stream the full body.
$this->response->disablePayload() is only invoked at line 88, after the static-file short-circuit at lines 66–76 has already called ->send($fileContents). HEAD requests whose URI matches a file loaded via loadFiles() will emit the body instead of headers-only, violating HTTP HEAD semantics. The existing test testHeadRequestResolvesToGetRouteWithPayloadDisabled covers HEAD for routes but not static files.
Normalize the method before the file-serving branch:
🔧 Proposed fix
+ $method = $this->request->getMethod();
+ if (Http::REQUEST_METHOD_HEAD === $method) {
+ $this->response->disablePayload();
+ }
+
if ($this->http->isFileLoaded($this->request->getURI())) {
$time = (60 * 60 * 24 * 365 * 2);
$this->response
->setContentType($this->http->getFileMimeType($this->request->getURI()))
->addHeader('Cache-Control', 'public, max-age=' . $time)
->addHeader('Expires', \date('D, d M Y H:i:s', \time() + $time) . ' GMT')
->send($this->http->getFileContents($this->request->getURI()));
return;
}
- $method = $this->request->getMethod();
$this->match = Router::matchRequest($this->request);
$this->http->setRequestResource('route', fn() => $this->match?->route);
$this->http->setRequestResource('routeMatch', fn() => $this->match);
$groups = $this->match?->route->getGroups() ?? [];
if (Http::REQUEST_METHOD_HEAD === $method) {
$method = Http::REQUEST_METHOD_GET;
- $this->response->disablePayload();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Http/Dispatcher.php` around lines 66 - 89, The Dispatcher currently
checks for static files before normalizing HEAD requests, so HEAD requests to
preloaded files still send bodies; to fix, obtain and normalize the request
method early (call $this->request->getMethod() and if it's
Http::REQUEST_METHOD_HEAD set $method = Http::REQUEST_METHOD_GET and call
$this->response->disablePayload()) before the
isFileLoaded($this->request->getURI()) branch so that when isFileLoaded(...)
triggers the subsequent ->send(...) it will have payloads disabled for HEAD;
update references to $method and keep existing use of isFileLoaded,
getFileContents, send, matchRequest, setRequestResource, etc., unchanged except
for moving the normalization logic earlier in Dispatcher.
1 participant
Summary
Under Swoole coroutines, the
Httpsingleton andRouter-registeredRouteobjects are shared by every concurrent request. Several fields on those shared objects were being written per-request —Http::$route,Route::$matchedPath, and a->path($requestUri)mutation on the wildcardRoute— creating races where one coroutine's state could clobber another's between match and execution.This PR extracts per-request state into a new
Dispatcherand an immutableRouteMatchvalue object, and leavesHttp/Router/Routecarrying only bootstrap-time configuration. Hook registries move to a new dedicatedHooksclass. Router public API collapses to a single matching entry point.Httpno longer holds$this->route; the matched route flows through the per-request DI container (already coroutine-local via the Swoole adapters'Coroutine::getContext()).Routeis frozen after registration:setMatchedPath/getMatchedPathare deleted, and the wildcard route is no longer mutated with the request path on each hit.Router::matchRequest(Request)is the sole public matching entry point, returningfinal readonly class RouteMatch. The string-basedRouter::match(string, string)is gone, andRouter::matchRoute(string, string)is private.RouterviaRouter::setWildcard(Route).Http::$wildcardRoute/Http::getWildcardRoute()deleted;Http::wildcard()still returns aRoute, now registered withRouter::setWildcardunder the hood. Dispatcher no longer special-cases wildcard fallback.Hooksowns the six lifecycle-hook registries (init/shutdown/options/error/start/request) that used to live asprotected staticfields onHttp.Http::init()etc. delegate toHooks::init()etc. — same public API, cleaner internals.Breaking changes
Public API changes. Deprecation shims are kept for one minor release on the
Httpsurface; theRoutersurface breaks outright (matching semantics change shape, no backwards-compat returnable).Route::setMatchedPath()/Route::getMatchedPath()$routeMatch->routeKeyor$routeMatch->preparedPathfrom the per-requestrouteMatchresource.Http::getRoute()(deprecation shim)$http->getResource('routeMatch')?->route— or injectroute/routeMatchinto your handler.Http::setRoute(Route)(deprecation shim)setRequestResource('routeMatch', fn() => new RouteMatch(...)). Tests/legacy only; the new dispatcher populates this automatically.Http::match(Request, bool $fresh)(deprecation shim)Router::matchRequest($request)— returns?RouteMatch. The$freshargument is now a no-op.Http::execute(Route, Request, Response)(deprecation shim)Dispatcher.Router::match(string, string): ?RouteRouter::matchRequest($request).Router::matchRoute(string, string): ?RouteMatchRouter::matchRequest($request).Http::$wildcardRoute/Http::getWildcardRoute()Routernow.Http::wildcard()is unchanged. If you need to inspect or reset the wildcard directly, callRouter::setWildcard()/ include it inRouter::reset().Http::$init/$shutdown/$options/$errors/$startHooks/$requestHooks(previouslyprotected static)Hooks::$init,Hooks::$shutdown,Hooks::$options,Hooks::$errors,Hooks::$start,Hooks::$requestinstead.Http::init()/shutdown()/options()/error()/onStart()/onRequest()are unchanged — they now delegate toHooks.Migration guide
1. Reading the matched route inside a handler / hook
Before:
After — preferred (via injection):
Or via the request resource:
2. Per-request route label mutations (e.g. Appwrite's
->label('router', true))Routeis now frozen after registration. Mutating a sharedRoute's labels from a handler is a concurrency hazard — the write lands on a process-global object seen by every coroutine.Http::get('/x')->label('router', true).$http->setResource('myRequestTag', fn () => new MyTag())— and mutate that. It lives in the per-request container (backed byCoroutine::getContext()in Swoole) and cannot clobber peer coroutines.3. Matching a route outside the dispatcher
Before:
After —
Router::matchRequesthandles URL parsing and HEAD-to-GET normalisation; the returnedRouteMatchcarries the per-request facts (the sharedRouteis never mutated):If you have only a method + path string and no
Request, construct a minimalRequest(the FPM adapter'sRequestreads$_SERVER['REQUEST_METHOD']/$_SERVER['REQUEST_URI']).Router::matchRoute(string, string)is no longer public.4. Wildcard inspection
Before:
After — the wildcard lives on
Routernow.Http::wildcard()still returns theRoutefor registration-time chaining. If you need to inspect or override it at runtime, useRouter::setWildcard($route).5. Direct access to
Httphook statics from subclassesRegistration continues to work through either surface —
Http::init()andHooks::init()are interchangeable.6. If you subclassed
Httpto overriderunInternal/executeDispatch now lives in
Utopia\Http\Dispatcher. For now, open an issue if you depended on overriding these.7.
Http::match($request, fresh: false)caching semanticsThe per-singleton cache of the last-matched route is gone — it was the core concurrency bug.
match()now always returns a fresh result and the$freshargument is ignored.Concurrency properties after this change
Routeis never mutated afterRouter::addRoute()(orRouter::setWildcard()) returns.Httpholds no per-request state.RouteMatchisfinal readonly class— immutable at the type level, not by convention.Coroutine::getContext(); the FPM adapter has one request per process and is trivially safe.Test plan
vendor/bin/phpunit— 113 tests, 322 assertions, all green.vendor/bin/phpstan analyse— no errors at level 7.vendor/bin/pint --test— clean.vendor/bin/rector process --dry-run— clean.🤖 Generated with Claude Code
Summary by CodeRabbit
Refactoring
Tests