09: Loader resolution and registration - #9
Open
nikolaystrikhar wants to merge 3 commits into
Open
Conversation
resolve() returned object and cached whatever the container handed back, so a mis-bound interface was stored and then failed on the accessor's return type -- a TypeError blaming this library rather than the binding. reset() threw the same one, so there was no way back out of it. It throws Config_Exception at the boundary now, before the memo. The per-accessor @var annotations are gone: verified against the pinned PHPStan 1.12 that an object return narrowing to an interface is unreported at level 5 and 6 either way, so they silenced nothing. resolve() carries @template T instead. The README pointed hosts at Loader::reset() to pick up a container set late, but reset() also drops every registration -- following that advice left nothing to load, silently. It now states the real rule: set the container before the first register() call.
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.
What:
Loader's collaborator resolution and registration — container-or-new, memoized — plusregister(),all(), andreset().boot()and the load loop land in the next PR.Stacked on #8.
Why this way: the container is never required. With none set every collaborator is a plain
new, so a host that does not use a container pays nothing and configures nothing. Resolution is memoized rather than re-looked-up, and deliberately does not swap a collaborator that arrives late — anything already holding the old instance would keep it, which is worse than not swapping.The review found two things worth fixing before merge.
A bad container binding was cached, and then there was no way back out.
resolve(): objectaccepted whatever the container returned and memoized it. The failure only surfaced one frame later, on the accessor's return type:TypeError: Loader::registrar(): Return value must be of type Registrar_Interface, stdClass returned— a message that blames this library rather than the binding. Worse, the bad instance was already in the memo, soLoader::reset()— which callsregistrar()— threw the same TypeError. The host could not reset out of it, and in a test suite every subsequenttearDownerrored. It now throwsConfig_Exceptionat the boundary, before the memo, naming the interface and what was actually bound. A test asserts you can still resolve afterwards.The README documented a recovery that silently destroyed the registry. It said a container set after the first resolve "has no effect until
Loader::reset()" — true, butreset()also empties every registration. A host following that advice (register(...)→set_container(...)→reset()→boot()) would get no exception and no notice; the bundled plugins would simply never load. The README now states the actual rule: set the container before your firstLoader::register().Also dropped the per-accessor
/** @var */annotations. Verified against the pinned PHPStan 1.12 that anobjectreturn narrowing to an interface goes unreported at level 5 and 6 with or without them — they were decorative, and the plan replicates them in three more accessors.resolve()carries@template Tinstead, so the accessors Tasks 10, 12 and 13 add are one-liners.Two things the review confirmed rather than changed, both worth recording: DI52's
has()falls back toclass_exists(), which is inert here only because every id this library resolves is an interface (class_exists()is false for interfaces; DI52 never callsinterface_exists()). That holds for all four planned collaborators. Andresolve()'snew $default_class()silently requires every default to be constructible with no arguments — verified true forNotices,Conflict\Resolver, andActivation, and now documented.The rebinding table in the README ships one row, not the plan's four: the other three interfaces do not exist until Tasks 10, 12 and 13, and the README is the published documentation at every intermediate tag.
Verify:
slic run unit— 99 tests, 144 assertions, green.composer test:analysis→[OK] No errors, exit 0.