refactor(deps): type-hint OpenRegister's published contract (ADR-084) - #519
Merged
rubenvdlinde merged 15 commits intoAug 16, 2026
Merged
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.
21 lib classes and 17 test classes take OCA\OpenRegister\Contract\ObjectServiceInterface instead of the concrete class, bound in the composition root, with hydra-gates bumped to v1.8.0 so composer installs the interface into vendor/. This is what makes the ADR-083 conversion mockable: a leaf app cannot load a class from another Nextcloud app, so a typed constructor parameter had no satisfiable double. Test doubles now mock the contract, which does load. Files naming ObjectService only as a CONTAINER KEY are untouched — those are availability-guarded lookups (the ADR-083 rule-1 exception), and the string must go on naming the concrete service because that is the key the alias resolves TO.
Contributor
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-15 07:53 UTC
Download the full PDF report from the workflow artifacts.
The first pass through this app used a transformer with three defects, each of which failed SILENTLY -- a skipped file is indistinguishable from a clean one. 1. It masked string literals with a REGEX. An apostrophe in a comment (`// King's Day.`) opened a string that did not close for 240 lines, so every type position between them looked quoted and the file was skipped. Now the comment and string ranges come from PHP's own token_get_all(). 2. It compared PHP's BYTE offsets against Python's CHARACTER indices. One file differed by 94 -- em dashes in prose comments -- so spans after the first non-ASCII byte were misaligned and a docblock was judged "not in a comment". Everything is handled as bytes now. 3. It missed short-form RETURN types (`): ?ObjectService`), which neither the parameter nor the fully-qualified pattern matched. And it dropped the concrete import even where `ObjectService::class` or `instanceof ObjectService` still needed it. That one is not merely incomplete, it is silent damage: `::class` does not require the class to exist, so the lookup would have resolved to this app's own namespace. An invariant check now enforces the rule -- a file may import the contract, or both, but never the contract alone while still naming bare ObjectService -- and reports 0 for every app in this rollout.
Contributor
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-15 08:06 UTC
Download the full PDF report from the workflow artifacts.
ADR-083 added a constructor parameter; the test constructions still passed the
old argument count:
ArgumentCountError: Too few arguments to __construct(),
N passed and exactly N+1 expected
Each site gains one argument BY NAME, which fills the right slot whether the
preceding arguments were written positionally or by name — so the same edit
works for both shapes, and a call short by more than this one parameter still
errors, correctly.
Every touched file is re-parsed with php -l and reverted on failure, and a
re-scan reports 0 remaining sites in each app.
phpstan caught a defect in the rollout transformer:
PHPDoc tag @var for property $objectService with type
OCA\OpenRegister\Service\ObjectServiceInterface is not subtype of native
type OCA\OpenRegister\Contract\ObjectServiceInterface
The docblock rewrite matched `@var \OCA\OpenRegister\Service\ObjectService` and
appended `Interface` to the CLASS name while leaving the NAMESPACE alone, so the
declared type named a class that does not exist. The native type next to it was
correct, which is why only phpstan noticed — PHP itself never reads the docblock,
and the tests pass either way.
That is the fifth silent failure from this transformer, and the same shape as
the others: it produced plausible output that no runtime check disagreed with.
…e-fix one
These branches were cut from ADR-083 branches predating the 2026-08-14
correction, so they carry an older code-quality.yml whose push trigger has no
refactor/** — which is this branch's prefix. development's version says why that
matters:
An ALLOW-LIST of branch prefixes is a gate with a hole in it, and the hole
is SILENT: a branch matching nothing gets no CI at all, and its last visible
status is whatever it inherited — indistinguishable, on every dashboard,
from a branch that passed.
Observed here: softwarecatalog#519 settled at FOUR checks (CodeQL and Analyze
only) and read as green, having previously reported 43. shillinq#556 did the
same at three, which is shillinq#557.
Takes merge-hygiene.yml with it, the companion added in the same change, which
runs the fast structural checks on ** so an unlisted prefix is not completely
unguarded.
This restores coverage via the PUSH path. It does not explain why the
pull_request runs stopped, which is tracked separately.
…isfy the contract
Three distinct pre-existing defects, all of which only became visible once the
tests could actually construct their subjects.
1. ARITY, completely this time. The first pass added `objectService` only,
because that is the parameter ADR-084 was about. ADR-083 added others in the
same commit — softwarecatalog's ContactpersonenController gained THREE
(objectService, magicMapper, organisationService) — so a call could be fixed
for one and still be short by two. Every required parameter is now supplied,
by NAME so it fills the right slot regardless of the existing arguments.
2. IMPORTS for the types those arguments name. `createMock(MagicMapper::class)`
without a `use` resolves the short name against the TEST's own namespace, and
`::class` does not require the class to exist — so it silently mocks a class
nobody declared. Same trap as the container-key strings in lib/.
3. The ObjectEntity STUB now implements ObjectEntityInterface. Once
ObjectServiceInterface is mocked its return types are enforced:
Method find may not return value of type MockObject_ObjectEntity, its
declared return type is "?OCA\OpenRegister\Contract\ObjectEntityInterface"
A hand-rolled double that does not declare the interface cannot be handed
back. This is ADR-084's argument arriving in the tests: ten apps had such a
double, and none of them was checked against anything until now.
softwarecatalog also gains a MagicMapper stub, because ADR-083 injected
OpenRegister's MagicMapper into a controller and this app has no way to load it.
That stub is debt of exactly the kind ADR-084 removed for ObjectService — noted
in the file so it stays visible rather than becoming furniture.
php -l on every touched file, reverted on failure.
…Db layer
ADR-083 injected OCA\OpenRegister\Db\MagicMapper into three classes here. That
is another app's DATABASE layer — the coupling ADR-022 exists to prevent — and
no leaf app can load it, so its tests could not construct their own subjects. I
had added a hand-rolled MagicMapper stub to get past that; this removes the need
for one instead.
All three sites were doing the same thing: save without schema validation. The
published contract already exposes that, and saveObject() is not a lesser
route — OpenRegister's own SaveObject calls
metaHydrationHandler->hydrateObjectMetadata(entity:, schema:)
objectEntityMapper->update(entity:, register:, schema:)
which IS the magic-mapper route, with the metadata hydration these callers were
performing by hand. The comment claiming a plain save touches "just the blob
table" was wrong; the code was reimplementing OpenRegister's save pipeline one
layer too deep.
The flags matter, and one of them nearly went missing. FIX #434 chose
MagicMapper for TWO reasons, not one:
to avoid validation errors on the organisatie field (stored as UUID string
but Schema expects object type) AND to avoid triggering ObjectUpdatedEvent
cascades. That could interfere with the ongoing org activation process.
`_validation: false` covers the first. The second needs `silent: true`, and a
replacement carrying only the validation flag would have re-emitted those events
into an in-flight org activation — a behaviour change with no test to catch it.
Both flags are now passed at every site.
Left alone: ContactpersoonService's two `container->get(MagicMapper)` lookups.
They are lazy, not injected, so they neither block tests nor need a stub; they
carry the same FIX #434 reasoning and are worth converting on their own terms.
The previous commit left these deliberately: they are lazy container lookups rather than injected dependencies, so they blocked no test and needed no stub. They carry the same FIX #434 reasoning, and converting them needed reading what each actually does rather than pattern-matching the first one. Site 1 changes only the payload (username), so it is the same shape as the three already converted: _validation: false, silent: true. Site 2 is different, and this is why they were worth separating. It sets two pieces of entity METADATA that the payload API expresses differently: organisation travels in `@self`. SaveObject reads it and applies it via setOrganisation() BEHIND AN ACCESS CHECK — so an organisation the caller may not use is now refused rather than written. The direct mapper call bypassed that check entirely, which is a quiet authorisation improvement, not just a refactor. owner is not settable from the payload at all; SaveObject derives it from the acting user, so the UID is resolved to an IUser and passed as `currentUser`. Both keep _validation: false and silent: true, so neither the organisatie type mismatch nor the ObjectUpdatedEvent cascade behaviour changes. lib/ now has no code reference to OCA\OpenRegister\Db\MagicMapper. What remains is prose in RenameDutchCatalogColumns explaining how OpenRegister's schema sync adds columns, which is documentation of the other app's behaviour and correct where it is.
phpstan caught what the first pass missed. Access to an undefined property OrganizationSyncService::$magicMapper Five more `$this->magicMapper->update()` calls remained in that file; the first pass converted one. All five now save through the published contract, carrying the organisation in `@self` where the code had set it on the entity — which SaveObject applies behind an access check the direct mapper call skipped. Argument for parameter $silent has already been passed My own "add silent: true" pass matched a second time inside one already-edited block and produced a duplicate at broken indentation. Removed. TooFewArguments ... expecting objectService / organisationMapper to be passed The composition root builds OrganizationSyncService and GebruikSyncService BY HAND, so ADR-083's new constructor parameters never reached them. psalm found this; PHPUnit could not, because the tests construct these services directly and never go through Application.php. A registration that cannot build its service fails at runtime on first use, which is the failure mode ADR-083 rule 3 is about. Both now resolve ObjectServiceInterface through the alias registered in the same file, so the composition root consumes its own binding rather than naming the concrete class again.
phpmd, on this branch:
UndefinedVariable $objectService
ADR-083 replaced
$objectService = $this->container->get('OCA\OpenRegister\Service\ObjectService');
with an injected property and rewrote most usages to $this->objectService — but
not all. What remained read a local that no longer exists. In PHP that is not a
parse error and not a test failure unless the line executes; it is null at
runtime, and the call it feeds gets null instead of the service.
Rewritten only where the enclosing function has no assignment to that local AND
does not take it as a PARAMETER — several helpers legitimately receive it, e.g.
CreditLimitGuard::sumOutstandingCents(object $objectService, ...), and those are
untouched.
php -l on every touched file, reverted on failure.
Several classes here share a FILE NAME with another in a different namespace —
lib/Service/ChecklistService.php and lib/Service/Inspection/ChecklistService.php,
BelplanRoutingService, HearingService and others. The arity fixer keyed
constructors by that file name, so some constructions received arguments from
the wrong class:
Error: Unknown named parameter $settingsService
Classes are now resolved through the file's own `use` imports, and the pass
refuses to act unless it is certain: a constructor that exists but parses to
nothing is treated as a PARSE FAILURE and skipped, never as "takes no
arguments".
It also reads only DEPTH-0 named arguments. A nested construction has its own
constructor:
new ProcestToolProvider(
caseReader: new ProcestCaseReader(
settingsService: $settingsService, <-- the INNER call's parameter
logger: $logger,
Judging those against the outer constructor is what made the first attempt
delete valid arguments; that attempt was reverted, and this is its replacement.
phpstan, and by volume this was the bulk of what remained:
138x PHPDoc tag @PARAM references unknown parameter: $container (shillinq)
62x (pipelinq)
37x (decidesk)
ADR-083 removed the ContainerInterface parameter from the classes it converted
and left the @PARAM line above it. Removed only where the documented signature
genuinely has no $container — classes still using the availability-guarded
lookup keep both, verified on three of them (signature present, docblock intact).
Also:
Dead catch - Throwable is never thrown in the try block
getObjectService() became a property read, which throws nothing.
Expression "$this->objectService" on a separate line does not do anything
the old `$objectService = $container->get(...)` line survived as a bare
expression after its right-hand side was removed.
Not touched here: `is_array()` on an ObjectEntityInterface, which phpstan says
always evaluates to false. That is 45 sites in shillinq alone and the correct
fix differs per branch — the array arm is dead now that find() returns an
entity, but what the surviving arm should read is a per-site question.
…ass shadowed
The conflict was in resolveContactOrganisation(), and development was right on
both counts:
* the method TAKES $objectService as a parameter. My dangling-reference pass
rewrote it to $this->objectService — it was meant to skip functions that
receive the value, and here the skip did not fire.
* the schema slug was renamed contactpersoon -> contactPerson on development.
A sweep for the same mistake found one more, in
UserProfileUpdatedEventListener::findContactPerson(), where a parameter was
likewise being shadowed by a property read. Both restored.
Neither would have failed loudly: the property exists and holds the same
service, so the code works — until a caller passes a DIFFERENT object service to
these helpers, which is the only reason they take one.
Contributor
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-15 22:35 UTC
Download the full PDF report from the workflow artifacts.
findAll() is (array $config, bool $_rbac, bool $_multitenancy). These two calls passed the register id and schema id positionally, so they landed on the two booleans: the query ran unscoped across every register, with $_rbac set to a register id and $_multitenancy to a schema id -- both truthy, so nothing failed loudly and the wrong rows came back. The sibling call 180 lines above already does it correctly, with register and schema inside 'filters'. Matched that shape. Verified the contract's signature against the real ObjectService on openregister@development before changing the callers: they agree exactly, so the callers were the wrong side. Pre-existing -- the untyped container lookup meant no analyser could compare them.
Contributor
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-16 01:21 UTC
Download the full PDF report from the workflow artifacts.
This was referenced Aug 16, 2026
rubenvdlinde
added a commit
that referenced
this pull request
Aug 16, 2026
…ying (#531) * fix(me): pass the two ADR-084 arguments the /me factory stopped supplying `GET /api/softwarecatalog/api/me` returns 500 for every user on `development` right now: Too few arguments to function OCA\SoftwareCatalog\Controller\ContactpersonenController::__construct(), 11 passed in .../lib/AppInfo/Application.php on line 730 and exactly 13 expected ADR-084 (#519) added `$objectService` and `$organisationService` to `ContactpersonenController::__construct()`. That controller is not autowired: `Application.php` registers it with a hand-written factory closure, added deliberately "for /me endpoint", and a hand-written factory does not gain a constructor argument automatically. Nothing we run catches that. `php -l` passes, phpcs/phpmd/psalm/phpstan pass, and the controller's own unit tests pass because they construct the controller themselves and never go through the factory. The first thing that notices is a request. It was found by an end-to-end run, as the only 5xx URL in 176 requests — and it accounts for 42 of that suite's 47 failures. The fix supplies both arguments from the same container entries the file already uses twice (`ObjectServiceInterface::class` via the alias registered at the top of `register()`, and OpenRegister's `OrganisationService`). `tests/Unit/AppInfo/CompositionRootArgumentsTest.php` closes the hole for every hand-written factory in the composition root, not just this one. It reads `Application.php`'s own source with PHP's tokeniser, extracts every `new <Class>(...)`, and reflects the target constructor. It refuses to report a vacuous pass: it asserts a floor on the number of call sites parsed and on the number of classes reflected, and it keeps the set of targets it cannot reflect explicit, because unreflectable is "unchecked", not "clean". * test(composition-root): resolve an unqualified class name against the file's own namespace The first version of the parser resolved `use ... as Alias` after taking the short name, and did not resolve an unqualified name against the file's own namespace at all. Both mistakes push a call site into the "could not reflect" bucket, and unreflectable reads as unchecked, which reads as clean. With both fixed, the set of factory targets this test cannot reflect in softwarecatalog is EMPTY — all 40 call sites in the composition root are actually checked — so the declaration is now asserted exactly rather than as a subset. * test(stubs): give the ObjectEntity stub the return types its contract declares The unit suite does not run on `development`. It dies with Fatal error: Declaration of OCA\OpenRegister\Db\ObjectEntity::getUuid() must be compatible with OCA\OpenRegister\Contract\ObjectEntityInterface::getUuid(): ?string in tests/Stubs/Db/ObjectEntity.php on line 151 ADR-084 made the stub `implements ObjectEntityInterface`, but its five abstract declarations kept their untyped, docblock-only signatures. PHP refuses to declare a class whose abstract method is less specific than the interface it satisfies, so the failure is a class-declaration fatal, not a test failure: it aborts the run rather than reporting anything. Under `tests/bootstrap.php` — the config CI uses — every stub is `require_once`d at bootstrap, so the suite dies before test one and the job reports zero tests. One anonymous subclass (MergeOrganisatieServiceTest) needed the same four return types for the same reason. `getId()` and `setObject()` are deliberately left untyped: neither is on the contract, so nothing constrains them. This does not fix a single test. It makes the suite measurable: before: 50 tests, then a fatal (0 under tests/bootstrap.php) after: Tests: 702, Assertions: 2656, Errors: 50, Failures: 12, Warnings: 3, Deprecations: 1, Skipped: 25 The 62 problems now visible are the rest of the ADR-084 fallout and are NOT addressed here — 38 are test helpers still declaring a return type of the concrete `OCA\OpenRegister\Service\ObjectService` while the code they feed now takes `ObjectServiceInterface`, and 8 are the deleted `$container` constructor parameter shifting every positional argument after it. They need their own change. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
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
1 participant
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
Services take
OCA\OpenRegister\Contract\ObjectServiceInterfaceinstead of theconcrete class, bound in the composition root, with
conduction/hydra-gatesbumped to v1.8.0 so composer installs the interface into
vendor/.Why this is what unblocks ADR-083
A leaf app cannot load a class from another Nextcloud app, so a typed
constructor parameter had no satisfiable double:
All seven ADR-083 rollout PRs stopped there. Measured across the fleet, ten of
sixteen consumers had hand-rolled a double rather than solve it — 0–13 methods
each against a real class of 88.
ObjectServiceInterfaceis 25 methods, scoped by measuring per CLASS rather thanper call: a class can only type-hint the interface if every method it calls is
on it. Per call, eight methods look sufficient; per class they cover 58% and
strand 414 consumer classes.
What is deliberately NOT touched
Files naming
'OCA\OpenRegister\Service\ObjectService'only as a containerkey. Those are availability-guarded lookups — the ADR-083 rule-1 exception —
and the string must go on naming the concrete service, because that is the key
the alias resolves to. Rewriting it would make the container look up something
nobody registered.
The binding
registerServiceAlias, not a factory: it resolves when something asks for theinterface, so an instance without OpenRegister fails at the route that needed the
data rather than at registration. Both names are strings and neither triggers an
autoload — which is what keeps ADR-083 rule 3's promise that the start screen
still boots.
On the transformer
The first version masked string literals with a regex, and it silently
under-converted: in one file the comment
// King's Day.opened a string thatdid not close until
date('N')240 lines later, so every type position betweenthem looked quoted and the file was skipped. A skipped file is
indistinguishable from a clean one — it took comparing counts to notice.
It now takes comment and string ranges from PHP's own
token_get_all(), lintsevery file with
php -land reverts on failure, and reports every file itdeclines to convert together with the reason.
Related
implements— feat(contract): publish ObjectService/ObjectEntity interfaces — one definition instead of eight openregister#2498 (merged)🤖 Generated with Claude Code