Skip to content

fix(me): pass the two ADR-084 arguments the /me factory stopped supplying - #531

Merged
rubenvdlinde merged 4 commits into
developmentfrom
R2/adr-084-composition-root
Aug 16, 2026
Merged

fix(me): pass the two ADR-084 arguments the /me factory stopped supplying#531
rubenvdlinde merged 4 commits into
developmentfrom
R2/adr-084-composition-root

Conversation

@rubenvdlinde

@rubenvdlinde rubenvdlinde commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

The defect

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 autowiredApplication.php:726 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 this. php -l passes. phpcs, phpmd, psalm and phpstan pass. The controller's own unit tests pass, because they build 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

Two arguments, resolved from the same container entries this file already uses twice — ObjectServiceInterface::class (via the alias registered at the top of register()) and OpenRegister's OrganisationService.

The test

tests/Unit/AppInfo/CompositionRootArgumentsTest.php closes the hole for every hand-written factory in the composition root, not only this one. It reads Application.php's own source with PHP's tokeniser (so comments and strings cannot be mistaken for code), extracts every new <Class>(...) call site, and compares it against the target constructor by reflection.

A unit test of the controller cannot catch a factory arity mismatch, so this test targets the composition root itself.

Shown to fail against the unfixed composition root (git stash of the one-hunk fix, same command, same container):

1) CompositionRootArgumentsTest::testMeEndpointControllerFactoryMatchesItsConstructor
GET /api/softwarecatalog/api/me resolves ContactpersonenController through the
hand-written factory in Application.php. It does not pass: $objectService,
$organisationService. Every call to /me will return 500 with "Too few arguments
to function ContactpersonenController::__construct()".

2) CompositionRootArgumentsTest::testEveryManualFactorySuppliesAllRequiredConstructorArguments
  - Application.php:730 new ContactpersonenController — required constructor
    parameter $objectService is not supplied (11 of 13 arguments given).
  - Application.php:730 new ContactpersonenController — required constructor
    parameter $organisationService is not supplied (11 of 13 arguments given).

Tests: 4, Assertions: 14, Failures: 2.

With the fix: OK (4 tests, 14 assertions).

It refuses to report a vacuous pass (a check that did not run looks exactly like one that passed):

  • testTheParserFindsTheFactoriesItIsSupposedToCheck asserts a floor of 30 parsed call sites and that ContactpersonenController is among them;
  • the main test asserts a floor of 10 reflected constructors;
  • testUnresolvableFactoryTargetsAreDeclared keeps the set of targets that cannot be reflected in a unit-test run explicit — an unloadable class is "I could not tell", not a pass, and a new one appearing there is a factory this file is not checking.

Before / after

measurement before after
composition-root argument mismatches in lib/AppInfo/Application.php 2 0
new <Class>(...) call sites scanned, both sides 40 40
CompositionRootArgumentsTest 2 failures 4 tests, 14 assertions, OK

