refactor(deps): inject OpenRegister instead of looking it up (ADR-083) - #515
Conversation
5 file(s) reached OpenRegister through $this->container->get(...) on an UNCONDITIONAL path — no availability check, no degrading catch. The dependency was announced nowhere: not in the constructor, not in the use block, not in any type. It appeared mid-method, as a string. Now constructor-injected and typed, so the dependency is visible to a reader and to tooling. Behaviour is unchanged: the same object, from the same container, resolved at construction instead of at first use. ContainerInterface is dropped only where nothing else used it. Deliberately NOT converted, because they are correct as written (ADR-083 rule 1's exception): lookups behind isInstalled()/getInstalledApps(), and lookups whose catch degrades rather than rethrows. Verified per file: php -l clean, and gate-66's lookup check reports zero remaining findings for each file changed. gate-66 for this app: 23 -> 8.
Quality Report — ConductionNL/softwarecatalog @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ❌ | ||||
| phpstan | ❌ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| check-vue-demi | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| composer | ✅ | ✅ 130/130 | |||
| npm | ✅ | ✅ 704/704 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-14 19:05 UTC
Download the full PDF report from the workflow artifacts.
|
Marked draft — not mergeable as it stands, same cause as decidesk/pipelinq/shillinq. Removing 7 test file(s) here construct the classes this PR changes and need updating to the new signature first. The |
Blocked on a missing prerequisite, not on this diffPiloting the test fixes surfaced why every rollout PR goes red the same way, and it is not the conversion: OpenRegister is not autoloadable in unit tests. This repo's Constructor-injecting the concrete Only So the recipe has a step nobody had written down
And step 1 is a decision, not a choreopenconnector's stub declares 10 of the real class's 59 methods. Vendoring seven more copies means seven doubles drifting away from a class none of them own — and a stale double on this fleet has already reported a security hole that did not exist. The alternative is OpenRegister publishing a contract (an interface, or a Left as a draft deliberately. |
5 file(s) reached OpenRegister through
$this->container->get(...)on an unconditional path — no availability check, no degrading catch. The dependency was announced nowhere: not in the constructor, not in theuseblock, not in any type. It appeared mid-method, as a string.Now constructor-injected and typed. Behaviour is unchanged — same object, same container, resolved at construction instead of at first use.
ContainerInterfaceis dropped only where nothing else used it.Deliberately not converted, because they are correct as written (ADR-083 rule 1's exception): lookups behind
isInstalled()/getInstalledApps(), and lookups whosecatchdegrades rather than rethrows. Converting those would make the service unconstructable without OpenRegister and turn a clean message into a 500.Verified per file:
php -lclean, and gate-66's lookup check reports zero remaining findings for each file changed.gate-66 for this app: 23 → 8.