fix(phpmd): scope the lib/Migration UnusedFormalParameter exclusion to its own ruleset - #434
Merged
Merged
Conversation
…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.
Contributor
Quality Report — ConductionNL/softwarecatalog @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| check-vue-demi | ✅ | ||||
| test-l10n | ✅ | ||||
| composer | ✅ | ✅ 121/121 | |||
| npm | ✅ | ✅ 849/849 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-05 15:26 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/softwarecatalog @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| check-vue-demi | ✅ | ||||
| test-l10n | ✅ | ||||
| composer | ✅ | ✅ 121/121 | |||
| npm | ✅ | ✅ 849/849 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-05 20:08 UTC
Download the full PDF report from the workflow artifacts.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 15, 2026
…Db layer
ADR-083 injected OCA\OpenRegister\Db\MagicMapper into three classes here. That
is another app's DATABASE layer — the coupling ADR-022 exists to prevent — and
no leaf app can load it, so its tests could not construct their own subjects. I
had added a hand-rolled MagicMapper stub to get past that; this removes the need
for one instead.
All three sites were doing the same thing: save without schema validation. The
published contract already exposes that, and saveObject() is not a lesser
route — OpenRegister's own SaveObject calls
metaHydrationHandler->hydrateObjectMetadata(entity:, schema:)
objectEntityMapper->update(entity:, register:, schema:)
which IS the magic-mapper route, with the metadata hydration these callers were
performing by hand. The comment claiming a plain save touches "just the blob
table" was wrong; the code was reimplementing OpenRegister's save pipeline one
layer too deep.
The flags matter, and one of them nearly went missing. FIX #434 chose
MagicMapper for TWO reasons, not one:
to avoid validation errors on the organisatie field (stored as UUID string
but Schema expects object type) AND to avoid triggering ObjectUpdatedEvent
cascades. That could interfere with the ongoing org activation process.
`_validation: false` covers the first. The second needs `silent: true`, and a
replacement carrying only the validation flag would have re-emitted those events
into an in-flight org activation — a behaviour change with no test to catch it.
Both flags are now passed at every site.
Left alone: ContactpersoonService's two `container->get(MagicMapper)` lookups.
They are lazy, not injected, so they neither block tests nor need a stub; they
carry the same FIX #434 reasoning and are worth converting on their own terms.
rubenvdlinde
added a commit
that referenced
this pull request
Aug 15, 2026
The previous commit left these deliberately: they are lazy container lookups rather than injected dependencies, so they blocked no test and needed no stub. They carry the same FIX #434 reasoning, and converting them needed reading what each actually does rather than pattern-matching the first one. Site 1 changes only the payload (username), so it is the same shape as the three already converted: _validation: false, silent: true. Site 2 is different, and this is why they were worth separating. It sets two pieces of entity METADATA that the payload API expresses differently: organisation travels in `@self`. SaveObject reads it and applies it via setOrganisation() BEHIND AN ACCESS CHECK — so an organisation the caller may not use is now refused rather than written. The direct mapper call bypassed that check entirely, which is a quiet authorisation improvement, not just a refactor. owner is not settable from the payload at all; SaveObject derives it from the acting user, so the UID is resolved to an IUser and passed as `currentUser`. Both keep _validation: false and silent: true, so neither the organisatie type mismatch nor the ObjectUpdatedEvent cascade behaviour changes. lib/ now has no code reference to OCA\OpenRegister\Db\MagicMapper. What remains is prose in RenameDutchCatalogColumns explaining how OpenRegister's schema sync adds columns, which is documentation of the other app's behaviour and correct where it is.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of the fleet PHPMD ruleset fix (ConductionNL/.github#155). Propagates the shape
already merged in
nextcloud-app-template#125,doriath#157,larpingapp#263 andplanix#315.The defect
phpmd.xmldeclared:A nested
<exclude-pattern>is inert. PHPMD 2.15 reads exclude-patterns inRuleSetFactory::getIgnorePattern(), which walks$xml->children()— only elementsdirectly under
<ruleset>. A nested one parses without error and is discarded, solib/Migrationwas scanned by the very rule the pattern was written to spare.Reproduced directly on this repo's ruleset: a probe class in
lib/Migrationwith threeunused 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'sExcludePathFilteratfile-collection time, so it drops the file from every rule in the ruleset — real
complexity,
StaticAccessand method-length findings in migrations would silently vanish.The shape shipped here
phpmd.xmlno longer declaresUnusedFormalParameter; a comment records why.phpmd-unusedparams.xmlholds that rule alone, with a top-level*/Migration/*exclude — so the exclusion is scoped to that one rule and nothing else.phpmdcomposer script runs both legs, keeping the worst exit code so neitherleg can short-circuit the other.
Why
lib/Migrationis exempt from this one rule:OCP\Migration\IMigrationStepmandateschangeSchema(IOutput $output, Closure $schemaClosure, array $options)andpreSchemaChange/postSchemaChangewith the same three parameters. A step that needs noneof 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:latestcontainer: host PHP 8.2 makesvendor/bin/phpmddie inplatform_check.phpwith exit 255, which reads exactly like a clean run. Exit codesare read directly, never through a pipe. Findings are compared as normalised
path:line:ruletriples — PHPMD right-pads thefile:linecolumn in text output, so rawline diffs are meaningless.
@SuppressWarningsstripped on a throwaway copy), beforeThe true count strips every
@SuppressWarningsinlib/on a throwaway copy that isnever 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
UnusedFormalParameterhits insidelib/Migrationlisted above.lib/Migrationin 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:
lib/Migration/ZzProbeMigration.phpUnusedFormalParameterlib/Migration/ZzProbeMigration.phpElseExpressionlib/ZzProbe/ZzProbe.phpUnusedFormalParameterLeg 2 goes from exit 0 to exit 2 under the probe, so it is live. The Migration
ElseExpressionprobe is still reported, so isolating the rule did not blind theother rules to
lib/Migration— which is exactly what a hoisted top-level pattern wouldhave 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
UnusedFormalParameterhits in
lib/Migrationthat the nested pattern was supposed to suppress.Baseline
phpmd.baseline.xmlpresent: yes (8 entries). None was added, deleted or shrunk.PHPMD auto-discovers
phpmd.baseline.xmlfrom the working directory, so removing the--baseline-fileflag would be a no-op — the baseline stays active either way. Verifiedempirically 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/Migrationdirectory 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 14
UnusedFormalParameterand 321 other suppressions elsewhere inlib/; those are outside this PR's scope and untouched.No
@SuppressWarningswas added. No threshold was changed, no rule weakened, no baselineentry added, nothing skipped.
Tests
phpunit -c phpunit-unit.xml:(The default
phpunit.xmlsuite needs a live Nextcloud runtime and errors out in a barecheckout — a pre-existing condition this change does not touch.)
What this PR does NOT do
@SuppressWarnings, a threshold change or a waiverUnusedFormalParameter