Skip to content

09: Loader resolution and registration - #9

Open
nikolaystrikhar wants to merge 3 commits into
08-registrarfrom
09-loader-resolve
Open

09: Loader resolution and registration#9
nikolaystrikhar wants to merge 3 commits into
08-registrarfrom
09-loader-resolve

Conversation

@nikolaystrikhar

Copy link
Copy Markdown
Contributor

What: Loader's collaborator resolution and registration — container-or-new, memoized — plus register(), all(), and reset(). 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(): object accepted 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, so Loader::reset() — which calls registrar() — threw the same TypeError. The host could not reset out of it, and in a test suite every subsequent tearDown errored. It now throws Config_Exception at 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, but reset() 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 first Loader::register().

Also dropped the per-accessor /** @var */ annotations. Verified against the pinned PHPStan 1.12 that an object return 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 T instead, 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 to class_exists(), which is inert here only because every id this library resolves is an interface (class_exists() is false for interfaces; DI52 never calls interface_exists()). That holds for all four planned collaborators. And resolve()'s new $default_class() silently requires every default to be constructible with no arguments — verified true for Notices, Conflict\Resolver, and Activation, 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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant