Refactor DI container to align with PSR-11#4
Merged
ChiragAgg5k merged 15 commits intomainfrom Mar 12, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe DI container was refactored to support named contexts with per-context registrations and per-context instance caches. A concrete Resource class replaces the previous Injection/Dependency types and the Dependency class was removed. Container APIs were updated to be context-aware (DEFAULT_CONTEXT constant; set, setResource, get, getResource, getResources, has, inject, refresh, purge; getDefinition helper). Tests were expanded to cover context behavior, caching, refresh/purge, and cross-context invalidation. README usage examples and PHP requirement were updated. CI workflows were reorganized (added ci.yml; removed several older workflows) and composer dev tooling/scripts were modified. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
5-6:⚠️ Potential issue | 🟡 MinorUpdate badge URLs to reference the DI repository.
The Travis CI and Packagist badges still reference
utopia-php/httpwhile the package has been renamed toutopia-php/di. This inconsistency could confuse users about the build status and download metrics.Suggested fix for badge URLs
-[](https://travis-ci.org/utopia-php/http) - +[](https://travis-ci.org/utopia-php/di) +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 5 - 6, The README badges still point to utopia-php/http; update both badge URLs to reference the new package name utopia-php/di by replacing occurrences of "utopia-php/http" in the badge markdown (the Travis CI image/link and the Packagist downloads image) so they show build and download info for utopia-php/di instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 28-29: The README example uses an invalid PHP placeholder "new
PDO(...)" which can cause syntax errors if copied; update the $di->setResource
call for the 'db' resource (the second argument) to show a valid PDO constructor
signature or a clear inline comment instead of ellipses — e.g., use a concrete
placeholder like new PDO('dsn', 'username', 'password') or new PDO(/* DSN */, /*
USER */, /* PASS */) in the closure passed to $di->setResource('db', fn (array
$config) => ... , ['config']) so readers see a syntactically correct example.
---
Outside diff comments:
In `@README.md`:
- Around line 5-6: The README badges still point to utopia-php/http; update both
badge URLs to reference the new package name utopia-php/di by replacing
occurrences of "utopia-php/http" in the badge markdown (the Travis CI image/link
and the Packagist downloads image) so they show build and download info for
utopia-php/di instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ede8be96-c49f-45ec-891d-c1f4faf3580d
📒 Files selected for processing (4)
README.mdsrc/DI/Container.phpsrc/DI/Resource.phptests/ContainerTest.php
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/DI/Resource.php (2)
81-90:⚠️ Potential issue | 🔴 CriticalBug: Duplicate dependency check will never trigger.
array_key_exists($name, $this->dependencies)checks if$nameis an array key, but$this->dependenciesstores dependency names as values (indexed array). Usein_array()instead.🐛 Proposed fix
- public function inject(string $name): self - { - if (array_key_exists($name, $this->dependencies)) { - throw new Exception('Dependency already declared for '.$name); - } - - $this->dependencies[] = $name; - - return $this; - } + public function inject(string $name): self + { + if (in_array($name, $this->dependencies, true)) { + throw new Exception('Dependency already declared for '.$name); + } + + $this->dependencies[] = $name; + + return $this; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/DI/Resource.php` around lines 81 - 90, In Resource::inject, the duplicate check uses array_key_exists against $this->dependencies (which is an indexed list of names) so it never detects duplicates; change the check to use in_array($name, $this->dependencies, true) (strict) before pushing to $this->dependencies to correctly detect and throw for already-declared dependency names in the inject method.
73-74:⚠️ Potential issue | 🟡 MinorTypo in docblock.
"Depenedency" should be "Dependency".
📝 Proposed fix
/** - * Depenedency + * Add a dependency to be injected. * * `@param` string $name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/DI/Resource.php` around lines 73 - 74, Fix the typo in the docblock that currently reads "Depenedency" by changing it to "Dependency" in the Resource class/file (update the docblock associated with the Resource declaration or the property annotated by that comment so the documentation string is corrected).README.md (1)
5-6:⚠️ Potential issue | 🟡 MinorUpdate badge URLs to reference the correct repository.
The badges still reference
utopia-php/httpinstead ofutopia-php/di. Also, the Travis CI badge may be obsolete since CI has moved to GitHub Actions.📝 Suggested fix
-[](https://travis-ci.org/utopia-php/http) - +[](https://github.com/utopia-php/di/actions/workflows/ci.yml) +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 5 - 6, Update the README badges so they point to the correct repository and CI system: replace the Travis badge URL "https://travis-ci.org/utopia-php/http.svg?branch=master" and the Packagist downloads badge "https://img.shields.io/packagist/dt/utopia-php/http.svg" with equivalents for utopia-php/di (e.g. the GitHub Actions workflow badge for the repo and the Packagist downloads badge for utopia-php/di); locate the badge lines in README.md (the two image links) and either remove the obsolete Travis badge or swap it for the GitHub Actions badge URL for the utopia-php/di repository and update the Packagist badge to reference utopia-php/di.
🧹 Nitpick comments (3)
src/DI/Container.php (2)
114-123: Consider adding$freshparameter togetResources()for consistency.
get()andgetResource()accept a$freshparameter, butgetResources()does not. If batch resolution with fresh instances is a valid use case, consider adding this parameter for API consistency.♻️ Optional enhancement
- public function getResources(array $names, string $context = self::DEFAULT_CONTEXT): array + public function getResources(array $names, string $context = self::DEFAULT_CONTEXT, bool $fresh = false): array { $resources = []; foreach ($names as $name) { - $resources[$name] = $this->get($name, $context); + $resources[$name] = $this->get($name, $context, $fresh); } return $resources; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/DI/Container.php` around lines 114 - 123, Add an optional $fresh boolean parameter to Container::getResources(array $names, string $context = self::DEFAULT_CONTEXT, bool $fresh = false) and pass it through to each call to $this->get($name, $context, $fresh) so batch resolution can request fresh instances like get() and getResource() already support; update the method signature and any docblock to reflect the new parameter and default value, and ensure callers are updated where necessary.
151-169: No circular dependency detection.If resource A depends on B and B depends on A, the container will enter infinite recursion and eventually hit PHP's stack limit. Consider adding detection with a resolution stack if this is a concern.
♻️ Example approach using a resolution stack
+ /** + * `@var` array<string, bool> + */ + protected array $resolving = []; + public function inject(Resource $injection, string $context = self::DEFAULT_CONTEXT, bool $fresh = false): mixed { $this->instances[$context] ??= []; if (\array_key_exists($injection->getName(), $this->instances[$context]) && !$fresh) { return $this->instances[$context][$injection->getName()]; } + $key = $context . '::' . $injection->getName(); + if (isset($this->resolving[$key])) { + throw new Exception('Circular dependency detected: "' . $injection->getName() . '"'); + } + $this->resolving[$key] = true; + $arguments = []; foreach ($injection->getDependencies() as $dependency) { $arguments[] = $this->get($dependency, $context); } $resolved = \call_user_func_array($injection->getCallback(), $arguments); $this->instances[$context][$injection->getName()] = $resolved; + unset($this->resolving[$key]); + return $resolved; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/DI/Container.php` around lines 151 - 169, The inject method can recurse infinitely on circular dependencies; modify Container::inject to track a resolution stack (e.g., a private array like $resolving keyed by context and resource name) and push the current $injection->getName() before resolving dependencies, then check before recursing via $this->get($dependency, $context) whether that dependency name is already in the stack and throw a descriptive CircularDependencyException if so; ensure you pop the name from the stack after resolution and use the same stack when handling the $fresh logic and caching in $this->instances to avoid false positives..github/workflows/ci.yml (1)
53-77: Consider addingfail-fast: falseand dependency caching for the test matrix.The nightly PHP build can be unstable. With the default
fail-fast: true, a nightly failure would cancel the 8.2 and 8.3 jobs, potentially hiding useful results. Adding dependency caching would also speed up CI runs.♻️ Suggested improvements
tests: name: Tests runs-on: ubuntu-latest strategy: + fail-fast: false matrix: php-versions: ["8.2", "8.3", "nightly"] steps: - name: Checkout repository uses: actions/checkout@v4 - name: Setup PHP ${{ matrix.php-versions }} uses: shivammathur/setup-php@v2 with: php-version: ${{ matrix.php-versions }} coverage: none - name: Validate composer.json and composer.lock run: composer validate --strict + - name: Cache Composer dependencies + uses: actions/cache@v4 + with: + path: vendor + key: ${{ runner.os }}-php-${{ matrix.php-versions }}-${{ hashFiles('**/composer.lock') }} + restore-keys: | + ${{ runner.os }}-php-${{ matrix.php-versions }}- + - name: Install dependencies run: composer install --prefer-dist --no-progress - name: Run Tests run: composer test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 53 - 77, Update the "tests" job configuration to set strategy.fail-fast to false so matrix failures (including the unstable "nightly" PHP) don't cancel other runs, and add dependency caching around the composer steps: use actions/cache (referencing the "Install dependencies" and "Run Tests" steps) to cache composer cache and/or vendor files keyed by composer.lock and PHP version to speed CI; ensure the cache restore happens before "Install dependencies" and save after install so subsequent matrix jobs benefit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@README.md`:
- Around line 5-6: Update the README badges so they point to the correct
repository and CI system: replace the Travis badge URL
"https://travis-ci.org/utopia-php/http.svg?branch=master" and the Packagist
downloads badge "https://img.shields.io/packagist/dt/utopia-php/http.svg" with
equivalents for utopia-php/di (e.g. the GitHub Actions workflow badge for the
repo and the Packagist downloads badge for utopia-php/di); locate the badge
lines in README.md (the two image links) and either remove the obsolete Travis
badge or swap it for the GitHub Actions badge URL for the utopia-php/di
repository and update the Packagist badge to reference utopia-php/di.
In `@src/DI/Resource.php`:
- Around line 81-90: In Resource::inject, the duplicate check uses
array_key_exists against $this->dependencies (which is an indexed list of names)
so it never detects duplicates; change the check to use in_array($name,
$this->dependencies, true) (strict) before pushing to $this->dependencies to
correctly detect and throw for already-declared dependency names in the inject
method.
- Around line 73-74: Fix the typo in the docblock that currently reads
"Depenedency" by changing it to "Dependency" in the Resource class/file (update
the docblock associated with the Resource declaration or the property annotated
by that comment so the documentation string is corrected).
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 53-77: Update the "tests" job configuration to set
strategy.fail-fast to false so matrix failures (including the unstable "nightly"
PHP) don't cancel other runs, and add dependency caching around the composer
steps: use actions/cache (referencing the "Install dependencies" and "Run Tests"
steps) to cache composer cache and/or vendor files keyed by composer.lock and
PHP version to speed CI; ensure the cache restore happens before "Install
dependencies" and save after install so subsequent matrix jobs benefit.
In `@src/DI/Container.php`:
- Around line 114-123: Add an optional $fresh boolean parameter to
Container::getResources(array $names, string $context = self::DEFAULT_CONTEXT,
bool $fresh = false) and pass it through to each call to $this->get($name,
$context, $fresh) so batch resolution can request fresh instances like get() and
getResource() already support; update the method signature and any docblock to
reflect the new parameter and default value, and ensure callers are updated
where necessary.
- Around line 151-169: The inject method can recurse infinitely on circular
dependencies; modify Container::inject to track a resolution stack (e.g., a
private array like $resolving keyed by context and resource name) and push the
current $injection->getName() before resolving dependencies, then check before
recursing via $this->get($dependency, $context) whether that dependency name is
already in the stack and throw a descriptive CircularDependencyException if so;
ensure you pop the name from the stack after resolution and use the same stack
when handling the $fresh logic and caching in $this->instances to avoid false
positives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e975354b-afe7-4d7c-a0a4-d288f4f54b98
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.github/workflows/ci.yml.github/workflows/codeql-analysis.yml.github/workflows/lint.yml.github/workflows/test.ymlREADME.mdcomposer.jsonphpstan.neonsrc/DI/Container.phpsrc/DI/Dependency.phpsrc/DI/Resource.phptests/ContainerTest.php
💤 Files with no reviewable changes (4)
- .github/workflows/lint.yml
- src/DI/Dependency.php
- .github/workflows/codeql-analysis.yml
- .github/workflows/test.yml
✅ Files skipped from review due to trivial changes (1)
- phpstan.neon
There was a problem hiding this comment.
Pull request overview
This PR extracts a scoped resource container from the utopia-php/http library, refactoring it into a standalone utopia-php/di package with context-aware dependency injection. The key changes are the introduction of per-context resource registration, resolution, caching, refresh, and purge behaviors.
Changes:
Container.php: Complete rewrite of the container to support two-level$dependencies[$context][$name]and$instances[$context][$name]storage, with newsetResource,getResource,getResources,refresh, andpurgemethodsResource.php/Dependency.php: TheInjectionabstract base class is renamed toResource(concrete class), and the now-emptyDependencysubclass is removed- CI/tests/tooling: Three separate workflow files consolidated into a single
ci.yml, test suite significantly expanded,composer.jsonscripts renamed, and dev dependencies updated
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/DI/Container.php |
Core scoped DI container with context-aware resource caching, resolution, and lifecycle management |
src/DI/Resource.php |
Abstract Injection class promoted to concrete Resource class |
src/DI/Dependency.php |
Legacy Dependency class removed as it was an empty subclass |
tests/ContainerTest.php |
Namespace updated, tests expanded to cover per-context caching, overrides, refresh, and purge |
README.md |
Updated to reflect the DI library scope, PHP 8.2+ requirement, and new usage examples |
composer.json |
Dev dependency versions bumped, scripts renamed |
composer.lock |
Lock file updated for new dependency versions |
phpstan.neon |
Minor whitespace fix |
.github/workflows/ci.yml |
New consolidated CI workflow replacing three separate ones |
.github/workflows/test.yml |
Removed (merged into ci.yml) |
.github/workflows/lint.yml |
Removed (merged into ci.yml) |
.github/workflows/codeql-analysis.yml |
Removed (merged into ci.yml) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/DI/Container.php (2)
119-128: Consider adding$freshparameter for consistency.The
getResourcesmethod lacks the$freshparameter that bothget()andgetResource()support. This could limit flexibility when batch-retrieving resources that need fresh instances.♻️ Proposed change
- public function getResources(array $names, string $context = self::DEFAULT_CONTEXT): array + public function getResources(array $names, string $context = self::DEFAULT_CONTEXT, bool $fresh = false): array { $resources = []; foreach ($names as $name) { - $resources[$name] = $this->get($name, $context); + $resources[$name] = $this->get($name, $context, $fresh); } return $resources; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/DI/Container.php` around lines 119 - 128, The getResources method should accept and forward the $fresh flag like get() and getResource(); update the signature of getResources(array $names, string $context = self::DEFAULT_CONTEXT, bool $fresh = false) and when iterating names call $this->get($name, $context, $fresh) (or $this->getResource(...) if that is the intended internal call) so batch retrieval respects the fresh instance option; adjust any callers/tests as needed to pass the new parameter.
156-174: Consider adding circular dependency detection.The
injectmethod recursively resolves dependencies without tracking the resolution stack. If a resource depends on itself (directly or indirectly), this will cause infinite recursion and a stack overflow rather than a clear error message.This may be acceptable for a lightweight DI container if circular dependencies are considered a programmer error, but documenting this limitation or adding detection would improve developer experience.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/DI/Container.php` around lines 156 - 174, The inject method in Container currently resolves dependencies recursively without detecting cycles, which can lead to infinite recursion; update Container::inject (and possibly Container::get call sites) to track an active resolution stack keyed by context and resource name (e.g., push $injection->getName() when starting, pop on return) and detect if a name is already on the stack; when a cycle is found throw a clear exception (e.g., CircularDependencyException) that includes the cycle path or involved Resource names instead of recursing indefinitely; ensure the stack is cleaned up on both success and error to avoid leaking state across calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/DI/Container.php`:
- Around line 119-128: The getResources method should accept and forward the
$fresh flag like get() and getResource(); update the signature of
getResources(array $names, string $context = self::DEFAULT_CONTEXT, bool $fresh
= false) and when iterating names call $this->get($name, $context, $fresh) (or
$this->getResource(...) if that is the intended internal call) so batch
retrieval respects the fresh instance option; adjust any callers/tests as needed
to pass the new parameter.
- Around line 156-174: The inject method in Container currently resolves
dependencies recursively without detecting cycles, which can lead to infinite
recursion; update Container::inject (and possibly Container::get call sites) to
track an active resolution stack keyed by context and resource name (e.g., push
$injection->getName() when starting, pop on return) and detect if a name is
already on the stack; when a cycle is found throw a clear exception (e.g.,
CircularDependencyException) that includes the cycle path or involved Resource
names instead of recursing indefinitely; ensure the stack is cleaned up on both
success and error to avoid leaking state across calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de18b33c-9dc5-4341-af6e-1ea02e78f4df
📒 Files selected for processing (3)
README.mdsrc/DI/Container.phptests/ContainerTest.php
✅ Files skipped from review due to trivial changes (1)
- README.md
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/DI/Container.php (2)
202-219: Documentation doesn't reflect the special handling of DEFAULT_CONTEXT.The docstring states "Remove context-specific registrations and cached instances," but when
$context === DEFAULT_CONTEXT, only cached instances are cleared (registrations are preserved). Consider updating the documentation to clarify this behavior.📝 Suggested documentation update
/** - * Remove context-specific registrations and cached instances. + * Remove context-specific registrations and cached instances. + * For DEFAULT_CONTEXT, only cached instances are cleared; registrations are preserved. * * `@param` string $context * `@return` self */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/DI/Container.php` around lines 202 - 219, The docblock for Container::purge is inaccurate: when $context === self::DEFAULT_CONTEXT the method only clears cached instances ($this->instances[$context]) and does not remove registrations ($this->dependencies[$context]); update the PHPDoc to explicitly state this special-case behavior (that default context preserves registrations but clears instances) and ensure it references the involved symbols (Container::purge, self::DEFAULT_CONTEXT, $this->instances, $this->dependencies) so callers understand the difference.
119-128: Consider adding$freshparameter for consistency.The
getResources()method doesn't support the$freshflag thatget()andgetResource()provide. This creates an API inconsistency where batch retrieval always uses cached instances.♻️ Suggested improvement
- public function getResources(array $names, string $context = self::DEFAULT_CONTEXT): array + public function getResources(array $names, string $context = self::DEFAULT_CONTEXT, bool $fresh = false): array { $resources = []; foreach ($names as $name) { - $resources[$name] = $this->get($name, $context); + $resources[$name] = $this->get($name, $context, $fresh); } return $resources; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/DI/Container.php` around lines 119 - 128, Add a $fresh boolean parameter to getResources (default false) and pass it through to the underlying get() calls so batch retrieval honors the same freshness behavior as get() and getResource(); update the method signature (getResources(array $names, string $context = self::DEFAULT_CONTEXT, bool $fresh = false)) and change the loop to call $this->get($name, $context, $fresh) to preserve API consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/DI/Container.php`:
- Around line 202-219: The docblock for Container::purge is inaccurate: when
$context === self::DEFAULT_CONTEXT the method only clears cached instances
($this->instances[$context]) and does not remove registrations
($this->dependencies[$context]); update the PHPDoc to explicitly state this
special-case behavior (that default context preserves registrations but clears
instances) and ensure it references the involved symbols (Container::purge,
self::DEFAULT_CONTEXT, $this->instances, $this->dependencies) so callers
understand the difference.
- Around line 119-128: Add a $fresh boolean parameter to getResources (default
false) and pass it through to the underlying get() calls so batch retrieval
honors the same freshness behavior as get() and getResource(); update the method
signature (getResources(array $names, string $context = self::DEFAULT_CONTEXT,
bool $fresh = false)) and change the loop to call $this->get($name, $context,
$fresh) to preserve API consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 535f28f2-7d84-4d50-9a4a-4258e659105e
📒 Files selected for processing (2)
src/DI/Container.phptests/ContainerTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/ContainerTest.php
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/ContainerTest.php (1)
108-120: Consider adding explicit context for clarity.The test correctly demonstrates the
freshflag behavior, but line 118-119 use named arguments to skip the$contextparameter. While valid PHP 8 syntax, explicitly passing the context would make the test's intent clearer.♻️ Optional: Add explicit context for readability
- $this->assertSame(['counter' => 1], $this->container->getResources(['counter'])); - $this->assertSame(['counter' => 2], $this->container->getResources(['counter'], fresh: true)); + $this->assertSame(['counter' => 1], $this->container->getResources(['counter'], Container::DEFAULT_CONTEXT)); + $this->assertSame(['counter' => 2], $this->container->getResources(['counter'], Container::DEFAULT_CONTEXT, fresh: true));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ContainerTest.php` around lines 108 - 120, The test method testGetResourcesCanBypassCacheWithFreshFlag omits the explicit $context argument when calling getResources(['counter'], fresh: true); update the call to pass an explicit context (e.g. an empty array) so the signature is clear, e.g. call getResources with the context argument before the fresh flag; this change involves the getResources invocation in testGetResourcesCanBypassCacheWithFreshFlag to make the second parameter explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/ContainerTest.php`:
- Around line 108-120: The test method
testGetResourcesCanBypassCacheWithFreshFlag omits the explicit $context argument
when calling getResources(['counter'], fresh: true); update the call to pass an
explicit context (e.g. an empty array) so the signature is clear, e.g. call
getResources with the context argument before the fresh flag; this change
involves the getResources invocation in
testGetResourcesCanBypassCacheWithFreshFlag to make the second parameter
explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14a49b8a-bbaa-4b5b-a3b7-dfa960959e1f
📒 Files selected for processing (2)
src/DI/Container.phptests/ContainerTest.php
ChiragAgg5k
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 19 changed files in this pull request and generated 2 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.
ChiragAgg5k
force-pushed
the
chore/extract-http-di-resources
branch
from
22f684c to
f4bbf60
Compare
loks0n
approved these changes
Mar 12, 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
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
ContainerInterfaceimplementationscope()and introduce spec-compliant container exceptionsBreaking changes
Resourceabstraction and the context-based resource APIsContainer::set()now accepts an entry id and factory callable instead of aResourceVerification
composer testvendor/bin/phpstan analyse -c phpstan.neon --debug --no-progresscomposer validate --strict