Instrument: a static composition-root checker (call sites from Application.php, constructors by parsing the repo's own lib/ tree), output redirected to a file and line-counted — never piped, so no exit code is read off the last stage of a pipe. Run identically on both sides; the only difference is the one hunk.

Quality legs, on the changed files

  • phpcs --standard=phpcs.xmllib/AppInfo/Application.php clean. (phpcs.xml scopes lib only; tests/ is outside it.)
  • phpstan analyse lib/AppInfo/Application.php tests/Unit/AppInfo/CompositionRootArgumentsTest.php[OK] No errors, 2 files. Positive control in the same container: the same command on lib/Controller/ContactpersonenController.php + lib/Service/OrganizationSyncService.php reports 12 errors, so the instrument can say no here.
  • php -l clean on both files.

Container: hermiq-llm-runner:gates (PHP 8.3.33) — the host is PHP 8.2, and php:8.3-cli ships neither python3 nor git.

Parity

Base 7cf3afa6. This PR does not touch any file involved in the inherited ADR-084 red set (4 PHP Quality legs + 6 PHPUnit cells) other than adding two arguments to one factory, which strictly reduces the failure surface. E2E on this base is separately blocked by the seed defect in #529.

Not done here, deliberately

The remaining ADR-084 fallout in this repo (~37 phpstan / 6 psalm / 3 phpmd findings and the 6 PHPUnit cells) is service-layer and call-site work, and it is a different change from the composition root. Keeping this PR to the runtime fatal makes it safe to merge immediately, which is the point — it is live right now.


Rebased onto development after #528 — and the stub work here is now theirs, deliberately

While this PR was open, #528 (fix/stub-return-types-match-the-contract) landed on development. It fixes tests/Stubs/Db/ObjectEntity.php with the same four return types and the same reasoning I had used here, and it goes further — it also repairs the test helpers and the two services whose failures the class-load fatal was hiding.

origin/development is merged in. The one conflict, in tests/Stubs/Db/ObjectEntity.php, is resolved wholesale in favour of development:

tests/Unit/Service/MergeOrganisatieServiceTest.php auto-merged to development's content for the same reason. Both files are now byte-identical to origin/development — my contribution to them is zero, and that is the correct outcome.

What remains on this branch is two files, and #528 did not touch either:

lib/AppInfo/Application.php                       |   9 +-
tests/Unit/AppInfo/CompositionRootArgumentsTest.php | 561 +++++

The fatal is still on development — re-verified after the merge, not assumed

$ git show origin/development:lib/AppInfo/Application.php   # the /me factory, line 726-746
  12 named arguments counted in the block, of which the factory passes 11 to the constructor
$ check_composition_root.py <origin/development>
MISSING_REQUIRED lib/AppInfo/Application.php:730 new ContactpersonenController
  -> parameter $objectService (11/13 supplied)
MISSING_REQUIRED lib/AppInfo/Application.php:730 new ContactpersonenController
  -> parameter $organisationService (11/13 supplied)
SCANNED lib/AppInfo/Application.php calls=40

vs this branch: SCANNED … calls=40 and no findings. Same command, same 40 call sites, both sides.

The test still fails first — re-confirmed on the merged tree

A test that no longer fails first is not evidence, so the positive control was re-run after the merge (git checkout origin/development -- lib/AppInfo/Application.php on the merged worktree):

There were 2 failures:
  - Application.php:730 new ContactpersonenController — required constructor parameter
    $objectService is not supplied (11 of 13 arguments given).
  - … $organisationService …
  GET /api/softwarecatalog/api/me resolves ContactpersonenController through the
  hand-written factory in Application.php. It does not pass: $objectService,
  $organisationService.
Tests: 4, Assertions: 14, Failures: 2.

Restore the one hunk and it is OK (4 tests, 14 assertions).

Parity on the merged tree, measured on both sides

Unit suite (--testsuite "Unit Tests", tests/bootstrap-unit.php, hermiq-llm-runner:gates PHP 8.3.33), origin/development extracted read-only vs this branch, same vendor/:

tests errors failures warnings skipped
origin/development (89a7ea1b) 698 4 12 3 25
this branch (502acbdc) 702 4 12 3 25

Identical failure set. The four extra tests are this PR's, and all four pass.

  • phpstan analyse lib/AppInfo/Application.php tests/Unit/AppInfo/CompositionRootArgumentsTest.php[OK] No errors (2 files).
  • phpcs --standard=phpcs.xml lib/AppInfo/Application.phprc 0.

Conduction Release Bot added 3 commits August 16, 2026 13:25
…ying

`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".
… 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.
… 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.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/softwarecatalog @ c827368

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 11:41 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/softwarecatalog @ 054661c

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 12:03 UTC

Download the full PDF report from the workflow artifacts.

`development` gained #528 (`fix/stub-return-types-match-the-contract`)
while this branch was open. It fixes `tests/Stubs/Db/ObjectEntity.php`
with the same four return types and the same reasoning as this branch's
`b711474c`, and it goes further: it also repairs the test helpers and the
two services whose failures that class-load fatal was hiding.

Conflict in `tests/Stubs/Db/ObjectEntity.php` resolved by taking
`development`'s version wholesale. The two are equivalent in effect —
`getUuid(): ?string`, `getObject(): array`, `getRegister(): ?string`,
`getSchema(): ?string`, with `getId()` and `setObject()` left untyped
because neither is on the contract — and #528's comment sits better.
`tests/Unit/Service/MergeOrganisatieServiceTest.php` auto-merged to
`development`'s content for the same reason.

Both files are now byte-identical to `origin/development`. What remains
on this branch is only the composition root itself, which #528 did NOT
touch: `Application.php:730` still passes 11 arguments to a 13-parameter
constructor, so `/api/me` still returns 500 for every user.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/softwarecatalog @ a5dd5e2

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 12:16 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 57f6c7a into development Aug 16, 2026
31 of 44 checks passed
@rubenvdlinde
rubenvdlinde deleted the R2/adr-084-composition-root branch August 16, 2026 12:17
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