Skip to content

fix(gate-57): delete six orphaned write capabilities - #530

Merged
rubenvdlinde merged 2 commits into
developmentfrom
S8/gate-57-softwarecatalog
Aug 16, 2026
Merged

fix(gate-57): delete six orphaned write capabilities#530
rubenvdlinde merged 2 commits into
developmentfrom
S8/gate-57-softwarecatalog

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

gate-57 orphaned-write-capability — softwarecatalog

Workstream S8 of the fleet debt programme. Base at branch point 0335cb20, which is the head of run 3193731427913 failing jobs (psalm, phpmd, phpcs, phpstan, 6× PHPUnit, E2E, Hydra Gates, Quality Report) from the refactor/adr-084-type-hint-the-contract merge at 08:46:30. That is slot R1's scope, not this PR's. Please judge this PR at parity with that run, job by job.

Gate evidence (L1 / L3 / L4)

python3 <hydra-gates>/scripts/lib/check_orphaned_write_capability.py <files> > out.txt

File set reproduced from run-hydra-gates.sh: git ls-files -- lib/Service, .php, minus tests/. Output captured to a file; never piped into grep.

findings files measured helper exit
before 6 55 0
after 0 53 0

The file count drops by 2 because two dead service classes are deleted. Helper identity line: app_id=softwarecatalog source=appinfo/info.xml foundation=no.

