Skip to content

Move resource ownership into utopia-php/di#220

Open
ChiragAgg5k wants to merge 15 commits into0.34.xfrom
feat/use-di-resource-container
Open

Move resource ownership into utopia-php/di#220
ChiragAgg5k wants to merge 15 commits into0.34.xfrom
feat/use-di-resource-container

Conversation

@ChiragAgg5k
Copy link
Member

@ChiragAgg5k ChiragAgg5k commented Mar 10, 2026

Summary

  • remove the public HTTP resource registration API and move resource ownership to Utopia\DI\Container
  • inject a DI container into Http and use it internally for request-scoped framework resources like request, response, route, error, and adapter-specific raw resources
  • update adapters, tests, and docs to register custom resources directly on the DI container instead of Http::setResource()
  • keep hook and route ->inject() semantics in HTTP, but delegate resolution and lifecycle cleanup to DI
  • restore the e2e test runtime to a PHP 8.2-compatible setup so this branch can be validated in Docker again

Bug Exposed By This PR

  • moving HTTP resource ownership to utopia-php/di also raised the effective runtime floor for the branch because the DI package uses PHP 8.2 syntax
  • the existing FPM/Swoole test images were still built around the older setup, so CI started failing before the HTTP behavior could actually be exercised
  • after fixing the container/runtime mismatch, the remaining failing test exposed a latent FPM bug in chunked responses: Response::chunk() was re-sending headers after body output had already started, which triggered Cannot modify header information warnings and corrupted the response body

Fixes Included

  • pin the PHPUnit dependency graph back to a PHP 8.2-compatible doctrine/instantiator line in the lockfile
  • switch the FPM test image to an actual PHP 8.2 FPM runtime and update the test startup scripts to use the official image layout
  • update response streaming so headers/cookies are only emitted once for chunked responses, fixing the FPM warning and preserving the expected Hello World! body

Verification

  • composer validate --strict
  • docker compose exec fpm vendor/bin/phpunit --configuration phpunit.xml
  • docker compose exec swoole vendor/bin/phpunit --configuration phpunit.xml

Notes

  • this intentionally breaks the old Http::setResource() / Http::getResource() usage in favor of explicit DI ownership
  • the chunked response failure was not caused by DI resolution itself; the DI migration forced the branch onto a runtime where the pre-existing FPM header-ordering bug became visible

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 10, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6ad74ba1-02c7-4e6a-9847-a29751604885

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/use-di-resource-container

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ChiragAgg5k ChiragAgg5k changed the title Use utopia-php/di for resource injection Move resource ownership into utopia-php/di Mar 10, 2026
@ChiragAgg5k
Copy link
Member Author

@coderabbitai review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 10, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 to Utopia\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.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 129 to 135
public function __construct(Adapter $server, string $timezone)
{
\date_default_timezone_set($timezone);
$this->files = new Files();
$this->server = $server;
$this->container = new Container();
}
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +49 to 50
$http = new Http(new Server(), 'America/New_York', $container);
$http->start();
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +87 to 88
$http = new Http(new Server(), 'America/New_York', $container);
$http->start();
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
Comment on lines +113 to 114
$http = new Http(new Server('0.0.0.0', '80'), 'America/New_York', $container);
$http->start();
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants