Allow recursive container calls from factory functions#301
Conversation
There was a problem hiding this comment.
Pull request overview
Enables factory functions in Container to make recursive container calls (e.g., getEnv() / getObject()) by temporarily removing the active factory entry during invocation, so recursion can fall back to global env or default autowiring instead of immediately recursing into the same factory.
Changes:
- Update
Container::loadObject()to temporarilyunset()the factory config entry while invoking factory closures. - Small internal refactor in
loadVariable()to inline param loading/invocation. - Update/add tests to cover recursive
getEnv()and recursivegetObject()behavior and adjust expected error messages.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Container.php |
Temporarily unsets factory entries during closure invocation to permit recursive lookups; minor invocation refactor in variable loading. |
tests/ContainerTest.php |
Removes old “recursive class” failure test and adds new test coverage for recursive getEnv()/getObject() patterns; updates an assertion message. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // invoke factory with list of parameters | ||
| $value = $params === [] ? ($this->container[$name])() : ($this->container[$name])(...$params); | ||
| // temporarily unset factory reference to allow loading recursive variables from environment | ||
| try { | ||
| unset($this->container[$name]); | ||
| $value = $factory(...$this->loadFunctionParams($closure, $depth, true, \explode("\0", $name)[0])); | ||
| } finally { |
There was a problem hiding this comment.
The comment says the factory is unset "to allow loading recursive variables from environment", but in this branch it's also enabling recursive getObject()/container lookups from within object factories. Updating the comment to reflect the broader intent would avoid confusion for future maintainers.
tests/ContainerTest.php
Outdated
| public function testGetObjectReturnDefaultStdclassInstanceWhenFactoryFunctionHasRecursiveArgument(): void | ||
| { | ||
| $container = new Container([ | ||
| \stdClass::class => function (\stdClass $object) { return $object; } | ||
| ]); | ||
|
|
There was a problem hiding this comment.
New test method name uses singular "Return" but the existing naming pattern in this file appears to be "testGetObjectReturns…". Renaming to "testGetObjectReturnsDefaultStdClassInstanceWhenFactoryFunctionHasRecursiveArgument" keeps test names consistent and easier to search/scan.
28ef07d to
61b9193
Compare
This changeset allows recursive container calls from factory functions for any variables or classes. On its own, this is rarely useful as the
Containerhas no public API exposed at the moment. This is mostly done as an internal preparation at the moment, but may be exposed as part of our public API that allows us to reuse this logic for more classes in a follow-up.Builds on top of #290, #289, #288, #96 and others