⚠️ softwarecatalog does not vendor conduction/hydra-gates, so the helper and the phpcs/phpmd rulesets were mounted in from docudesk's vendored copy and run on softwarecatalog's own paths (the helper derives each app root from the file's own path, never from cwd).

Per-finding judgement — 6 findings, 6 deletes, 0 wires

# finding judgement reasoning
1 AangebodenGebruik/GebruikStatusHandler.php:66 updateStatus DELETE (whole class) See below.
2 SoftwareCatalogContactSyncService.php:133 importContact DELETE Validated that a UID resolved, then returned that same UID unchanged — no import, no write. The two live consumers, BackgroundJob\OrganizationContactSyncJob and Repair\MigrateContactsToNc, use isAvailable() / findContactByUid() / findContactForRecord() / syncToContacts(), which already serve resolve-or-create. Spec REQ-SCNC-003 updated so it no longer names a method the service does not have.
3 SoftwareCatalogue/ContactPersonHandler.php:964 updateUserGroupsFromRoles DELETE Already @deprecated; logged "updateUserGroupsFromRoles is deprecated - role assignment now based on organization type" on every call; ignored both $newRoles and $oldRoles; forwarded to updateUserGroupsFromContactData(), the live method. No caller — so the backward compatibility it existed for had no consumer either. Group membership is a permission fact; a second, role-shaped entry point into it is a surface, not a convenience.
4 SoftwareCatalogueService.php:851 sendGebruikerWelcomeEmail DELETE Body was one logger->info('Sending gebruiker welcome email'). It never sent anything.
5 SoftwareCatalogueService.php:969 syncUserWithRevertedContact DELETE Same shape — one logger->info().
6 SoftwareCatalogueService.php:988 updateUserFromRevertedGebruiker DELETE Same shape — one logger->info().

1 — GebruikStatusHandler. The class docblock says "AangebodenGebruikService delegates all updateStatus() logic here so that its own methods stay below ExcessiveMethodLength." AangebodenGebruikService has no such method, never constructs the handler, and nothing else references the class — the method-decomposition extraction landed and its caller was never updated. Deleted with the class, and with StatusTransitionValidator, whose only consumer it was. Neither is DI-registered and neither has tests.

4/5/6 — why deleting a log line is the right remedy. These three name capabilities the app does not have: send a welcome mail; reconcile the Nextcloud user after an object revert. Wiring a method whose entire body is a log line would have turned "this capability is missing" into "this capability is called", which is the invisible pass the programme forbids. Removing them leaves the gap visible; both are on the fleet board as capability gaps rather than dead code.

⚠️ A stale test found on the way, and corrected here

tests/Unit/EventListener/SoftwareCatalogEventListenerTest.php asserted ->expects($this->once()) on sendGebruikerWelcomeEmail, syncUserWithRevertedContact and updateUserFromRevertedGebruiker. SoftwareCatalogEventListener does not call any of them, and there is no code path in it that ever did — the tests described a wiring that does not exist.

They are corrected rather than deleted:

  • the gebruiker-created case keeps its real assertion (handleNewGebruiker is called) and drops the imaginary one;
  • the two revert cases are replaced by one case that asserts the true invariant — an ObjectRevertedEvent reaches the listener, runs, and does not reach a capability the app does not implement.

Worth flagging to whoever is looking at the 6 red PHPUnit cells on the branch point: these three expectations could not have been passing.

No gate-exclusion comment was used anywhere in this PR.

Quality (L10)

Run under PHP 8.3 in a php:8.3-cli container against this worktree (host PHP is 8.2):

  • lint — clean on every changed file
  • phpcs on the 3 changed lib/ files — 0 errors, 8 warnings (pre-existing). That includes two pre-existing phpcs errors fixed here (Inline comments must start with a capital letter, lines 1998 and 2090 of SoftwareCatalogueService.php), per the fix-what-you-touch rule; the file is now phpcs-clean.
  • phpmd on the same 3 files — clean, exit 0. This PR only deletes, so no complexity ceiling moves.
  • phpunit — the branch point already has 6 red cells from the adr-084 rollout. Please compare this PR's own CI to run 31937314279 job by job rather than to a green expectation.

gate-57 orphaned-write-capability reported 6 findings over 55 lib/Service
files. All six had zero callers; all six are deleted.

- AangebodenGebruik\GebruikStatusHandler::updateStatus — the whole class is
  unreferenced. Its docblock says "AangebodenGebruikService delegates all
  updateStatus() logic here"; AangebodenGebruikService has no such method and
  never constructs the handler, so the method-decomposition extraction landed
  without its caller ever being updated. Deleted with the class, and with
  StatusTransitionValidator, whose only consumer it was.
- SoftwareCatalogContactSyncService::importContact — validated a UID and
  returned it unchanged. The two live consumers (OrganizationContactSyncJob,
  Repair\MigrateContactsToNc) use isAvailable/findContactByUid/
  findContactForRecord/syncToContacts, which already serve resolve-or-create.
  Spec REQ-SCNC-003 updated: the service no longer names importContact.
- SoftwareCatalogue\ContactPersonHandler::updateUserGroupsFromRoles — already
  @deprecated, logged "deprecated - role assignment now based on organization
  type" on every call, ignored both role arguments and forwarded to
  updateUserGroupsFromContactData(). No caller, so its backward compatibility
  had no consumer. Group membership is a permission fact; a second entry point
  into it is a surface, not a convenience.
- SoftwareCatalogueService::sendGebruikerWelcomeEmail,
  ::syncUserWithRevertedContact and ::updateUserFromRevertedGebruiker — each
  was a single logger->info() and nothing else. They named capabilities
  (send a welcome mail, reconcile a user after an object revert) that have
  never been implemented; wiring a log line in would have hidden the gap.

⚠️ SoftwareCatalogEventListenerTest asserted that the listener calls
sendGebruikerWelcomeEmail, syncUserWithRevertedContact and
updateUserFromRevertedGebruiker exactly once each. It does not, and there is no
code path in SoftwareCatalogEventListener that ever did — the tests described a
wiring that does not exist. They are corrected here rather than deleted: the
revert case now asserts the true invariant (the revert path runs without
reaching a capability the app does not implement).

Before: 6 findings over 55 files. After: 0 findings over 53 files (two service
files fewer because the dead handler pair was deleted).
Two pre-existing phpcs errors ('Inline comments must start with a capital
letter') in a file this branch already touches, per the fix-what-you-touch
rule. lib/Service/SoftwareCatalogueService.php is now phpcs-clean (0 errors).
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/softwarecatalog @ 77908d2

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 10:53 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 1a1632a into development Aug 16, 2026
31 of 44 checks passed
@rubenvdlinde
rubenvdlinde deleted the S8/gate-57-softwarecatalog branch August 16, 2026 13:17
@rubenvdlinde
rubenvdlinde restored the S8/gate-57-softwarecatalog branch August 19, 2026 13:49
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