Repository navigation
feat(engine:container): Container improvements from RFC-0006 - #77
Merged
Merged
Conversation
Added the InvalidResolutionException class with a static constructor for situations where a class is attempted to be resolved using both a name and qualifier. Implements: RFC-0006 Refs: #45
The equals method existed to compared qualified resolutions using qualifier specific logic. Qualifiers were never intended to be used that way, and were instead simply an attribute-based version of names. If values are required for resolution, resolvables should be used instead.
Add the InstanceCache class and replace the current arrays in Container, as well as their usage. Also flips the default 'shared' state of resolutions Implements: RFC-006 Refs: #45
A shared parent kept the liminal dependency alive, so the parent is bound not shared. The qualified test now asserts one instance per qualifier class, and the fixtures drop the removed equals() method. Implements: RFC-0006 Refs: #45
Removed the has method from InstanceCache as its never used, and it's unlikely to ever be needed. Should it be needed in the future, it can be added easily. Implements: RFC-0006 Refs: #45
Make the readonly property InstanceCache::$weak private. There was no reason for it to be public, and like InstanceCache::has() it's unlikely to be needed. Having it public just created another spot that needed coverage from unit tests and mutation testing.
Covers the strong and weak caches across the class, named and qualified dimensions, including weak references clearing once collected and being replaced on the next put. Implements: RFC-0006 Refs: #45
Covers named and qualified liminal resolutions being found on the second resolve, liminal and shared entries staying separate, a class with no binding being shared, and the resolution guard against both a name and a qualifier. Implements: RFC-0006 Refs: #45
The ignore named a line number that moved when the container shed its cache arrays, so the mutant escaped again. Matching the method stops it drifting.
Names and qualifiers exist to pick between bindings of the same abstract, so without a binding there's nothing to pick. Throw BindingNotFoundException instead of constructing the class. Also wraps any failure while resolving a parameter in a DependencyResolutionException that names the parameter and its function, with the original as the previous.
Reading the cache used the requested class while writing used the binding's abstract, so resolving a shared binding through an alias constructed a new instance every time. The class is now normalised once at the start of resolution and used for both, as well as for construction when the binding has no concrete class. Implements: RFC-0006 Refs: #44
Add ClassAttributeCache, which reads NoResolution, Lazy and Liminal from a class once, without instantiating any attributes, and answers from the stored flags after that. The container now checks both the requested class and the binding's concrete class, so a marker on an abstract applies too. A binding for a class that doesn't exist now throws, since the requested class is reflected. Implements: RFC-0006 Refs: #43
…ce cache Reading the cache only looked at the resolution's own liminal flag, while writing also used the binding and the Liminal attribute, so liminal bindings and Liminal classes were never found again. A factory binding for a Liminal class was cached strongly, because the attribute was only read when the container constructed the class. Liminality is now worked out once, from the resolution, its binding and the Liminal attribute on the requested or concrete class, and used for both. Also moves the binding lookup above the cache and lazy checks, so a lazy named or qualified resolution with no binding throws straight away, and a factory or instance binding for a class that doesn't exist now throws, since the requested class is reflected. Implements: RFC-0006 Refs: #46
A circular dependency used to recurse until PHP ran out of stack, with nothing to say which classes were involved. The container now keeps a stack of the resolutions it is constructing, identified by class and by name or qualifier, and throws CircularDependencyException with the chain when one is required again before it finishes. Also lets CircularDependencyException through the parameter and union wrappers, so it reaches the caller as itself. Implements: RFC-0006 Refs: #47
Bumps every locked package to its latest release within the existing constraints, including php-cs-fixer 3.95.27, PHPStan 2.3.1 and the Symfony 8.1 components. composer.json is unchanged.
The fixer config had global_namespace_import switched off entirely and risky rules disallowed, so neither imports nor strict type declarations were enforced. It now imports global classes, in code and PHPDoc, and adds declare(strict_types=1) to every file, and the 43 files that were missing one or both are fixed.
A binding marked lazy, or #[Lazy] on the requested or concrete class, now returns a proxy for the binding's concrete class. NoResolution and classes PHP cannot proxy now throw when the proxy is requested, and a class with nothing to defer is constructed instead. A proxy whose factory returns a different class now throws DependencyResolutionException on first use, instead of PHP's TypeError. Implements: RFC-0006 Refs: #76
resolve() now reads as the steps it runs: finding the binding, working out liminality and laziness, guarding NoResolution, and resolving fresh through build() and instantiate(). The duplicated NoResolution check is now one method. Also, a factory returning null now fails on the return type instead of falling through to construction, as RFC-0006 has a factory's return value be the result. Implements: RFC-0006
… them The circular dependency throw and the stack push now each time out in their own method, so the ignores name enterResolutionStack and resolveFresh, and the liminal default names isLiminal. Also drops the lazy() TrueValue ignore, which nothing needs any more.
GhostResolver looked up its binding directly, so a name or qualifier with no binding gave a ghost of the requested class instead of throwing BindingNotFoundException. findBinding() is now public and takes the class, name and qualifier, so the resolver goes through the same check. Implements: ADR-0029
invoke() had no docblock, and six private methods had tags but no summary. Also drops the comment in isLiminal() that repeated its summary.
RFC-0006 has put() return void, and nothing chained on the cache it returned. Also removes the test for the old return. Implements: RFC-0006 Refs: #45
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🔵 Needs a closer look
It is a large, behavior-changing rewrite of the core DI container spanning many files, which warrants final human review despite strong test coverage.
1 open finding
What changed in this PR
This PR implements RFC-0006 ("Container improvements") and ADR-0029, reworking the Engine\Container dependency-injection container. It replaces the container's four separate instance arrays with a unified InstanceCache (strong + weak), normalizes aliases before the cache is consulted, memoizes class-level NoResolution/Lazy/Liminal attributes, moves binding lookup ahead of the cache check to achieve liminal parity, adds circular-dependency detection, and brings lazy-proxy behavior in line with the RFC. It also enforces declare(strict_types=1) and global-namespace class imports via php-cs-fixer, clearing part of the #49 hygiene sweep.
Changes:
- Split
resolve()into one method per RFC-0006 resolution step and centralize cache selection/liminality so named, qualified, aliased, and liminal resolutions consistently share instances. - Add new types/behaviors:
InstanceCache,ClassAttributeCache,CircularDependencyException,InvalidResolutionException,InvalidAttributeException, plusDependencyResolutionException::parameter()/proxy(); names/qualifiers selecting no binding now throwBindingNotFoundException(ADR-0029), andGhostResolverroutes through the publicContainer::findBinding(). - Tooling/hygiene: enable
declare_strict_typesandglobal_namespace_import, update Infection ignores, and update dependencies within existing constraints.
| File | Description |
|---|---|
src/Container/Container.php |
Core refactor: step-per-method resolve(), binding-before-cache lookup, unified liminal/lazy handling, resolution stack for circular-dependency detection. |
src/Container/InstanceCache.php |
New strong/weak cache keyed by class/name/qualifier; replaces the four instance arrays (contains the flagged docblock typo). |
src/Container/ClassAttributeCache.php |
New memoized per-class lookup of NoResolution/Lazy/Liminal markers. |
src/Container/ReflectionHelper.php |
Adds class-attribute, lazy-instantiability, and function-name reflection helpers. |
src/Container/Resolution.php |
named()/qualifiedBy() now throw InvalidResolutionException when the other identifier is already set. |
src/Container/Exceptions/* |
New CircularDependencyException, InvalidResolutionException, InvalidAttributeException; new DependencyResolutionException factories. |
src/Container/Resolvers/GhostResolver.php |
Routes binding lookup through public Container::findBinding() for ADR-0029 parity. |
.php-cs-fixer.dist.php |
Enables declare_strict_types and global-namespace class imports. |
infection.json5 / composer.* |
Updated mutation-testing ignores and dependency constraints. |
tests/** + container fixtures |
Extensive new/updated unit tests and fixtures covering cache, liminal, lazy, circular-dependency, and ADR-0029 behavior; import cleanups (e.g. EnumColumnTest.php). |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
It said the cache holds resolved instances of caches, where it holds resolved instances of classes.
33 tasks
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.
Implements RFC-0006, the container improvements, and ADR-0029.
Design: RFC-0006, Container improvements: https://thegamepanel.archdoc.dev/rfc/0006 (source: https://github.com/thegamepanel/spec/blob/main/rfc/0006-container-improvements.md)
Design: ADR-0029, Require a binding for named and qualified resolutions: https://thegamepanel.archdoc.dev/adr/0029 (source: https://github.com/thegamepanel/spec/blob/main/adr/0029-selection-requires-binding.md)
Closes #45, closes #44, closes #43, closes #46, closes #47, closes #76.
Cards
InstanceCachereplaces the container's four instance arrays, with a strong and a weak cache keyed by class, name and qualifier class. Naming and qualifying one resolution throwsInvalidResolutionException, andQualifier::equals()is gone.NoResolution,LazyandLiminalare memoised per class as presence flags, read from the requested class and the binding's concrete class.CircularDependencyExceptionnaming the chain, instead of exhausting the stack.Lazyon either class, are made for the concrete class, and throw container exceptions where PHP would throw its own errors.Also on this branch
BindingNotFoundException, for resolutions and for ghost objects (ADR-0029).Container::findBinding()is public soGhostResolvergoes through the same check.resolve()is split into one method per step of RFC-0006's resolution steps.InstanceCache::put()returnsvoid, as the RFC declares.strict_typestask.Behaviour changes
RFC-0006's Backwards compatibility section lists what changes for code resolving through the container. Two changes on this branch are not in that list:
InvalidClassExceptionfor every binding type, including factory and instance bindings and liminal resolutions, because the requested class is always reflected for its class attributes.nullfails onContainer::build()'s return type, instead of the container constructing the class itself. RFC-0006 has a factory's return value be the result.spec/container.mdwill be updated to describe the container as it now is once this is merged.Verification
vendor/bin/phpunit --testsuite=Unit: 1080 tests, 1644 assertions, OKcomposer analyse: no errorsvendor/bin/php-cs-fixer fix --dry-run: 0 of 303 filesEngine\Containersrc/Containerwith the unit suite: 478 mutants, 476 killed, 1 caught by static analysis, 1 escaped. The escaped mutant removes the memoised return inResolverCatalogue, which this branch does not touch.