feat(refactor-deps): replace laminas-servicemanager with lightweight PSR-11 container#12
Open
Fahl-Design wants to merge 9 commits intomainfrom
Open
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:
📝 WalkthroughWalkthroughReplaces Laminas ServiceManager with a new PSR-11 Changes
Sequence DiagramsequenceDiagram
participant Client
participant SimpleContainer
participant Aliases
participant Cache
participant Factories
participant Delegators
Client->>SimpleContainer: get(id)
SimpleContainer->>Aliases: resolve(id)
Aliases-->>SimpleContainer: resolvedId
SimpleContainer->>Cache: has(resolvedId)?
alt cache hit
Cache-->>SimpleContainer: instance
SimpleContainer-->>Client: instance
else cache miss
SimpleContainer->>Factories: fetch factory(resolvedId)
Factories-->>SimpleContainer: factory
SimpleContainer->>SimpleContainer: invoke factory(this[, resolvedId])
SimpleContainer->>Delegators: apply delegators(resolvedId, next)
Delegators-->>SimpleContainer: decoratedInstance
SimpleContainer->>Cache: store(resolvedId, decoratedInstance)
SimpleContainer-->>Client: decoratedInstance
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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 docstrings
🧪 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 |
Fahl-Design
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Container/SimpleContainer.php`:
- Around line 52-53: Replace bare RuntimeException throws in SimpleContainer
(e.g., the is_callable($factory) check in the factory resolution, the invalid
delegator error, and the circular-alias failure) with exceptions implementing
Psr\Container\ContainerExceptionInterface; follow the existing pattern used for
the NotFound anonymous class by returning/throwing an anonymous class that
implements ContainerExceptionInterface and extends RuntimeException (or wraps
the message), or create a small dedicated ContainerException class and throw
that from resolveFactory, resolveDelegator, and alias-resolution code paths
(refer to symbols is_callable($factory), resolveDelegator/resolveFactory, and
alias resolution/circular-alias handling to locate each throw site).
- Around line 34-35: Remove the PHP 8-only return type from the
SimpleContainer::get method signature: locate the public function get(string
$id): mixed declaration and drop the ": mixed" return type so the method has no
PHP 8 return type, keeping only the parameter type; update or add a PHPDoc above
get() if needed to document the possible return types (e.g. mixed) for clarity
and PSR-11 compatibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 94b03de6-13a6-41e9-a63d-c21eb033cab4
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
composer.jsonconfig/container.phpphpstan.neonsrc/Container/SimpleContainer.phptests/phpstan-container.php
💤 Files with no reviewable changes (3)
- composer.json
- phpstan.neon
- tests/phpstan-container.php
…1 container Drop laminas/laminas-servicemanager to avoid downstream dependency conflicts. Introduce a minimal SimpleContainer that implements PSR-11 ContainerInterface and supports the Mezzio ConfigProvider format (factories, aliases, invokables, delegators). - Add src/Container/SimpleContainer.php - Update config/container.php to use SimpleContainer - Widen psr/container to ^1.1 || ^2.0 (matching Symfony/Mezzio convention) - Remove slam/phpstan-laminas-framework (tied to ServiceManager) - Remove tests/phpstan-container.php (no longer needed)
PSR-11 requires get() to throw ContainerExceptionInterface for general errors. Apply the same anonymous-class pattern already used for NotFoundExceptionInterface to factory, delegator, and circular-alias error paths.
resolveAlias() is called from both get() and has(); throwing ContainerExceptionInterface from has() would violate PSR-11.
…Exception types Add dedicated ContainerException and NotFoundException in Container\Exception namespace, replacing inline anonymous classes.
Fahl-Design
force-pushed
the
feature/replace-laminas-servicemanager-with-simple-container
branch
from
c850e76 to
eab20a6
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Container/SimpleContainer.php`:
- Around line 79-83: has() currently calls resolveAlias() which can throw a
ContainerException on circular aliases, violating PSR-11; change
resolveAlias(string $id, bool $throwOnCircular = true) to make circular-alias
detection conditional and update has() to call resolveAlias($id, false) and
return false when resolveAlias indicates a circular alias (or returns
null/false) while leaving get() to call resolveAlias($id, true) to preserve
existing exception behavior; reference resolveAlias, has, and get in the fix.
- Line 56: The get() method currently calls factories as $factory($this,
$resolved) which breaks 0- and 1-arg factories (e.g., closures from
setInvokableClass() and config factories like
OpenApiMockMiddlewareFactory::__invoke). Update the factory invocation in
SimpleContainer::get to detect the callable's required parameter count (via
ReflectionFunction/ReflectionMethod or ReflectionParameter inspection) and
invoke the factory with the appropriate arguments (no args, only $this, or $this
and $resolved) so all registered factories (0-, 1- and 2-arg) are called safely;
ensure you reference the factory callable used in get() and keep existing
resolved resolution logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5777a29-d309-4a92-a6dc-fc7af9a63c20
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
composer.jsonconfig/container.phpphpstan.neonsrc/Container/Exception/ContainerException.phpsrc/Container/Exception/NotFoundException.phpsrc/Container/SimpleContainer.phptests/phpstan-container.php
💤 Files with no reviewable changes (3)
- tests/phpstan-container.php
- composer.json
- phpstan.neon
✅ Files skipped from review due to trivial changes (3)
- src/Container/Exception/NotFoundException.php
- config/container.php
- src/Container/Exception/ContainerException.php
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Unit/SimpleContainerTest.php (1)
19-28: Remove stale comments in test method.Lines 25-26 contain comments that are now misleading: they describe a "current behavior" and "desired behavior" that suggest the test was written before the fix. Since the fix is now implemented and the test passes, these comments should be removed.
Suggested fix
public function testHasReturnsFalseOnCircularAlias(): void { $container = new SimpleContainer(); $container->setAlias('a', 'b'); $container->setAlias('b', 'a'); - // Current behavior: this throws ContainerException - // Desired behavior: this returns false self::assertFalse($container->has('a')); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/SimpleContainerTest.php` around lines 19 - 28, Remove the outdated explanatory comments inside the test method testHasReturnsFalseOnCircularAlias: open the method in SimpleContainerTest (the method that constructs a SimpleContainer, calls setAlias('a','b') and setAlias('b','a') and assertsFalse($container->has('a'))), and delete the two lines that start with "// Current behavior..." and "// Desired behavior..." so the test only contains the setup and assertion without stale commentary.
🤖 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/Unit/SimpleContainerTest.php`:
- Around line 19-28: Remove the outdated explanatory comments inside the test
method testHasReturnsFalseOnCircularAlias: open the method in
SimpleContainerTest (the method that constructs a SimpleContainer, calls
setAlias('a','b') and setAlias('b','a') and assertsFalse($container->has('a'))),
and delete the two lines that start with "// Current behavior..." and "//
Desired behavior..." so the test only contains the setup and assertion without
stale commentary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01897dd1-2d6f-4f91-a0f9-856a08586656
📒 Files selected for processing (3)
src/Container/SimpleContainer.phptests/Unit/SimpleContainerFactoryTest.phptests/Unit/SimpleContainerTest.php
✅ Files skipped from review due to trivial changes (1)
- tests/Unit/SimpleContainerFactoryTest.php
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
config/routes.php (1)
15-15:?: '/app'fallback is likely unreachable here.
dirname(__DIR__)should always return a non-empty string in this context, so the fallback adds noise without effect.♻️ Suggested simplification
- $packageRoot = dirname(__DIR__) ?: '/app'; + $packageRoot = dirname(__DIR__);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/routes.php` at line 15, The fallback "?: '/app'" is unnecessary because dirname(__DIR__) always returns a non-empty string here; simplify by removing the redundant null-coalescing fallback and assign $packageRoot directly from dirname(__DIR__) (update the assignment using the $packageRoot variable and the dirname(__DIR__) expression in config/routes.php).
🤖 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/Container/SimpleContainer.php`:
- Around line 160-177: The setters currently don't remove competing
registrations so older layers can shadow newer ones; update setService,
setFactory (and setInvokableClass), and setAlias to clear any conflicting
entries before writing: setService(string $name, mixed $service) should unset
$this->aliases[$name] and $this->factories[$name] before assigning
$this->services[$name]; setFactory(string $name, callable|string $factory) and
setInvokableClass(string $name, string $class) should unset
$this->aliases[$name] and $this->services[$name] before assigning
$this->factories[$name]; setAlias(string $alias, string $target) should unset
$this->services[$alias] and $this->factories[$alias] before assigning
$this->aliases[$alias]; this ensures the last registration wins given the lookup
order (aliases -> services -> factories).
---
Nitpick comments:
In `@config/routes.php`:
- Line 15: The fallback "?: '/app'" is unnecessary because dirname(__DIR__)
always returns a non-empty string here; simplify by removing the redundant
null-coalescing fallback and assign $packageRoot directly from dirname(__DIR__)
(update the assignment using the $packageRoot variable and the dirname(__DIR__)
expression in config/routes.php).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46d15eeb-82ea-4bab-9a96-8cbe13744396
📒 Files selected for processing (4)
config/routes.phpphpstan.neonsrc/Container/SimpleContainer.phptests/Unit/SimpleContainerTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- phpstan.neon
- tests/Unit/SimpleContainerTest.php
Comment on lines
+49
to
+106
| public function get(string $id): mixed | ||
| { | ||
| $resolved = $this->resolveAlias($id, true); | ||
| Assert::string($resolved); | ||
|
|
||
| if (array_key_exists($resolved, $this->services)) { | ||
| return $this->services[$resolved]; | ||
| } | ||
|
|
||
| if (!isset($this->factories[$resolved])) { | ||
| throw new NotFoundException("Service '{$id}' not found in container."); | ||
| } | ||
|
|
||
| $factory = $this->factories[$resolved]; | ||
|
|
||
| if (is_string($factory) && class_exists($factory)) { | ||
| $factory = new $factory(); | ||
| } | ||
|
|
||
| if (!is_callable($factory)) { | ||
| throw new ContainerException("Factory for '{$resolved}' is not callable."); | ||
| } | ||
|
|
||
| $reflection = match (true) { | ||
| $factory instanceof Closure => new ReflectionFunction($factory), | ||
| is_string($factory) && !str_contains($factory, '::') => new ReflectionFunction($factory), | ||
| is_string($factory) && str_contains($factory, '::') => new ReflectionMethod($factory), | ||
| is_array($factory) => new ReflectionMethod($factory[0], $factory[1]), | ||
| is_object($factory) => new ReflectionMethod($factory, '__invoke'), | ||
| default => throw new ContainerException("Unsupported factory type for '{$resolved}'."), | ||
| }; | ||
|
|
||
| $params = $reflection->getNumberOfParameters(); | ||
| $instance = match (true) { | ||
| 0 === $params => $factory(), | ||
| 1 === $params => $factory($this), | ||
| default => $factory($this, $resolved), | ||
| }; | ||
|
|
||
| if (isset($this->delegators[$resolved])) { | ||
| foreach ($this->delegators[$resolved] as $delegator) { | ||
| $current = $instance; | ||
|
|
||
| if (is_string($delegator) && class_exists($delegator)) { | ||
| $delegator = new $delegator(); | ||
| } | ||
|
|
||
| if (!is_callable($delegator)) { | ||
| throw new ContainerException("Delegator for '{$resolved}' is not callable."); | ||
| } | ||
|
|
||
| $instance = $delegator($this, $resolved, static fn (): mixed => $current); | ||
| } | ||
| } | ||
|
|
||
| $this->services[$resolved] = $instance; | ||
|
|
||
| return $instance; |
There was a problem hiding this comment.
Detect circular service resolution before invoking nested get() calls.
Alias loops are handled, but factory/delegator loops are not. If service A resolves B and B resolves A, this path recurses until PHP blows the stack or memory instead of failing with a ContainerException.
Suggested fix
final class SimpleContainer implements ContainerInterface
{
/** `@var` array<string, mixed> */
private array $services = [];
+
+ /** `@var` array<string, true> */
+ private array $resolving = [];
@@
public function get(string $id): mixed
{
$resolved = $this->resolveAlias($id, true);
Assert::string($resolved);
+
+ if (isset($this->resolving[$resolved])) {
+ throw new ContainerException("Circular service dependency detected for '{$resolved}'.");
+ }
if (array_key_exists($resolved, $this->services)) {
return $this->services[$resolved];
}
@@
- $factory = $this->factories[$resolved];
+ $this->resolving[$resolved] = true;
+
+ try {
+ $factory = $this->factories[$resolved];
- if (is_string($factory) && class_exists($factory)) {
- $factory = new $factory();
- }
+ if (is_string($factory) && class_exists($factory)) {
+ $factory = new $factory();
+ }
@@
- if (isset($this->delegators[$resolved])) {
- foreach ($this->delegators[$resolved] as $delegator) {
- $current = $instance;
+ if (isset($this->delegators[$resolved])) {
+ foreach ($this->delegators[$resolved] as $delegator) {
+ $current = $instance;
@@
- $instance = $delegator($this, $resolved, static fn (): mixed => $current);
+ $instance = $delegator($this, $resolved, static fn (): mixed => $current);
+ }
}
- }
- $this->services[$resolved] = $instance;
+ $this->services[$resolved] = $instance;
- return $instance;
+ return $instance;
+ } finally {
+ unset($this->resolving[$resolved]);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public function get(string $id): mixed | |
| { | |
| $resolved = $this->resolveAlias($id, true); | |
| Assert::string($resolved); | |
| if (array_key_exists($resolved, $this->services)) { | |
| return $this->services[$resolved]; | |
| } | |
| if (!isset($this->factories[$resolved])) { | |
| throw new NotFoundException("Service '{$id}' not found in container."); | |
| } | |
| $factory = $this->factories[$resolved]; | |
| if (is_string($factory) && class_exists($factory)) { | |
| $factory = new $factory(); | |
| } | |
| if (!is_callable($factory)) { | |
| throw new ContainerException("Factory for '{$resolved}' is not callable."); | |
| } | |
| $reflection = match (true) { | |
| $factory instanceof Closure => new ReflectionFunction($factory), | |
| is_string($factory) && !str_contains($factory, '::') => new ReflectionFunction($factory), | |
| is_string($factory) && str_contains($factory, '::') => new ReflectionMethod($factory), | |
| is_array($factory) => new ReflectionMethod($factory[0], $factory[1]), | |
| is_object($factory) => new ReflectionMethod($factory, '__invoke'), | |
| default => throw new ContainerException("Unsupported factory type for '{$resolved}'."), | |
| }; | |
| $params = $reflection->getNumberOfParameters(); | |
| $instance = match (true) { | |
| 0 === $params => $factory(), | |
| 1 === $params => $factory($this), | |
| default => $factory($this, $resolved), | |
| }; | |
| if (isset($this->delegators[$resolved])) { | |
| foreach ($this->delegators[$resolved] as $delegator) { | |
| $current = $instance; | |
| if (is_string($delegator) && class_exists($delegator)) { | |
| $delegator = new $delegator(); | |
| } | |
| if (!is_callable($delegator)) { | |
| throw new ContainerException("Delegator for '{$resolved}' is not callable."); | |
| } | |
| $instance = $delegator($this, $resolved, static fn (): mixed => $current); | |
| } | |
| } | |
| $this->services[$resolved] = $instance; | |
| return $instance; | |
| /** `@var` array<string, mixed> */ | |
| private array $services = []; | |
| /** `@var` array<string, true> */ | |
| private array $resolving = []; | |
| public function get(string $id): mixed | |
| { | |
| $resolved = $this->resolveAlias($id, true); | |
| Assert::string($resolved); | |
| if (isset($this->resolving[$resolved])) { | |
| throw new ContainerException("Circular service dependency detected for '{$resolved}'."); | |
| } | |
| if (array_key_exists($resolved, $this->services)) { | |
| return $this->services[$resolved]; | |
| } | |
| if (!isset($this->factories[$resolved])) { | |
| throw new NotFoundException("Service '{$id}' not found in container."); | |
| } | |
| $this->resolving[$resolved] = true; | |
| try { | |
| $factory = $this->factories[$resolved]; | |
| if (is_string($factory) && class_exists($factory)) { | |
| $factory = new $factory(); | |
| } | |
| if (!is_callable($factory)) { | |
| throw new ContainerException("Factory for '{$resolved}' is not callable."); | |
| } | |
| $reflection = match (true) { | |
| $factory instanceof Closure => new ReflectionFunction($factory), | |
| is_string($factory) && !str_contains($factory, '::') => new ReflectionFunction($factory), | |
| is_string($factory) && str_contains($factory, '::') => new ReflectionMethod($factory), | |
| is_array($factory) => new ReflectionMethod($factory[0], $factory[1]), | |
| is_object($factory) => new ReflectionMethod($factory, '__invoke'), | |
| default => throw new ContainerException("Unsupported factory type for '{$resolved}'."), | |
| }; | |
| $params = $reflection->getNumberOfParameters(); | |
| $instance = match (true) { | |
| 0 === $params => $factory(), | |
| 1 === $params => $factory($this), | |
| default => $factory($this, $resolved), | |
| }; | |
| if (isset($this->delegators[$resolved])) { | |
| foreach ($this->delegators[$resolved] as $delegator) { | |
| $current = $instance; | |
| if (is_string($delegator) && class_exists($delegator)) { | |
| $delegator = new $delegator(); | |
| } | |
| if (!is_callable($delegator)) { | |
| throw new ContainerException("Delegator for '{$resolved}' is not callable."); | |
| } | |
| $instance = $delegator($this, $resolved, static fn (): mixed => $current); | |
| } | |
| } | |
| $this->services[$resolved] = $instance; | |
| return $instance; | |
| } finally { | |
| unset($this->resolving[$resolved]); | |
| } | |
| } |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/Container/SimpleContainer.php (2)
49-106:⚠️ Potential issue | 🟠 MajorDetect circular service resolution before invoking nested
get()calls.Alias loops are handled by
resolveAlias(), but factory/delegator loops are not. If service A's factory resolves B and B's factory resolves A, the recursion continues until PHP exhausts the stack—noContainerExceptionis thrown.Suggested fix using a resolution guard
final class SimpleContainer implements ContainerInterface { /** `@var` array<string, mixed> */ private array $services = []; + /** `@var` array<string, true> */ + private array $resolving = []; + /** `@var` array<string, callable|string> */ private array $factories = [];Then in
get():public function get(string $id): mixed { $resolved = $this->resolveAlias($id, true); Assert::string($resolved); + if (isset($this->resolving[$resolved])) { + throw new ContainerException("Circular service dependency detected for '{$resolved}'."); + } + if (array_key_exists($resolved, $this->services)) { return $this->services[$resolved]; } if (!isset($this->factories[$resolved])) { throw new NotFoundException("Service '{$id}' not found in container."); } + $this->resolving[$resolved] = true; + + try { $factory = $this->factories[$resolved]; // ... factory invocation and delegator logic ... $this->services[$resolved] = $instance; return $instance; + } finally { + unset($this->resolving[$resolved]); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Container/SimpleContainer.php` around lines 49 - 106, The get() method needs a resolution guard to detect circular factory/delegator resolution: add a property (e.g., array $resolving) to track ids currently being constructed, check resolveAlias($id) against that guard at the start of get() and throw a ContainerException if it's already present, push the resolved id onto $resolving before invoking the factory or delegators, and remove it in a finally block so the entry is cleared even on errors; reference the get(), resolveAlias(), $this->factories, $this->delegators and $this->services symbols when implementing the guard and ensure nested get() calls will hit the guard and throw a clear ContainerException on cycles.
151-157:⚠️ Potential issue | 🟡 MinorDelegators accumulate across multiple
configure()calls and survive service re-registration.Unlike
setService/setFactory/setAliaswhich clear competing registrations, delegators are only appended. This means:
- Calling
configure()twice with the same delegator config duplicates delegator entries- Re-registering a service via
setFactory()still applies previously registered delegatorsIf this is intentional, consider documenting the behavior. Otherwise, consider clearing delegators when the target service is re-registered.
Optional: clear delegators on re-registration
public function setFactory(string $name, callable|string $factory): void { - unset($this->aliases[$name], $this->services[$name]); + unset($this->aliases[$name], $this->services[$name], $this->delegators[$name]); $this->factories[$name] = $factory; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Container/SimpleContainer.php` around lines 151 - 157, Delegators are being appended in configure() causing duplicates and persisting across re-registration; modify behavior so delegators do not accumulate unexpectedly by either resetting entries on configure() or clearing them when a service is re-registered. Specifically, update configure() (where $this->delegators is populated) to replace the delegators for a given service name instead of always appending (use direct assignment rather than $this->delegators[$name][]), or add a clearing step in setFactory(), setService(), and setAlias() to unset or empty $this->delegators[$name] when a service is re-registered; reference the methods configure(), setFactory(), setService(), setAlias() and the $delegators property to locate where to apply the change. Ensure tests cover calling configure() twice and re-registering a service to confirm no duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Container/SimpleContainer.php`:
- Around line 49-106: The get() method needs a resolution guard to detect
circular factory/delegator resolution: add a property (e.g., array $resolving)
to track ids currently being constructed, check resolveAlias($id) against that
guard at the start of get() and throw a ContainerException if it's already
present, push the resolved id onto $resolving before invoking the factory or
delegators, and remove it in a finally block so the entry is cleared even on
errors; reference the get(), resolveAlias(), $this->factories, $this->delegators
and $this->services symbols when implementing the guard and ensure nested get()
calls will hit the guard and throw a clear ContainerException on cycles.
- Around line 151-157: Delegators are being appended in configure() causing
duplicates and persisting across re-registration; modify behavior so delegators
do not accumulate unexpectedly by either resetting entries on configure() or
clearing them when a service is re-registered. Specifically, update configure()
(where $this->delegators is populated) to replace the delegators for a given
service name instead of always appending (use direct assignment rather than
$this->delegators[$name][]), or add a clearing step in setFactory(),
setService(), and setAlias() to unset or empty $this->delegators[$name] when a
service is re-registered; reference the methods configure(), setFactory(),
setService(), setAlias() and the $delegators property to locate where to apply
the change. Ensure tests cover calling configure() twice and re-registering a
service to confirm no duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e179e559-ef70-4458-88fe-59369b22d400
📒 Files selected for processing (2)
config/routes.phpsrc/Container/SimpleContainer.php
🚧 Files skipped from review as they are similar to previous changes (1)
- config/routes.php
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
1 participant
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
laminas/laminas-servicemanagerto avoid downstream dependency conflictssrc/Container/SimpleContainer.php— a minimal PSR-11 container supporting Mezzio's ConfigProvider format (factories, aliases, invokables, delegators)psr/containerconstraint to^1.1 || ^2.0(matching Symfony/Mezzio convention)slam/phpstan-laminas-frameworkdev dependency (tied to ServiceManager)Test plan
Summary by CodeRabbit
Chores
Bug Fixes
Tests