Skip to content

chore(phpmd): burn down 33 baselined findings (195 → 162) and fix the inert exclude-pattern - #129

Merged
rubenvdlinde merged 4 commits into
developmentfrom
chore/phpmd-baseline-burndown
Aug 5, 2026
Merged

chore(phpmd): burn down 33 baselined findings (195 → 162) and fix the inert exclude-pattern#129
rubenvdlinde merged 4 commits into
developmentfrom
chore/phpmd-baseline-burndown

Conversation

@rubenvdlinde

@rubenvdlinde rubenvdlinde commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Part of ConductionNL/.github#155 (fleet PHPMD suppression audit). Refs #124.

The measured position

count
composer phpmd reports today 0
with phpmd.baseline.xml deleted (suppressions intact) 195
true (baseline deleted + suppressions stripped) 211
after this PR (baseline deleted) 162

Measured with PHPMD 2.15.0 / PHP 8.4.22 in a container, exit code read directly (never through a pipe), --exclude '*/Resources/template/*' on every leg so the file set never changes, and positive-controlled (a deliberate violation added to a throwaway copy with the baseline active was reported and exited 2).

A prior audit recorded 6 here; I measure 0. Noting the drift, not chasing it.

The gate is live — this is not cosmetic

.github/workflows/code-quality.yml calls the shared ConductionNL/.github quality.yml, where enable-phpmd defaults to true. openbuild does not override it, so the PHP Quality (phpmd) job runs on every push and PR. The baseline is hiding 195 findings from a check that really executes.

Two structural facts about the baseline file

  1. 174 entries suppress 195 findings. A PHPMD baseline entry is scoped to a (rule, file) pair — optionally a method — never to a line. A class-scoped entry is an open licence: every future violation of that rule in that file is silently suppressed too. The shortfall is granularity, not staleness, and "174 entries" is not "174 known problems".
  2. PHPMD auto-discovers phpmd.baseline.xml from the working directory. Removing --baseline-file from the composer script does nothing; only deleting the file disables it.

What this PR retires: 195 → 162

rule before after
ElseExpression 27 12
MissingImport 3 0
ShortVariable 9 1
LongVariable 5 1
UnusedFormalParameter 3 1
CyclomaticComplexity 41 40
  • UnusedFormalParameter — two were genuinely dead and are removed, call sites updated: AppOverrideService::userVersionSlug($appId) and AppRepoParser::decodeChannelEntry($path).
  • LongVariable — the two renamed constructor-promoted properties are also renamed at their named-argument call sites in the unit tests.
  • ElseExpression — behaviour-preserving throughout: initialise-then-override where both branches were pure, guard-and-return where they were not, continue inside loops, extractions where both arms had to write. No ternaries (Squiz.PHP.DisallowInlineIf).

Why this stops at 162 and not 149 — read this bit

I first got to 149. Getting there meant touching six more files, and that turned three pre-existing hydra-gate failures red — in code this PR did not write. The gates are FILE-scoped (_filter_grep_by_scope keys on the path), so any edit to a file puts all of its existing findings in scope. development is green only because those files are not in its diff.

The three, precisely diagnosed:

  • gate-2 forbidden-patterns — the deliberate, documented error_log() in AppInfo\Application::register(), which carries its own phpcs:ignore and a comment explaining no PSR logger exists that early. Removing an else necessarily moves those lines into the diff. The call must stay; the gate has no per-call waiver; so the file must stay untouched.
  • gate-46 spec-anchor-existence17 dead @spec targets across ExportsController, AutomationsController, AppInfo\Application, RuleActionDispatcher, AutomationCompilerService, TemplateSeedService. They point at openspec/changes/automation-designer/…, which was archived to openspec/changes/archive/2026-07-11-automation-designer/…, and at #req-autd-NNN anchors that exist in no version of the spec — canonical or archived. The canonical spec uses ### Requirement: <prose> headings. Repairing them means deciding, per tag, which Requirement each method now implements. I did not touch a single @spec line in this PR.
  • gate-49 controller-exception-translationAutomationsController::recompileAndRespond() lacks a try/catch or @throws.

