Simplify DI container API by removing Dependency class usage#228
Merged
lohanidamodar merged 7 commits intomasterfrom Mar 16, 2026
Merged
Simplify DI container API by removing Dependency class usage#228lohanidamodar merged 7 commits intomasterfrom
lohanidamodar merged 7 commits intomasterfrom
Conversation
- Replace all Dependency class usage with new Container::set(string, callable) API - Add executeHook() method to Http class to replace removed Container::inject() - Update Route to call parent::__construct() for Hook's new constructor - Update all test files and examples to use new DI API - Resolves breaking changes from DI dropping Dependency/Injection classes https://claude.ai/code/session_0196tmVsX1KjJKmuHncp7HiV
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR replaces Dependency object-based DI with direct Container::set(string, callable, array) bindings across examples, tests, and e2e scripts, and bumps utopia-php/servers in composer.json from 0.2.* to 0.3.*. Http receives a new protected executeHook(Container, \Utopia\Servers\Hook) method, and several Http method signatures were changed to accept a Container and use container bindings for request/response/route/error lifecycle handling. Route::__construct now calls parent::__construct(), and Route::getAction() is expected to be callable by default. Tests and examples updated to the new container binding style. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Http/Http.php (1)
175-177: Minor cleanup: remove unused loop variable on Line 175.
$nameis not used in this loop.♻️ Proposed cleanup
- foreach ($hook->getInjections() as $name => $injection) { + foreach ($hook->getInjections() as $injection) { $dependencies[] = $injection; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/Http.php` around lines 175 - 177, The foreach is capturing an unused key ($name) when iterating hook injections; update the loop that iterates $hook->getInjections() to drop the unused key (e.g., iterate as foreach ($hook->getInjections() as $injection)) and push $injection into $dependencies, or replace the loop with a direct merge/append (using array_merge/array_values) to add the returned injections to $dependencies; locate the loop by the $hook->getInjections() call and the $dependencies array usage to apply the change.tests/HttpTest.php (1)
654-656: Make the init-hook route check explicit.Line 655 says route availability is being verified, but no assertion is made. Consider capturing the injected route and asserting it after
run()to make the test intention concrete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/HttpTest.php` around lines 654 - 656, The test's action callback registers a Route parameter but never asserts it; before calling run(), declare a local variable (e.g., $capturedRoute = null), assign the injected Route inside the ->action(function (Route $route) { $capturedRoute = $route; }) closure, call run(), then add assertions after run() that $capturedRoute is not null and has the expected properties (e.g., path, methods) to make the init-hook route check explicit; reference the action() closure, the Route parameter, and the test's run() call to locate where to add the capture and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/HttpTest.php`:
- Around line 631-635: The test sets the request factory on $this->context but
run() uses the cloned $context, causing the HEAD request to use the wrong
instance; change the call to set the 'request' factory on the cloned $context
(replace $this->context->set('request', ...) with $context->set('request', ...))
so run() receives the overridden Request (the closure creating Request() and
modifying $_SERVER should be attached to $context before calling run()).
---
Nitpick comments:
In `@src/Http/Http.php`:
- Around line 175-177: The foreach is capturing an unused key ($name) when
iterating hook injections; update the loop that iterates $hook->getInjections()
to drop the unused key (e.g., iterate as foreach ($hook->getInjections() as
$injection)) and push $injection into $dependencies, or replace the loop with a
direct merge/append (using array_merge/array_values) to add the returned
injections to $dependencies; locate the loop by the $hook->getInjections() call
and the $dependencies array usage to apply the change.
In `@tests/HttpTest.php`:
- Around line 654-656: The test's action callback registers a Route parameter
but never asserts it; before calling run(), declare a local variable (e.g.,
$capturedRoute = null), assign the injected Route inside the ->action(function
(Route $route) { $capturedRoute = $route; }) closure, call run(), then add
assertions after run() that $capturedRoute is not null and has the expected
properties (e.g., path, methods) to make the init-hook route check explicit;
reference the action() closure, the Route parameter, and the test's run() call
to locate where to add the capture and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 631b7e12-7262-4c5e-aa5c-9e660e28ee36
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
composer.jsonexample/src/server.phpsrc/Http/Http.phpsrc/Http/Route.phptests/HookTest.phptests/HttpTest.phptests/RouteTest.phptests/e2e/init.phptests/e2e/server-swoole-coroutine.phptests/e2e/server-swoole.php
The test was setting the request factory on $this->context instead of the cloned $context, so run() never received the HEAD request. Fixed the target container and updated the assertion to match actual HEAD behavior. https://claude.ai/code/session_0196tmVsX1KjJKmuHncp7HiV
ChiragAgg5k
reviewed
Mar 16, 2026
src/Http/Http.php
Outdated
|
|
||
| $context->set(clone $dependency->setName('request')->setCallback(fn () => $request)) | ||
| ->set(clone $dependency->setName('response')->setCallback(fn () => $response)); | ||
| $context->set('request', fn () => $request) |
Member
There was a problem hiding this comment.
any need to call this context? can we simply do $this-container below?
it helps consolidating dependency injection to the keyword container rather than splitting it to context and container. i would normally mistake context in the old terms of using arrays like we did before
ChiragAgg5k
approved these changes
Mar 16, 2026
Consolidates the DI variable naming to $container throughout, avoiding confusion with the legacy array-based context pattern. https://claude.ai/code/session_0196tmVsX1KjJKmuHncp7HiV
Replace `clone $this->container` with `new Container($this->container)` to use the parent-child delegation pattern introduced in utopia-php/di 0.3, consistent with how Base::prepare() already creates scoped containers. https://claude.ai/code/session_0196tmVsX1KjJKmuHncp7HiV
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/HttpTest.php (2)
686-694: Reusing cached request instance is valid but subtle.At line 689,
$context->get('request')->setMethod('OPTIONS')works becauseContainer::get()caches the resolved instance. The secondrun()call receives the same (now modified) Request object. This is correct but relies on container caching behavior—consider adding a brief comment for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/HttpTest.php` around lines 686 - 694, The test relies on Container::get() returning a cached Request instance so calling $context->get('request')->setMethod('OPTIONS') mutates the same Request used by $this->http->run($context); make this explicit by adding a brief inline comment above that line explaining the container caches resolved services (so the second run uses the same Request object) and, if desired, consider calling $context->get('request') into a local $request variable to show the intentional reuse; reference the get('request') call, Container::get(), and run() in the comment for clarity.
654-658: Unused$routeparameter appears intentional for injection verification.The static analysis flags
$routeas unused, but this appears to be intentional—the test verifies that the route can be injected into the init hook. Consider adding a minimal assertion or a comment to clarify the intent and silence the warning.♻️ Optional: Add explicit verification or suppress warning
Http::init() ->inject('route') ->action(function (Route $route) { - // Verify route is available in init hook + // Verify route is available in init hook (parameter intentionally unused) + \assert($route instanceof Route); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/HttpTest.php` around lines 654 - 658, The test currently injects a Route via Http::init()->inject('route')->action(function (Route $route) { ... }) but leaves the $route parameter unused, causing static-analysis warnings; update the closure to either perform a minimal assertion (e.g., assertNotNull/assertInstanceOf on $route) or add an explicit explanatory comment or an annotation to indicate the parameter is intentionally unused so the analyzer is satisfied, referencing the Http::init(), inject('route') and action(function (Route $route) { ... }) locations.src/Http/Http.php (1)
175-177: Unused loop variable$name.The static analysis correctly identified that
$nameis unused. Since you're only using the$injectionvalue, use_or simply iterate over values.♻️ Suggested fix
- foreach ($hook->getInjections() as $name => $injection) { + foreach ($hook->getInjections() as $injection) { $dependencies[] = $injection; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/Http.php` around lines 175 - 177, The foreach uses an unused key variable $name; update the loop over $hook->getInjections() to iterate values only (remove $name) so you only bind $injection and push it into $dependencies (i.e., change the foreach signature that currently uses "$name => $injection" to iterate values-only), keeping references to $hook->getInjections() and $dependencies unchanged.
🤖 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/Http.php`:
- Around line 661-684: Remove the unreachable duplicate OPTIONS branch: delete
the entire elseif block guarded by self::REQUEST_METHOD_OPTIONS (the block that
iterates self::$options for group-specific and global hooks and its catch that
runs self::$errors), since OPTIONS is already handled and returned earlier;
ensure no other code depends on that duplicate executeHook/prepare/error
handling logic and run tests to confirm behavior unchanged.
---
Nitpick comments:
In `@src/Http/Http.php`:
- Around line 175-177: The foreach uses an unused key variable $name; update the
loop over $hook->getInjections() to iterate values only (remove $name) so you
only bind $injection and push it into $dependencies (i.e., change the foreach
signature that currently uses "$name => $injection" to iterate values-only),
keeping references to $hook->getInjections() and $dependencies unchanged.
In `@tests/HttpTest.php`:
- Around line 686-694: The test relies on Container::get() returning a cached
Request instance so calling $context->get('request')->setMethod('OPTIONS')
mutates the same Request used by $this->http->run($context); make this explicit
by adding a brief inline comment above that line explaining the container caches
resolved services (so the second run uses the same Request object) and, if
desired, consider calling $context->get('request') into a local $request
variable to show the intentional reuse; reference the get('request') call,
Container::get(), and run() in the comment for clarity.
- Around line 654-658: The test currently injects a Route via
Http::init()->inject('route')->action(function (Route $route) { ... }) but
leaves the $route parameter unused, causing static-analysis warnings; update the
closure to either perform a minimal assertion (e.g.,
assertNotNull/assertInstanceOf on $route) or add an explicit explanatory comment
or an annotation to indicate the parameter is intentionally unused so the
analyzer is satisfied, referencing the Http::init(), inject('route') and
action(function (Route $route) { ... }) locations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce252bd8-6c10-486b-963f-c08e9af96bf9
📒 Files selected for processing (2)
src/Http/Http.phptests/HttpTest.php
…requests - testContainerIsolationBetweenRequests: verifies child containers resolve their own request/response independently and parent is not polluted - testContainerIsolationForErrors: verifies error state from one request does not leak into subsequent requests https://claude.ai/code/session_0196tmVsX1KjJKmuHncp7HiV
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Http/Http.php (1)
661-684:⚠️ Potential issue | 🟡 MinorDelete unreachable duplicate OPTIONS branch.
Line 661-684 is dead code: OPTIONS is already handled at Line 628-656 with an early return at Line 656, so this branch can never execute.
Suggested removal
- } elseif (self::REQUEST_METHOD_OPTIONS == $method) { - try { - foreach ($groups as $group) { - foreach (self::$options as $option) { // Group options hooks - if (in_array($group, $option->getGroups())) { - $this->executeHook($this->prepare($container, $option, [], $request->getParams()), $option); - } - } - } - - foreach (self::$options as $option) { // Global options hooks - if (in_array('*', $option->getGroups())) { - $this->executeHook($this->prepare($container, $option, [], $request->getParams()), $option); - } - } - } catch (\Throwable $e) { - foreach (self::$errors as $error) { // Global error hooks - if (in_array('*', $error->getGroups())) { - $container->set('error', fn () => $e); - - $this->executeHook($this->prepare($container, $error, [], $request->getParams()), $error); - } - } - } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/Http.php` around lines 661 - 684, Remove the unreachable duplicate OPTIONS branch in the Http class that checks self::REQUEST_METHOD_OPTIONS and executes group/global option and error hooks (the block that iterates self::$options and self::$errors and calls $this->prepare(...) and $this->executeHook(...)); since OPTIONS is already handled earlier with an early return, delete this entire conditional branch referencing self::REQUEST_METHOD_OPTIONS, self::$options, self::$errors, $this->prepare, and $this->executeHook to avoid dead code.
🤖 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/Http.php`:
- Line 175: The foreach uses an unused key variable $name which triggers static
analysis noise; change the loop from "foreach ($hook->getInjections() as $name
=> $injection)" to a value-only iteration "foreach ($hook->getInjections() as
$injection)" so only $injection is declared and used (leave the rest of the loop
body unchanged).
---
Duplicate comments:
In `@src/Http/Http.php`:
- Around line 661-684: Remove the unreachable duplicate OPTIONS branch in the
Http class that checks self::REQUEST_METHOD_OPTIONS and executes group/global
option and error hooks (the block that iterates self::$options and self::$errors
and calls $this->prepare(...) and $this->executeHook(...)); since OPTIONS is
already handled earlier with an early return, delete this entire conditional
branch referencing self::REQUEST_METHOD_OPTIONS, self::$options, self::$errors,
$this->prepare, and $this->executeHook to avoid dead code.
Drops the unused key variable from the getInjections() foreach at Http.php:175 to silence static analysis warnings. https://claude.ai/code/session_0196tmVsX1KjJKmuHncp7HiV
The elseif branch for REQUEST_METHOD_OPTIONS at line 661 was dead code — OPTIONS requests are already handled and returned earlier (lines 628-657). Removing the duplicate simplifies the control flow. https://claude.ai/code/session_0196tmVsX1KjJKmuHncp7HiV
abnegate
approved these changes
Mar 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors the codebase to use a simplified DI container API that eliminates the need for the
Dependencyclass. Instead of creatingDependencyobjects and setting properties, dependencies are now registered directly using the container'sset()method with a name and callback.Key Changes
Dependencyclass usage: Replaced all instances of creating and configuringDependencyobjects with directContainer::set(name, callback)callsexecuteHook()method: Introduced a new protected method inHttpclass to handle hook execution with proper dependency resolution and ordering$container->inject($hook, true)calls with$this->executeHook($container, $hook)for cleaner hook invocationutopia-php/serversfrom0.2.*to0.3.*to support the new simplified APIparent::__construct()call inRouteconstructor to properly initialize the parentHookclassImplementation Details
The new
executeHook()method:This change maintains backward compatibility in functionality while providing a cleaner, more intuitive API for dependency injection.
https://claude.ai/code/session_0196tmVsX1KjJKmuHncp7HiV
Summary by CodeRabbit
Chores
Improvements
Tests