Move resource ownership into utopia-php/di#220
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
This PR migrates HTTP “resource” ownership out of utopia-php/http and into utopia-php/di by injecting a DI container into Http and using it for request-scoped resources and adapter-provided raw resources.
Changes:
- Removed
Http::setResource()/Http::getResource()and delegated resource resolution/cleanup toUtopia\DI\Container. - Updated FPM/Swoole adapters to pass adapter-specific raw resources into
Http’s request lifecycle. - Updated tests and docs to register custom resources directly on the DI container, and bumped minimum PHP version to 8.2.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/Http/Http.php |
Introduces DI container integration for resource wiring, resolution, refresh/purge lifecycle, and removes legacy resource APIs. |
src/Http/Adapter/FPM/Server.php |
Passes adapter raw resources through the onRequest callback to be registered per request. |
src/Http/Adapter/Swoole/Server.php |
Same as FPM: provides raw Swoole request/response as request-scoped resources. |
tests/HttpTest.php |
Refactors tests to use the DI container for resource registration/resolution instead of Http. |
README.md |
Updates docs/examples to use the DI container and the new Http constructor signature. |
docs/Getting-Starting-Guide.md |
Attempts to update the getting-started snippet and PHP requirement. |
composer.json |
Adds utopia-php/di dependency, VCS repository entry, and raises PHP constraint to >=8.2. |
composer.lock |
Locks the new DI dependency and updates platform PHP to >=8.2. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| public function __construct(Adapter $server, string $timezone) | ||
| { | ||
| \date_default_timezone_set($timezone); | ||
| $this->files = new Files(); | ||
| $this->server = $server; | ||
| $this->container = new Container(); | ||
| } |
There was a problem hiding this comment.
Http's constructor still only accepts (Adapter $server, string $timezone) and always creates a new internal Container. This prevents consumers from registering application dependencies (and forces tests to use Closure::bind to access $container), and it contradicts the updated docs/README which show passing a container into the constructor. Consider adding an optional ?Container $container parameter (defaulting to a new instance) and assigning it to $this->container so resource ownership can actually be externalized as described in the PR.
| $http = new Http(new Server(), 'America/New_York', $container); | ||
| $http->start(); |
There was a problem hiding this comment.
This example calls new Http(..., $container), but Http::__construct currently only accepts (Adapter $server, string $timezone) and will throw an argument count error. Either update Http to accept an injected container (as the docs suggest) or adjust this snippet to match the actual constructor signature.
| $http = new Http(new Server(), 'America/New_York', $container); | ||
| $http->start(); |
There was a problem hiding this comment.
This snippet passes $container as a third argument to Http::__construct, but Http currently only accepts two constructor arguments. This will fail at runtime; please align the docs with the actual API (or update Http to accept the container).
| $http = new Http(new Server('0.0.0.0', '80'), 'America/New_York', $container); | ||
| $http->start(); |
There was a problem hiding this comment.
This Swoole example constructs Http with a third $container argument, but Http::__construct currently only accepts two parameters. Either make the constructor accept an injected container or update the example to avoid passing one.
Summary
Utopia\DI\ContainerHttpand use it internally for request-scoped framework resources likerequest,response,route,error, and adapter-specific raw resourcesHttp::setResource()->inject()semantics in HTTP, but delegate resolution and lifecycle cleanup to DIBug Exposed By This PR
utopia-php/dialso raised the effective runtime floor for the branch because the DI package uses PHP 8.2 syntaxResponse::chunk()was re-sending headers after body output had already started, which triggeredCannot modify header informationwarnings and corrupted the response bodyFixes Included
doctrine/instantiatorline in the lockfileHello World!bodyVerification
composer validate --strictdocker compose exec fpm vendor/bin/phpunit --configuration phpunit.xmldocker compose exec swoole vendor/bin/phpunit --configuration phpunit.xmlNotes
Http::setResource()/Http::getResource()usage in favor of explicit DI ownership