Skip to content

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

Merged
rubenvdlinde merged 2 commits into
developmentfrom
fix/phpmd-unusedparams-ruleset
Aug 5, 2026
Merged

fix(phpmd): scope the lib/Migration UnusedFormalParameter exclusion to its own ruleset#279
rubenvdlinde merged 2 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 5 leg1=2
true, after 5 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, so the coverage is real the day a migration is added.

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

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

Tests

phpunit -c phpunit-unit.xml:

  • before: 893 tests, 4126 assertions, 1 PHPUnit warning — OK
  • after: 893 tests, 4126 assertions, 1 PHPUnit warning — OK (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/scholiq @ 71a5be2

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-specs
composer ✅ 100/100
npm ✅ 750/750
PHPUnit
Newman ⏭️
Playwright ⏭️
Hydra gates

Quality workflow — 2026-08-05 15:22 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/scholiq @ 2eb9a12

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-specs
composer ✅ 100/100
npm ✅ 750/750
PHPUnit
Newman ⏭️
Playwright ⏭️
Hydra gates

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

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 0d1ecf9 into development Aug 5, 2026
33 checks passed
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