Refactor: split static resources from per-request context#254
Conversation
Renames the request-scoped DI container concept to "context" and splits adapter container access into two methods: resources() (static, shared across requests) and context() (per-request). Removes the Http::setResource / getResource / getResources(list) wrappers in favor of going through resources()/context() directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
k6 benchmark
|
Greptile Summary
Confidence Score: 4/5Safe to merge with awareness of two unresolved P1 concerns from prior review threads that are still present in the code The refactoring is structurally sound and the new lifetime split is correctly implemented in all three adapters. No new P0/P1 issues were found in this pass. The ceiling is 4 because previously-flagged P1 issues — the shortened utopia coroutine context key and the startup error leaking into the long-lived resources container — remain unaddressed in the current diff. src/Http/Http.php (startup-error path at start() line ~522) and src/Http/Adapter/Swoole/Server.php + src/Http/Adapter/SwooleCoroutine/Server.php (CONTEXT_KEY value) Important Files Changed
Reviews (2): Last reviewed commit: "Chore: apply Rector first-class callable..." | Re-trigger Greptile |
| @@ -578,7 +534,7 @@ public function start(): void | |||
| } | |||
There was a problem hiding this comment.
Startup error permanently stored in the static resources container
When a server start hook throws, error is set on resources() — the long-lived, shared container. Because context() falls through to resources() on a miss, any subsequent per-request handler that injects error without having encountered its own error will silently receive the stale startup exception. The per-request error handlers (runInternal, execute) do shadow this by calling $this->context()->set('error', ...) on the per-request container, but only when an error actually occurs in that request. A happy-path request that happens to inject error will get the leftover boot failure instead of null or an exception.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cb1912d to
d77957d
Compare
The Http instance is shared across coroutines, so $this->route and $this->matchedPath would race the same way Route's mutable fields did. Store them in the per-request context() container instead, which is already request-scoped post-#254. getRoute()/setRoute() now read/write through the context too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Reworks how DI containers are exposed by the HTTP layer. The previous single
getContainer()API conflated two distinct lifetimes — long-lived "boot the server" wiring and short-lived per-request state. This PR makes the split explicit:resources()— static container, shared across every request for the lifetime of the server. Use this at boot to register config, clients, and shared services.context()— per-request child container, created fresh on each incoming request. Lookups fall through toresources(), so static services remain reachable from request handlers. The library writesrequest,response,route, anderrorinto this container.The internal coroutine context key is now
__utopia__.Breaking changes
AdaptergetContainer(): Containerresources(): Containercontext(): ContainerBoth new methods are abstract and documented on
Utopia\Http\Adapter. All three bundled adapters (FPM,Swoole,SwooleCoroutine) implement both.Adapter constructors
The container parameter has been renamed from
$containerto$resourcesand is now a constructor-promoted property on every adapter:If you were passing the container positionally to the Swoole adapters this still works. If you were passing it by name, rename
container:→resources:.Httpnew Http(Adapter $server, string $timezone, ?Container $container = null)new Http(Adapter $adapter, string $timezone, Files $files = new Files())Http::getResource(string $name): mixed$http->context()->get($name)Http::getResources(array $list): array$http->context()->get(...)per nameHttp::setResource(string $name, callable $cb, array $injections = [])$http->resources()->set(...)Http::resources(): Container(shortcut to$adapter->resources())Http::context(): Container(shortcut to$adapter->context())The
Httpconstructor no longer accepts a container — pass it to the adapter instead. The internal$serverfield has been renamed to$adapterto match the parameter type.Http::getResource()previously normalized DI errors intoUtopia\Http\Exceptionwith a "resource"-flavored message. That normalization is gone; lookups now surface the underlyingUtopia\DIexceptions directly.Migration
Non-breaking cleanup
Httpconstructor uses property promotion for$adapterand$files.Psr\Container\*Exceptionimports (no more error normalization).$resourcesnaming.$this->containerrenamed to$this->resources.Test plan
composer testpasses locally🤖 Generated with Claude Code