Skip to content

fix(phpmd): scope the lib/Migration UnusedFormalParameter exclusion to its own ruleset - #739

Merged
rubenvdlinde merged 3 commits into
developmentfrom
fix/phpmd-unusedparams-ruleset
Aug 6, 2026
Merged

fix(phpmd): scope the lib/Migration UnusedFormalParameter exclusion to its own ruleset#739
rubenvdlinde merged 3 commits into
developmentfrom
fix/phpmd-unusedparams-ruleset

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Part of the fleet PHPMD ruleset fix (ConductionNL/.github#155). Propagates the shape
already merged in nextcloud-app-template #125, doriath #157, larpingapp #263 and planix #315.

The defect

phpmd.xml declared:

<rule ref="rulesets/unusedcode.xml/UnusedFormalParameter">
    <exclude-pattern>*Migration*</exclude-pattern>
</rule>

A nested <exclude-pattern> is inert. PHPMD 2.15 reads exclude-patterns in
RuleSetFactory::getIgnorePattern(), which walks $xml->children() — only elements
directly under <ruleset>. A nested one parses without error and is discarded, so
lib/Migration was scanned by the very rule the pattern was written to spare.

Reproduced directly on this repo's ruleset: a probe class in lib/Migration with three
unused formal parameters was reported, exit 2, with the nested pattern in place.

Why not simply hoist the pattern

A top-level <exclude-pattern> is applied by PDepend's ExcludePathFilter at
file-collection time, so it drops the file from every rule in the ruleset — real
complexity, StaticAccess and method-length findings in migrations would silently vanish.

The shape shipped here

  • phpmd.xml no longer declares UnusedFormalParameter; a comment records why.
  • New phpmd-unusedparams.xml holds that rule alone, with a top-level
    */Migration/* exclude — so the exclusion is scoped to that one rule and nothing else.
  • The phpmd composer script runs both legs, keeping the worst exit code so neither
    leg can short-circuit the other.

Why lib/Migration is exempt from this one rule: OCP\Migration\IMigrationStep mandates
changeSchema(IOutput $output, Closure $schemaClosure, array $options) and
preSchemaChange/postSchemaChange with the same three parameters. A step that needs none
of them still cannot drop them — the signature is not ours to change.

Measurement

PHPMD 2.15.0 (the version this repo's lockfile pins) on PHP 8.4.22, run in a
nextcloud:latest container: host PHP 8.2 makes vendor/bin/phpmd die in
platform_check.php with exit 255, which reads exactly like a clean run. Exit codes
are read directly, never through a pipe. Findings are compared as normalised
path:line:rule triples — PHPMD right-pads the file:line column in text output, so raw
line diffs are meaningless.

findings leg exits
reported, before 0 leg1=0
reported, after 0 leg1=0 leg2=0
true (every @SuppressWarnings stripped on a throwaway copy), before 286 leg1=2
true, after 286 leg1=2 leg2=2

The true count strips every @SuppressWarnings in lib/ on a throwaway copy that is
never committed
, so the comparison isolates the ruleset change from the suppressions.

Retired: 0

Newly appearing (true): 0 — must be 0, and is.
Newly appearing (reported): 0. Retired (reported): 0.

Zero newly-hidden findings: every triple present before is present after, except the
retired UnusedFormalParameter hits inside lib/Migration listed above.

lib/Migration in this repo: 0 PHP file(s).

Dead-gate proof

A new leg that exits 0 on the shipped tree is indistinguishable from a leg that does not
run. Proven otherwise on a throwaway copy (probes removed before committing) — three
probes, run through the shipped two-leg invocation:

probe expected observed
lib/Migration/ZzProbeMigration.php UnusedFormalParameter dropped not reported
lib/Migration/ZzProbeMigration.php ElseExpression still live reported, leg 1
lib/ZzProbe/ZzProbe.php UnusedFormalParameter still live reported, leg 2
leg1=2
leg2=2
leg1	lib/Migration/ZzProbeMigration.php:6:ElseExpression
leg2	lib/ZzProbe/ZzProbe.php:4:UnusedFormalParameter
leg2	lib/ZzProbe/ZzProbe.php:4:UnusedFormalParameter

Leg 2 goes from exit 0 to exit 2 under the probe, so it is live. The Migration
ElseExpression probe is still reported, so isolating the rule did not blind the
other rules to lib/Migration — which is exactly what a hoisted top-level pattern would
have done.

The rig itself was positive-controlled before the first measurement: the same probes under
the unfixed ruleset were reported with exit 2, including the three UnusedFormalParameter
hits in lib/Migration that the nested pattern was supposed to suppress.

Baseline

phpmd.baseline.xml present: no. None was added, deleted or shrunk.

PHPMD auto-discovers phpmd.baseline.xml from the working directory, so removing the
--baseline-file flag would be a no-op — the baseline stays active either way. Verified
empirically on this fleet: identical command, baseline file present → 0 findings / exit 0;
same command with the file absent → 93 findings / exit 2.

Suppressions

0 deleted. This repo has no lib/Migration directory at all, so it carried no @SuppressWarnings(PHPMD.UnusedFormalParameter) tags for this fix to make redundant.

This fix therefore retires nothing in this repo. Its value is the corrected shape — the old nested pattern was inert either way, and the new leg is proven live below.

This repo still carries 59 UnusedFormalParameter and 211 other suppressions elsewhere in lib/; those are outside this PR's scope and untouched.

Note: lib/Service/TenantMigrationService.php contains "Migration" in its filename. It is not matched by */lib/Migration/* (nor by */Migration/*, which needs a directory segment), but it would have been matched by the older *Migration* form. It stays fully analysed.

No @SuppressWarnings was added. No threshold was changed, no rule weakened, no baseline
entry added, nothing skipped.

Tests

phpunit -c phpunit-unit.xml:

  • before: 1692 tests, 5648 assertions, 2 failures, 2 warnings, 10 deprecations, 5 skipped — already RED
  • after: 1692 tests, 5648 assertions, 2 failures, 2 warnings, 10 deprecations, 5 skipped — still the same 2 failures, unchanged

(The default phpunit.xml suite needs a live Nextcloud runtime and errors out in a bare
checkout — a pre-existing condition this change does not touch.)

What this PR does NOT do

  • does not add or remove a baseline, a @SuppressWarnings, a threshold change or a waiver
  • does not touch any rule other than UnusedFormalParameter
  • does not burn down any of the remaining suppression-free findings — that is separate work

…o its own ruleset

The nested <exclude-pattern> inside the UnusedFormalParameter <rule> was inert:
PHPMD 2.15 honours exclude-patterns only as direct children of <ruleset>, so
lib/Migration was scanned by the very rule the pattern was written to spare.

Hoisting the pattern to the top level of phpmd.xml would have worked but is
applied at file-collection time, dropping lib/Migration from EVERY rule and
silently swallowing real complexity, StaticAccess and method-length findings.

UnusedFormalParameter now lives alone in phpmd-unusedparams.xml with a
top-level */Migration/* exclude, and the phpmd composer script runs both legs
keeping the worst exit code.
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/procest @ d9b1471

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

Quality workflow — 2026-08-05 15:40 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/procest @ 482ea0b

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

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

Download the full PDF report from the workflow artifacts.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Held, not merged — blocked by a repo-wide gate-24 wiring failure, not by anything in this PR.

Hydra Gates is red here, but the failure is "a gate did not run", not "a gate found something":

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

hydra-gates-require-full-coverage is set on this repo, so a gate whose subject matter exists but which fails to report is a hard failure. That is the correct behaviour — a gate that did not run is not a gate that passed.

It is not this PR's doing. Measured across three independent procest PRs tonight with completely different diffs:

PR branch scope result
#739 fix/phpmd-unusedparams-ruleset 3 changed files gate-24 DID NOT RUN
#737 fix/wire-bewijsstuk-immutability-guard 9 changed files gate-24 DID NOT RUN
#742 chore/eupl-license-normalisation-2026-08-05 32 changed files gate-24 DID NOT RUN

Three different diffs, three different sizes, identical outcome. gate-24 (integration-parity) is structurally unwired in this repo right now, so no PR can currently go green here regardless of its content.

I deliberately did not reach for development to call this pre-existing: a push to the base branch scopes 0 files and Hydra Gates passes in ~20s having inspected nothing, so a green base run here would be vacuous and proves nothing. Absence of evidence from a vacuous run is not evidence of absence.

What unblocks this: the gate-24 parity work that is in flight (fix/gate-24-integration-parity, and the sibling work in openconnector and hermiq). Once a real gate-24 lands in procest, this PR needs only a re-run — its own substantive jobs are already green (28 SUCCESS on #739, with E2E Tests (Playwright) passing after a real 1633-second run).

Not merging on a red Hydra Gates, and not adding a waiver or flipping require-full-coverage to go green — that would convert a known-broken gate into a silently absent one across the whole repo.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/procest @ fa4fc1d

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

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

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 728c0e4 into development Aug 6, 2026
35 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/phpmd-unusedparams-ruleset branch August 6, 2026 05:08
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