refactor(phpmd): larpingapp true findings 17 → 1, suppressions 25 → 1

[rubenvdlinde]

Result

	before	after
TRUE findings (every @SuppressWarnings stripped on a throwaway copy)	17	1
shipped findings	0	0
@SuppressWarnings tags in lib/	25	1
phpmd.baseline.xml present	no	no
unit tests	153 / 541 OK	153 / 541 OK

phpmd.xml, phpmd-unusedparams.xml, phpcs.xml and every phpunit config are byte-unchanged. No threshold raised, no rule removed, no baseline added, no assertion weakened.

The one surviving finding is the floor

Application::boot(IBootContext $context) — UnusedFormalParameter. The signature is mandated by OCP\AppFramework\Bootstrap\IBootstrap and the body is legitimately empty: register/schema initialisation moved to the InitializeRegister repair step. The parameter can be neither dropped nor used.

Clean differential as a positive control: TRUE = 1 finding / exit 2, SHIPPED = 0 findings / exit 0. The single surviving suppression demonstrably suppresses exactly one real finding, and the harness discriminates in both directions — so "0 shipped" is a measurement, not an artefact of a rule that never fired.

Suppressions adjudicated: 25 → 1

verdict	n	detail
DEAD — deleted	8	6× ShortVariable (EventsController ×3, CharactersController ×2, RegisterObjectFetcher ×1) on methods whose only short variable is $id, which phpmd.xml's ShortVariable exceptions allowlist already covers — they suppressed nothing. 1× UnusedFormalParameter on Application::register(), which uses $context. 1× NPathComplexity on the listener's handle(), which never fired.
FIXABLE — fixed, then deleted	16	see below
JUSTIFIED — kept	1	Application::boot()

All 25 originally carried a bare tag with no reason text. The survivor now carries a written one.

What the fixes actually were

• EventsController (7 findings: CouplingBetweenObjects, ExcessiveClassComplexity, 3× CyclomaticComplexity, 2× NPathComplexity) — extracted EventRosterService, which now owns the participation, attendance and run-sheet-context rules. The controller is a thin HTTP/auth boundary again. Also collapsed the authenticate-then-authorize pair into resolveGameMaster() returning [uid, refusal], and removed a duplicate of resolvePlayerName that had been inlined into the cast builder.
• SkillRequirementService (4) — extracted SkillRequirementChecker (the four requirement kinds, with requiredConditions/requiredEffects collapsed into one shared membership check) and IdListNormaliser, used by both. validate() went from ~117 lines to orchestration.
• CharacterService (1) — extracted EffectApplier, a stateless collaborator holding effect resolution, non-cumulative dedup and signed-modifier logic. Pure move; the arithmetic is unchanged.
• CharacterRequirementListener (2) — extracted extractEntities() (early-return, so the else disappears rather than relocating) and collectVeto(). Measured, not assumed — removing an else is not complexity-neutral, so both the ElseExpression and the CyclomaticComplexity were re-measured and both cleared.
• BooleanArgumentFlag ×2 — the rule fires on the parameter's default value, not the call site; that was confirmed before changing anything. Split into intention-revealing loadSettings() / reloadSettings() over a private importRegister(bool $force) with no default. Three call sites and the test updated.

Tests

153 tests / 541 assertions, OK — before and after, identically conditioned, re-run after every refactor round. Where a constructor gained a dependency the tests construct the real collaborator over the existing mock (e.g. new EventRosterService($this->objectFetcher)), so every assertion still exercises the same behaviour end to end. No assertion was weakened; the two loadSettings delegation tests gained an expects($this->never()) on the sibling method.

How it was measured

PHPMD 2.15.0 on PHP 8.4.22 inside nextcloud:latest, with composer install run in this worktree so nothing tests a shared checkout's lib/. Host PHP 8.2 kills vendor/bin/phpmd in platform_check.php with exit 255, which reads exactly like a clean run — so no measurement was taken on the host. Exit codes read directly, never through a pipe. Findings matched on normalised path:line:rule triples, because PHPMD right-pads file:line and raw line diffs are therefore wrong.

Also verified in the same rig: phpcs 0 errors, PHPStan [OK] No errors, php -l clean across lib/ and tests/.

Not done, and why

• SkillRequirementService::resolveXpAbility() is public with no caller. Left alone — it is documented as a shared resolution rule with event-xp-award-workflow and carries a @spec tag. Deleting a documented public API is not a debt burn-down.
• Psalm dies before analysis in a bare container on Cannot resolve stubfile path vendor/nextcloud/ocp/OCP.bak/…. Pre-existing and unrelated — psalm.xml is untouched here and that directory does not exist in a fresh clone. psalm.xml was deliberately not edited to make it pass; PHPStan covers the same ground and is clean.
• tests/integration/EffectChainIntegrationTest.php is run by neither phpunit.xml nor phpunit-unit.xml — both include only tests/unit. Its constructor call and import were updated so it stays consistent and lints, but flagging it: that file is currently dead weight in CI and deserves its own look.

🤖 Generated with Claude Code

[github-actions]

Quality Report — ConductionNL/larpingapp @ 910311e

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-specs		✅
test-l10n		❌
composer			✅	✅ 101/101
npm			✅	✅ 537/537
PHPUnit					✅
Newman					❌
Playwright					❌
Hydra gates					❌

---

Quality workflow — 2026-08-05 14:24 UTC

Download the full PDF report from the workflow artifacts.

[rubenvdlinde]

Follow-up commit: gate-28 fixed, and CharactersController deliberately taken back out of scope