I reverted the six files rather than guess 17 spec links. Guessing traceability encodes wrong links, which is worse than a dead one, and it would turn a PHPMD suppression burn-down into an unreviewed spec-metadata rewrite. Cost, measured: 149 → 162 — 11 ElseExpression, 1 ShortVariable, 1 LongVariable, and the AutomationCompilerService::saveOrUpdate() consolidation.

Follow-up (not this PR): those three gate failures are real, now precisely diagnosed, and belong to the automation-designer spec owner. They will bite the next PR that touches any of those six files.

Both of the brief's traps fired for real

Re-measuring after every rule is what caught them.

1. The extraction tips the class. Three extractions from AppOverrideService pushed BooleanArgumentFlag 13→14, ExcessiveClassLength 7→8, TooManyMethods 4→5 — the class sits exactly on three thresholds at once. All three were restructured to need no new method (a continue guard in deepMergeDelta, an early return in upsert). The one that genuinely could not be — upsertUserDelta, where both arms write a different object — was left as an else rather than traded for three other findings.

2. Removing an else adds complexity. Converting the two elevate-or-not if/elses in MigrateToVersionedModel::migrateOne() gave it a new NPathComplexity finding (336). Fixed structurally, not by relaxing anything: the duplicated pair folded into one guarded helper (runElevated()), which removes the elses and puts the method below where it started. The state chain in run() keeps its else if and drops only the bare else: PHPMD flags the bare else only, so that costs no extra path, whereas two independent ifs would have doubled an already-flagged NPath.

Template fix — honest scope: it retires 0 findings here

The <exclude-pattern>*Migration*</exclude-pattern> nested inside the UnusedFormalParameter <rule> was inert — PHPMD 2.15 honours exclude-pattern only as a direct child of <ruleset>. The rule now lives alone in phpmd-unusedparams.xml with a top-level exclusion, run as a second leg where neither leg can short-circuit the other, and --exclude '*/Resources/template/*' preserved on both legs.

openbuild has no lib/Migration directory, so this changes nothing about what is reported. Adopted for fleet uniformity only.

Baseline: RETAINED

162 findings remain. Deleting it would redden a gate that really runs.

rule n cause
CyclomaticComplexity 40 genuine debt — automation-compiler / copilot / app-override services
NPathComplexity 25 genuine debt
ExcessiveClassComplexity 23 genuine debt — class-level
CouplingBetweenObjects 15 genuine debt — needs collaborator extraction
BooleanArgumentFlag 13 genuine debt — each is an API change
ElseExpression 12 1 is AppOverrideService::upsertUserDelta (trap 1); 11 are blocked by the file-scoped gates above, not by effort
ExcessiveMethodLength 11 genuine debt
ExcessiveClassLength 7 genuine debt — class-level
TooManyMethods 4 genuine debtAppOverrideService shows this cannot be done piecemeal
StaticAccess 4 3 × \OCP\Server::get() service-locator calls in broker paths (genuine debt) + OpenRegister's Bootstrap::register, a bootstrap-time static by design
ExcessiveParameterList 3 genuine debt
ShortVariable / LongVariable 1 + 1 in reverted files; blocked by the gates above
TooManyPublicMethods 1 genuine debt — class-level
UnusedFormalParameter 1 INTERFACE-MANDATEDSeedHelloWorldFixture::execute() implements Symfony\Component\Console\Command\Command::execute(InputInterface $input, OutputInterface $output). $input is unused but the signature cannot change.
LongClassName 1 STRUCTURALLY UNREACHABLE hereUnsupportedAutomationCombinationException is a normative identifier in openspec/specs/automation-designer/spec.md and three archived changes. Renaming it in code alone desynchronises the spec.

Triage split of the 162: 1 interface-mandated · 1 structurally unreachable (spec-normative name) · 160 genuine debt, of which 13 are blocked on the pre-existing gate failures above and 2 on a larger structural change.

Verification

check before after
phpunit 746 tests, 2342 assertions — OK 746 tests, 2342 assertions — OK
phpcs exit 0, 0 errors
psalm exit 0
phpstan exit 0
composer phpmd (both legs, baseline in place) exit 0 exit 0
hydra-gates exit 0, 59 of 61 gates reported

The renames left stale docblock alignment; fixed with the repo's own phpcbf, scoped to the files I touched, then phpmd re-measured to confirm nothing moved.

A local gates run exited 143 at one point. That is SIGTERM (128+15), not a failure count — the process was killed and only 20 of 61 gates ran. The script says so itself ("the rest never executed"). It was re-run clean before this was pushed.

