Skip to content

fix(quality): scope the Migration phpmd exclusion to UnusedFormalParameter only - #2343

Merged
rubenvdlinde merged 2 commits into
developmentfrom
fix/phpmd-scoped-migration-exclude
Aug 5, 2026
Merged

fix(quality): scope the Migration phpmd exclusion to UnusedFormalParameter only#2343
rubenvdlinde merged 2 commits into
developmentfrom
fix/phpmd-scoped-migration-exclude

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

What this fixes

Part of the fleet suppression audit (ConductionNL/.github#155). Tracked here by #2338.

openregister was the fleet's only repo whose <exclude-pattern>*/Migration/*</exclude-pattern>
actually worked — and that was the problem. A top-level exclude-pattern is applied by PDepend at
file-collection time, so it does not drop lib/Migration from one rule; it drops it from
every rule in the ruleset.

Measured on PHPMD 2.15.0 / PHP 8.4.22, that single line was silently swallowing 11 real
findings
that had nothing to do with unused parameters:

rule count
NPathComplexity 3
ElseExpression 3
StaticAccess 2
ExcessiveMethodLength 2
CyclomaticComplexity 1

The same file also carried a second <exclude-pattern>*Migration*</exclude-pattern> nested
inside
the UnusedFormalParameter rule. PHPMD 2.15 honours exclude-patterns only as direct
children of <ruleset>; nested ones are parsed and discarded. That one was inert — it was the
top-level line doing all the work, for every rule.

The fix

phpmd.xml loses its top-level exclude-pattern and no longer declares UnusedFormalParameter.
A new sibling ruleset, phpmd-unusedparams.xml, holds that rule alone with its own top-level
exclude-pattern — so the lib/Migration exclusion now applies to exactly one rule, and every
other rule sees lib/Migration again.

composer phpmd runs both rulesets as separate legs, worst exit code winning; neither leg can
short-circuit the other:

"phpmd": "E=0; ./vendor/bin/phpmd lib text phpmd.xml --baseline-file phpmd.baseline.xml || E=$?; ./vendor/bin/phpmd lib text phpmd-unusedparams.xml --baseline-file phpmd.baseline.xml || E=$?; exit $E",

UnusedFormalParameter genuinely cannot apply to migrations: 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.

Retired suppressions

With the exclusion now scoped to the rule that needs it, every
@SuppressWarnings(PHPMD.UnusedFormalParameter) in lib/Migration/ is dead weight.
246 removed across 174 files (236 exact-match + 10 with a stray space before the paren, which
were never valid annotations in the first place). No suppression was added anywhere.

The 11 surfaced findings — all fixed

  • Version1Date20260521120000.php — 2× ElseExpression. Extracted the two platform-specific
    index-drop paths into dropBareUuidIndexesPostgres() / dropBareUuidIndexesMysql() behind a
    dispatcher, and the platform-specific introspection query into
    queryTablesWithBareUuidIndex(). Same SQL, same order, same output messages.
  • Version1Date20260524100000.phpNPathComplexity 256 + ExcessiveMethodLength 104.
    Five independent if (hasColumn) addColumn blocks became a spec list + one loop.
  • Version1Date20260524130000.phpExcessiveMethodLength 137. Column and index declarations
    extracted into addColumns() / addIndexes(), columns driven from a spec list.
  • Version1Date20260525240000.phpNPathComplexity 256. Eight if (hasColumn) blocks →
    spec list + loop (the schema_id entry keeps its extra index).
  • Version1Date20260614100000.phpCyclomaticComplexity 18 + NPathComplexity 131072.
    Fourteen if (hasColumn) blocks → spec list + loop.
  • Version1Date20260706100000.php — 2× StaticAccess on \OCP\Server::get(). Now injects
    Psr\Container\ContainerInterface and resolves through it, which keeps the lazy resolution the
    original comment asks for (a DI failure during upgrade still degrades instead of aborting)
    while removing the static call. The unit test gains a container mock that throws, reproducing
    exactly the path it exercised before.
  • Version1Date20260726000000.phpElseExpression. Inverted to an early return.

All column-order, SQL text and IOutput messages are unchanged. No thresholds were relaxed and
no ternaries were introduced (Squiz.PHP.DisallowInlineIf).

Not in this PR

  • phpmd.baseline.xml (517 entries) is untouched here; it is the subject of a follow-up.
  • The 65 non-UnusedFormalParameter @SuppressWarnings still in lib/Migration/
    (36 ExcessiveMethodLength, 15 CyclomaticComplexity, 11 NPathComplexity, 2 StaticAccess,
    1 ExcessiveClassLength) were previously dead — the directory-wide exclusion made them
    unreachable. They are load-bearing from this PR onward. Burning them down belongs with the
    baseline work, not here.
  • BUG: PDF→ODT anonymisation fallback (Path B) is unreachable in production — its tests construct the trigger themselves #2339 (the PdfTextReplacer $strict dead-parameter bug) is a real defect and is not
    touched by this PR. It lives outside lib/Migration, so it was never affected by the
    exclude-pattern either way.

Closes part of #2338.

…meter only

phpmd.xml carried a TOP-LEVEL <exclude-pattern>*/Migration/*</exclude-pattern>.
A top-level exclude-pattern is applied by PDepend at file-collection time, so it
drops lib/Migration from EVERY rule in the ruleset, not from one rule. Measured
on phpmd 2.15.0 / PHP 8.4.22, that line was silently swallowing 11 real
findings: 3 NPathComplexity, 3 ElseExpression, 2 StaticAccess,
2 ExcessiveMethodLength, 1 CyclomaticComplexity.

The same file also had a second, NESTED <exclude-pattern>*Migration*</exclude-pattern>
inside the UnusedFormalParameter rule. PHPMD 2.15 honours exclude-patterns only
as direct children of <ruleset>; nested ones are parsed and discarded, so that
one was inert.

UnusedFormalParameter now lives alone in phpmd-unusedparams.xml with its own
top-level exclude-pattern, so the exclusion applies to that rule and nothing
else. `composer phpmd` runs both rulesets as separate legs, worst exit code
winning, and neither leg can short-circuit the other. OCP\Migration\IMigrationStep
mandates the changeSchema / preSchemaChange / postSchemaChange signatures, so
that one rule genuinely cannot apply to migrations.

With the exclusion scoped, 246 now-redundant
@SuppressWarnings(PHPMD.UnusedFormalParameter) annotations were deleted from
174 files in lib/Migration (236 exact + 10 with a stray space before the paren,
which were never valid annotations). No suppression was added anywhere.

All 11 surfaced findings are fixed with behaviour-preserving refactors:
column-addition ifs replaced by a spec list plus a loop, platform branches
extracted into named private methods, one else inverted to an early return, and
two \OCP\Server::get() static calls replaced by an injected
Psr\Container\ContainerInterface (keeping the lazy resolution the original
comment asks for). Column order, SQL text and IOutput messages are unchanged.

Verified: both legs exit 0 over all 1400 files in lib/, and both were
positive-controlled (a planted UnusedFormalParameter outside lib/Migration and a
planted ElseExpression inside it are both reported, exit 2). phpcs clean over
lib/. Unit suite unchanged at 16007 tests / 35916 assertions before and after.

Refs ConductionNL/.github#155
Refs #2338
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 359295e

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

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

Download the full PDF report from the workflow artifacts.

Both are pre-existing debt in files this PR already touches; the gates are
diff-scoped, so they only became visible now.

gate-28 license-triangle: five migrations carried
@license AGPL-3.0-or-later while composer.json declares EUPL-1.2. Corrected
to EUPL-1.2 — this is a docblock correction to match the declared licence,
not a relicensing.

gate-46 spec-anchor-existence: Version1Date20260511100000 pointed its @SPEC
at openspec/changes/scholiq-deps/tenant-key-api/tasks.md, a change directory
that was never archived under that name, so the target does not resolve.
Retargeted at the canonical openspec/specs/saas-multi-tenant/spec.md, which
is where the openregister_tenant_keys requirements live. (The same stale
pointer also sits in lib/Service/TenantKeyService.php; that file is outside
this PR's diff and is left for a follow-up rather than widening the scope.)
@rubenvdlinde rubenvdlinde reopened this Aug 5, 2026
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 34c323f

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

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

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit c39acaa into development Aug 5, 2026
30 of 33 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/phpmd-scoped-migration-exclude branch August 5, 2026 14:17
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