gate-28 (license-triangle) went red and it was this PR that surfaced it. The gate is diff-scoped, so a stale header is only compared against composer.json once the file is edited. 11 touched files declared AGPL-3.0-or-later while every authoritative declaration says EUPL-1.2 — composer.json "license": "EUPL-1.2", appinfo/info.xml <licence>EUPL-1.2</licence>, and the LICENSE file itself ("EUROPEAN UNION PUBLIC LICENCE v. 1.2"). 19 files under lib/ already said EUPL-1.2; the AGPL headers are stale copy-paste from the upstream Nextcloud skeleton. Corrected in both docblock positions, for the touched files only. This is not a licence change — it makes the headers agree with the licence the project already grants under.

lib/Controller/CharactersController.php reverted to its development state — with the reasoning, because reverting deliberate work needs one

The only change there was deleting two dead @SuppressWarnings(PHPMD.ShortVariable) tags (dead because the sole short variable is $id, already in phpmd.xml's ShortVariable exceptions list). But editing the file at all pulled it into gate-49 (controller-exception-translation), which then reported downloadPdf() and requirementReport(): both call RegisterObjectFetcher::getObject() and catch only a broad \Exception, with no tracked-exception catch and no @throws.

That is a real pre-existing debt, and the honest fix was tried: split the catch into DoesNotExistException (client error → 404, silent) and \Exception (server-side, e.g. an unconfigured register → same 404 so nothing leaks, but logged, since today a broken install is indistinguishable from a bad id). It works, and gate-49 goes green.

It also costs a PHPMD finding — measured, not assumed. The use OCP\AppFramework\Db\DoesNotExistException; import takes CharactersController's CouplingBetweenObjects from 12 to 13, one over the threshold:

lib/Controller/CharactersController.php:59  CouplingBetweenObjects
  The class CharactersController has a coupling between objects value of 13.
  Consider to reduce the number of dependencies under 13.

That takes SHIPPED findings from 0 → 1. An FQCN in the catch instead of an import is not a way out either: phpmd.xml enables MissingImport.

Why the headroom is gone, and what the real fix is. downloadPdf() re-implements inline — via ContainerInterface + IAppManager + a copy of the UUID regex — exactly what Service\DocuDeskPdfRenderer already provides (isDocuDeskAvailable, normaliseTemplateId, getTemplate, render) and which EventsController already consumes. Consolidating onto it would drop two dependencies, delete the duplication and leave room for the exception import. But it rewrites ~60 lines plus the ContainerInterface mocks in CharactersControllerTest — that is its own change, not a rider on a PHPMD burn-down.

Rather than ship a net PHPMD regression to satisfy a different gate, this PR leaves the file alone. Net effect: TRUE stays at 1 and SHIPPED at 0 (the two reverted suppressions were dead, so removing them never moved either number), gate-49 returns to its development state, and gate-28 is satisfied for every file this PR does touch.

Re-verified after the revert, in nextcloud:latest / PHP 8.4.22: TRUE = 1, SHIPPED = 0, PHPUnit 153 / 541 OK, phpcs exit 0, PHPStan exit 0. Locally reproduced gate-28 and gate-49 over the final diff scope: both clean.

[github-actions]

Quality Report — ConductionNL/larpingapp @ 898fef0

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-specs		✅
test-l10n		❌
composer			✅	✅ 101/101
npm			✅	✅ 537/537
PHPUnit					✅
Newman					❌
Playwright					❌
Hydra gates					❌

---

Quality workflow — 2026-08-05 14:45 UTC

Download the full PDF report from the workflow artifacts.

[rubenvdlinde]

CI verdict, job by job — and the one red I have to account for myself

Every Hydra gate that ran now PASSES, including the two this PR turned red and then fixed:

[hydra-gates] RESULT: ALL GATES PASSED — EXCEPT GATES 4 24 33, WHICH DID NOT RUN.
[hydra-gates] GATES THAT DID NOT RUN: 4 24 33

The job is still red, but not because a gate failed — it is --require-full-coverage treating a gate that did not run as unverified. Breaking those three down:

• gates 24 and 33 did not run on development either (the base run says GATES THAT DID NOT RUN: 24 33) — pre-existing, structural.

• gate-4 (composer-audit) is diff-scoped: if [ "${SCOPE_TO_DIFF}" = "1" ]; then _in_scope "composer.json" || _in_scope "composer.lock" || _run_audit=0. This PR touches neither, so gate-4 has nothing in scope and emits nothing. It is unreachable for any PR to this repo that leaves the composer files alone — and manufacturing a composer.json edit purely to trip it would be gaming the gate, not satisfying it.

  So rather than leave that assurance missing, I made the measurement gate-4 would have made, in nextcloud:latest / PHP 8.4.22:

  $ composer audit --locked --format=plain
  No security vulnerability advisories found.
  AUDIT_EXIT=0
  

  That is a real audit of the lock, not the 2.7.x "No packages - skipping audit" fail-open the gate exists to catch — composer read the lock and reported on it.

The rest of the reds, all proven pre-existing on development

job	cause	red on development too?
lint-check	npm error 404 … @conduction/nextcloud-vue-3.0.0-vue3.6.tgz is not in this registry — dies at npm ci, before any lint runs	yes (environmental; the pin is unpublished, handled separately)
E2E Tests (Playwright)	same npm ci 404	yes
Frontend Check (test:l10n)	pre-existing	yes
Integration Tests (Newman)	pre-existing	yes
Quality Report	rollup of the above	yes

What passed

All four PHPUnit matrix jobs (PHP 8.3/8.4 × NC stable31/32) — pass. PHP Quality (phpmd), (phpcs), (psalm), (phpstan), (lint), (phpmetrics) — pass. Security (composer), Security (npm), License (composer), License (npm), Frontend Build, Frontend Tests (unit), Vue Quality (eslint), (stylelint), check:specs, CodeQL — pass.

Nothing red here was caused by this change.