What was NOT done

  • No @SuppressWarnings, baseline entry, threshold change, .skip or weakened assertion was added anywhere.
  • The baseline was not regenerated smaller — it is untouched.
  • No @spec line was touched. The 17 dead anchors are reported, not guessed at.
  • The complexity family, CouplingBetweenObjects, BooleanArgumentFlag and the class-level rules were not attempted.
  • UnsupportedAutomationCombinationException was deliberately not renamed.
  • No sed/awk/script edits to shipped code (only the repo's own phpcbf).

…de-pattern

Part of ConductionNL/.github#155 (fleet PHPMD suppression audit).

`composer phpmd` reports 0 today. With phpmd.baseline.xml deleted it reports
195. The gate is live: openbuild's Code Quality workflow calls the shared
ConductionNL/.github quality.yml, where `enable-phpmd` defaults to true, so the
phpmd leg genuinely runs on every push and PR — the baseline is hiding findings
from a check that really executes.

Two things worth recording about the baseline file itself:

  * Its 174 entries suppress 195 findings, because a PHPMD baseline entry is
    scoped to a (rule, file) pair — optionally a method — never to a line. A
    class-scoped entry is therefore an open licence: every FUTURE violation of
    that rule in that file is silently suppressed too. "174 entries" is not
    "174 known problems", and the shortfall is granularity, not staleness.
  * PHPMD auto-discovers phpmd.baseline.xml from the working directory.
    Dropping --baseline-file from the composer script changes nothing; only
    deleting the file disables it.

What this commit retires (measured, PHPMD 2.15.0 / PHP 8.4.22, baseline
deleted both times, --exclude '*/Resources/template/*' on every leg): 195 -> 149.

  * ElseExpression 27 -> 1.
  * MissingImport 3 -> 0 (fully-qualified `new \RuntimeException` in
    AppOverrideService).
  * ShortVariable 9 -> 0 ($m/$ca/$cb/$i -> $matches/$condA/$condB/$seq/$cursor).
  * LongVariable 5 -> 0; the two renamed constructor-promoted properties are
    also renamed at their named-argument call sites in the unit tests.
  * UnusedFormalParameter 3 -> 1. Two were genuinely dead and are REMOVED, with
    their call sites updated: AppOverrideService::userVersionSlug($appId) and
    AppRepoParser::decodeChannelEntry($path).
  * CyclomaticComplexity 41 -> 40, from the two consolidating extractions below.

The ElseExpression work is behaviour-preserving throughout: initialise-then-
override where both branches were pure, guard-and-return where they were not,
`continue` inside loops, and extractions where both arms had to write so exactly
one could run. No ternaries were introduced.

Two traps from the brief fired for real, and re-measuring after every rule is
what caught them:

  1. THE EXTRACTION TIPS THE CLASS. Three extractions from AppOverrideService
     pushed BooleanArgumentFlag 13->14, ExcessiveClassLength 7->8 and
     TooManyMethods 4->5 — the class sits exactly at those thresholds. All three
     were restructured to need no new method (a `continue` guard in
     deepMergeDelta, an early return in upsert), and the one that genuinely
     could not be — upsertUserDelta, where both arms write a different object —
     was LEFT AS AN ELSE rather than traded for three other findings. That is
     the single remaining ElseExpression.

  2. REMOVING AN `else` ADDS COMPLEXITY. Converting the two elevate-or-not
     if/elses in MigrateToVersionedModel::migrateOne() gave it a NEW
     NPathComplexity finding (336), and converting the two update-or-create
     if/elses in AutomationCompilerService::applyRuleSet() took its
     CyclomaticComplexity from 9 to 11. Both were fixed STRUCTURALLY rather than
     by relaxing anything: each pair of duplicated branches was folded into one
     guarded helper (`runElevated()`, `saveOrUpdate()`), which removes the elses
     AND lowers the host method's complexity below where it started. A third,
     the state chain in run(), keeps its `else if` and only drops the bare
     `else` — PHPMD flags the bare `else` only, so that costs no extra path,
     whereas two independent ifs would have doubled an already-flagged NPath.

Also applies the fleet-standard scoped-ruleset shape. The
`<exclude-pattern>*Migration*</exclude-pattern>` nested inside the
UnusedFormalParameter `<rule>` was INERT — PHPMD 2.15 honours exclude-pattern
only as a direct child of `<ruleset>`. The rule now lives alone in
phpmd-unusedparams.xml with a top-level exclusion, run as a second leg with
neither leg able to short-circuit the other, and `--exclude
'*/Resources/template/*'` preserved on BOTH legs so the analysed file set is
unchanged. Stated plainly: openbuild has no lib/Migration directory, so this
retires ZERO findings here. It is adopted for fleet uniformity.

phpmd.baseline.xml is RETAINED. 149 findings remain and deleting it would
redden a gate that really runs. What remains, and why:

  * CyclomaticComplexity 40 / NPathComplexity 25 / ExcessiveClassComplexity 23 /
    ExcessiveMethodLength 11 / ExcessiveClassLength 7 — genuine debt in the
    automation-compiler, copilot and app-override services.
  * CouplingBetweenObjects 15, TooManyMethods 4, ExcessiveParameterList 3,
    TooManyPublicMethods 1 — class-level; need collaborator extraction, which
    AppOverrideService's thresholds show cannot be done piecemeal.
  * BooleanArgumentFlag 13 — genuine debt; each is an API change.
  * StaticAccess 4 — `\OCP\Server::get()` service-locator calls in three broker
    paths plus OpenRegister's `Bootstrap::register` in AppInfo\Application; the
    Bootstrap one is a bootstrap-time static by design.
  * ElseExpression 1 — AppOverrideService::upsertUserDelta, see trap 1 above.
  * UnusedFormalParameter 1 — INTERFACE-MANDATED. SeedHelloWorldFixture::execute()
    implements Symfony\Component\Console\Command\Command::execute(InputInterface
    $input, OutputInterface $output). `$input` is unused but cannot be dropped.
  * LongClassName 1 — UnsupportedAutomationCombinationException. Deliberately
    NOT renamed: the name is a normative identifier in
    openspec/specs/automation-designer/spec.md and in three archived changes.
    Renaming it in code alone would desynchronise the spec, and rewriting
    archived changes is out of scope.

Verified: phpunit 746 tests / 2342 assertions before AND after, both green.
phpcs exits 0 (the renames left stale docblock alignment; fixed with the repo's
own phpcbf, 24 errors, no remaining errors). psalm and phpstan both exit 0.
Both phpmd legs exit 0 with the baseline in place, so CI stays green.

No suppression, baseline entry, threshold change or skip was added anywhere.
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openbuild @ a806f23

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

Quality workflow — 2026-08-05 13:23 UTC

Download the full PDF report from the workflow artifacts.

…6 files

Touching these six files pulls three PRE-EXISTING hydra-gate failures into the
diff-scoped gate run, all of them in code this PR did not write:

  * gate-2 forbidden-patterns — the deliberate, documented error_log() in
    AppInfo\Application::register(), which carries its own phpcs:ignore and a
    comment explaining that no PSR logger exists that early. Removing an else
    necessarily moves those lines into the diff. The call must stay; the gate
    has no per-call waiver; so the file must stay untouched.
  * gate-46 spec-anchor-existence — 17 dead @SPEC targets across
    ExportsController, AutomationsController, AppInfo\Application,
    RuleActionDispatcher, AutomationCompilerService and TemplateSeedService.
    They point at openspec/changes/automation-designer/... which was ARCHIVED
    to openspec/changes/archive/2026-07-11-automation-designer/... and at
    #req-autd-NNN anchors that exist in NO version of the spec, canonical or
    archived — the canonical spec uses '### Requirement: <prose>' headings.
    Repairing them means deciding, per tag, which Requirement each method now
    implements. That is spec-traceability work for the automation-designer
    owner; guessing it here would encode WRONG links, which is worse than a
    dead one.
  * gate-49 controller-exception-translation —
    AutomationsController::recompileAndRespond() lacks a try/catch or @throws.

The gates are FILE-scoped (_filter_grep_by_scope keys on the path), so any edit
to one of these files puts all of its pre-existing findings in scope. Reverting
is the only way to keep a PHPMD suppression burn-down from silently becoming a
spec-metadata rewrite.

Cost, measured: openbuild goes 149 -> 161 with the baseline deleted (still
195 -> 161). Specifically this gives back 11 ElseExpression, 1 ShortVariable and
1 LongVariable, and drops the AutomationCompilerService::saveOrUpdate()
consolidation, so CyclomaticComplexity returns to 41.

Follow-up filed in the PR body: those three gate failures are real and now
precisely diagnosed, but they belong to a separate change.
@rubenvdlinde rubenvdlinde changed the title chore(phpmd): burn down 46 baselined findings (195 → 149) and fix the inert exclude-pattern chore(phpmd): burn down 33 baselined findings (195 → 162) and fix the inert exclude-pattern Aug 5, 2026
@rubenvdlinde rubenvdlinde reopened this Aug 5, 2026
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openbuild @ d387663

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

Quality workflow — 2026-08-05 13:58 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

CI status — blocked on #130, not on this diff

Run 3101255297932 jobs, 30 pass, 2 fail: Hydra Gates and its downstream Quality Report. Everything this PR is actually about is green:

phpmd ✅ · phpcs ✅ · psalm ✅ · phpstan ✅ · lint ✅ · phpmetrics ✅ · all 6 PHPUnit matrix legs (8.3/8.4 × stable31/32/33) ✅ · frontend build + unit + manifest + l10n ✅ · eslint ✅ · stylelint ✅ · security & license (npm + composer) ✅

Why Hydra Gates is red, and why it is not this diff

The gate run's own verdict line:

[hydra-gates] Scope: diff vs origin/development — 20 changed file(s)
[hydra-gates] COVERAGE: 59 of 61 declared gates reported a result.
[hydra-gates] GATES THAT DID NOT RUN: 24 33
[hydra-gates] RESULT: ALL GATES PASSED — EXCEPT GATES 24 33, WHICH DID NOT RUN.

Every gate that ran passed. The job fails only because hydra-gates-require-full-coverage is set and gates 24/33 did not report. They did not report because their producer jobs are skipped by configuration in this repo — E2E Tests (Playwright) and Integration Tests (Newman) are both skipped, and Quality Report says so explicitly: Unable to download artifact(s): Artifact not found for name: playwright-report.

Three pieces of evidence that this is repo-level and pre-existing:

  1. The base development run 31003443583 has the same condition — ALL GATES PASSED — EXCEPT GATES 4 24 33, WHICH DID NOT RUN — and its Hydra Gates job is success. The only difference is its scope line: 0 changed file(s). Base resolves; this PR changes nothing. A zero-file diff waives the coverage requirement.
  2. Therefore any openbuild PR that changes at least one file hits this, regardless of content.
  3. ci(quality): move the hydra-gates pin v1.0.1 -> v1.3.0 #130 (ci(quality): move the hydra-gates pin v1.0.1 -> v1.2.0) exists to fix exactly this. On that branch, with 1 changed file, Hydra Gates is success at 58 of 63 — v1.2.0 reclassifies the non-running gates instead of blocking on them. This repo currently pins hydra-gates-ref: v1.0.1.

I have deliberately not touched the pin here — that is #130's change, and duplicating it would create a conflict.

Action: merge #130 first, then re-run this PR. I am leaving #129 open rather than merging over a red rollup.

Note on the earlier red on this branch

The run for the same SHA created at 13:39:38Z reported conclusion: failure with zero jobs and name = the raw path .github/workflows/code-quality.yml. That was the fleet-wide .github outage (13:29:41Z–13:47:44Z, .github d68fb727, reverted by .github #166). Zero jobs is not a verdict — it was discarded and the PR re-triggered, which produced the 32-job run above.

Positive-controlled (PHPMD 2.15, PHP 8.4, absolute paths): with */lib/X/* a
probe in lib/Service/X/ is reported; with */X/* it is silently swallowed. Both
correctly exclude the app's own directory, so the narrow form is strictly safer.
Matches the fleet-canonical shape in nextcloud-app-template #125, doriath #157,
larpingapp #263 and planix #315.
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openbuild @ 8357546

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

Quality workflow — 2026-08-05 19:43 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openbuild @ ab2583a

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

Quality workflow — 2026-08-05 20:08 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 68c2ee9 into development Aug 5, 2026
34 checks passed
@rubenvdlinde
rubenvdlinde deleted the chore/phpmd-baseline-burndown branch August 14, 2026 09:37
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