fix(quality): scope the Migration phpmd exclude to lib/, retire CountInLoopExpression, untrack the phpmd result cache - #2359
Conversation
…gn copyright claim, clear the coverage ratchet phpmd-unusedparams.xml carried <exclude-pattern>*/Migration/*</exclude-pattern>. PDepend compiles an exclude-pattern into an UNANCHORED regex (Input\ExcludePathFilter preg_quote()s the pattern, then turns `\*` into `.*`), so that form matches ANY path containing a `/Migration/` segment - lib/Service/Migration/, lib/Command/Migration/, any future lib/*/Migration/. Those are ordinary classes with no interface-mandated signature, so a genuine unused parameter in one would never be reported and the run would still look clean. openconnector carried exactly such a file. openregister has no lib/*/Migration/ directory today, which is precisely why this had to be fixed before one appears: the broad form fails silently and only on the day someone adds the directory. Now `*/lib/Migration/*`, matching the 19 repos that already carry the corrected shape. Twelve files carried `SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors`, scaffolding residue from the Nextcloud app skeleton. The licence sweep in #2350 relabelled the adjacent SPDX-License-Identifier from AGPL-3.0-or-later to EUPL-1.2 - which asserts that Nextcloud GmbH's copyright is EUPL-licensed. We cannot relicense a third party's copyright. Every one of the twelve is Conduction-authored: each carries `@author Conduction Development Team` and `@copyright Conduction B.V.` in its own PHPDoc, and `git log --follow` shows only Conduction committers. The stray holder is corrected to Conduction rather than deleted, so no file loses its REUSE metadata. Side effect, measured by running phpcs on both versions at the identical path: 2 pre-existing "Missing short description in doc comment" errors go away. CountInLoopExpression retired entirely - all 3 baseline entries, all 3 findings. Two are `do { … } while (count($page) === $limit)` where the page is replaced wholesale each iteration and never mutated in the body, so the count is taken once per page into a variable; one is `for ($i = 1; $i < count($rings); $i++)` over an array the body does not touch, so the count is hoisted. Behaviour is identical in all three. .phpmd.result-cache.php untracked and gitignored. It is generated output, and a correctness hazard while committed: `composer quality:phpmd-score` passes --cache, so a stale cache in the tree makes PHPMD replay a verdict for code that has since changed - a gate reporting a result it never computed. .coverage-baseline 58.87 -> 58.93. This was the ONLY red job on development: CI measured coverage that had improved past its own committed baseline. Raising it tightens the ratchet. Unit suite before and after, same container and same vendor: 16030 tests, 35963 assertions, 0 failures, 0 errors - byte-identical totals.
#2356 landed the same 12 Nextcloud GmbH SPDX residue files while this branch was in flight, deleting the stray block rather than correcting the holder. Both remove the false third-party licence claim, which is the load-bearing outcome, so this takes development's version verbatim and drops the duplicate work here.
Quality Report — ConductionNL/openregister @
|
| 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 21:33 UTC
Download the full PDF report from the workflow artifacts.
Hydra gate-46 (spec-anchor-existence) failed on this PR with 6 unresolved targets. All six are pre-existing debt in files this PR already touches, which is what pulled them into the gate's ADR-020 diff scope; none was introduced here. 62 of the other gates passed and coverage was 60 of 60 applicable, so this was a single real failure, not a broken run. lib/Service/VocabularyImportService.php (4 tags) pointed at openspec/changes/skos-concept-registers/... - a CHANGE directory. That change was archived on 2026-07-23, so the path stopped existing the moment it moved to openspec/changes/archive/. A @SPEC tag must target the canonical openspec/specs/ home, which is where the spec lives now; two of the four also carried "#skos-002", which is not a heading, and now name the heading that actually exists. lib/ContextChat/ContentProvider.php (4 tags, 2 distinct anchors) named "#requirement-getitemurl-must-resolve-through-the-existing-deep-link-registry" and "#requirement-initial-import-must-walk-opted-in-schemas-in-batches-and-must- be-re-runnable-via-occ". Neither heading exists; both requirements were merged into one, "Requirement: getItemUrl and initial import reuse existing OpenRegister infrastructure", and the tags were never moved with it. Every target verified against gate-46's OWN two slug rules - slugify() and gh_slugify(), which differ on punctuation inside a word - by resolving each fragment back to the heading text it matches. No overlap with #2355, which repoints a different set of anchors in file-actions.
Quality Report — ConductionNL/openregister @
|
| 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 21:59 UTC
Download the full PDF report from the workflow artifacts.
… with development
I raised .coverage-baseline 58.87 -> 58.93 because that was the value CI itself
recomputed on development, and Coverage Baseline Check was development's only
red job. On this PR it then failed the OTHER direction:
Coverage baseline: 58.93%
Coverage current: 58.88%
FAIL: Coverage dropped by 0.05%
Not a regression from this PR. Development moved between my two CI runs — the
suite went 16030 -> 16038 tests — so the merge base this PR is measured against
computes 58.88, not the 58.93 that development's own HEAD computed earlier. The
two jobs also check opposite things: development's runs coverage-guard.php
--update-baseline and fails when the committed value is STALE, while a PR runs
it plain and fails when coverage DROPS below the committed value. Pinning a
number from one tree to satisfy the other is what broke this.
So the bump leaves this PR. It belongs in a one-line change computed on
development's own HEAD, at a moment development is not mid-merge — not carried
in on a PHPMD branch whose merge base keeps moving underneath it. The job was
red before this branch existed and is unaffected by it either way.
Nothing is weakened: .coverage-baseline returns to development's committed
58.87, exactly as found.
Quality Report — ConductionNL/openregister @
|
| 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 23:36 UTC
Download the full PDF report from the workflow artifacts.
… 10 entries for a deleted file phpmd.baseline.xml 519 -> 506 entries, and one rule family leaves ENTIRELY. A PHPMD baseline entry is scoped to (rule, file) - optionally a method, NEVER a line - so one entry covers every current AND future violation of that rule in that file. It is an open licence, not a record. Shrinking the count is therefore not the point; getting a family to zero is, because only then does a NEW violation of that rule fail CI. CountInLoopExpression: all 3 entries removed. The 3 findings behind them were fixed in this PR, not suppressed. Verified with a single-rule ruleset over all of lib and NO baseline in play: 3 findings before, 0 after. lib/Service/Flow/FlowActionService.php: 10 entries for a file that no longer exists (WeightedMethodCount, CouplingBetweenObjects, LongVariable, ShortVariable, MissingImport, and Cyclomatic/Npath on run/runNamedFlow/runAction). Deleted with the file; suppressing nothing; free to remove. The other 509 entries are all LIVE and were left alone. I checked, and the first answer was wrong in an instructive way: matching baseline entries against the report by rule name reported FIVE families - NPath (62), LongMethod (33), WeightedMethodCount (32), LongParameterList (14), LongClass (6) - as "entirely stale", 147 free deletions. They are not. The baseline stores the rule CLASS (PHPMD\Rule\Design\LongMethod) while the XML report writes the rule NAME (ExcessiveMethodLength), and those five differ. With the mapping applied the accounting closes exactly: 767 true findings, 767 suppressed by live entries, nothing unexplained. Uniformity across five independent families was the tell. Measured with the baseline file MOVED ASIDE, not by dropping --baseline-file: PHPMD auto-discovers phpmd.baseline.xml sitting next to the ruleset and applies it either way, so un-flagging it yields a silently baselined run that looks clean. Independently corroborates #2347's 749 + 16.
Quality Report — ConductionNL/openregister @
|
| 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-06 05:08 UTC
Download the full PDF report from the workflow artifacts.
Quality Report — ConductionNL/openregister @
|
| 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-06 05:20 UTC
Download the full PDF report from the workflow artifacts.
Quality Report — ConductionNL/openregister @
|
| 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-06 05:30 UTC
Download the full PDF report from the workflow artifacts.
E2E:
|
| run | branch | failed at |
|---|---|---|
| 31050141324 | #2358 (docs only) | expect(locator).toBeVisible() |
| 31073228191 | this PR | line 185 — "Run now did not produce a run for this flow" (20s poll) |
| 31073228191 (rerun) | this PR | line 155 — waitForURL off #/flows/new after Save (20s) |
3. The margin is wide, so it is load-sensitive. development passes the whole spec in 5.5s; every failure is a 20-second timeout expiring. That is a factor-of-four gap being crossed by runner load, not by a logic change.
4. The only code in this PR is statically unreachable from the flow path. Three CountInLoopExpression fixes, full caller trace:
ContentProvider::reindex()— called only fromtriggerInitialImport(), its own unit test, andContextChatReindexCommand(an occ command). No HTTP route reaches it.GeoFeatureCollectionBuilder::geodesicAreaM2()— called only from inside that class and its unit test.VocabularyImportService::deprecateMissing()—private, called only from within the SKOS importer.
grep over lib/Service/Flow/, FlowRunsController and FlowsController returns no reference to any of the three classes. Nothing else in the diff is executable: a 13-line phpmd.baseline.xml deletion, phpmd-unusedparams.xml, .gitignore, @spec docblock text, and untracking .phpmd.result-cache.php.
All three loops are also behaviour-preserving by construction — each collection is replaced wholesale per iteration and never mutated in the body, so the hoisted count is the same value the condition read.
What I am not claiming: that this branch passing E2E at 34d32110 exonerates it. It does not — the spec only arrived here with the development merge (+191 lines), so the earlier green runs never executed it.
Recommendation: do not merge on a re-run that happens to go green; the race is in the spec. flow-controls.spec.ts needs its three 20s waits root-caused (Save round-trip, node-catalog load, and run-row creation after "Run now") rather than widened. Raising the timeouts would hide a real slowness signal.
Hydra Gates and Quality Report are red on development too and are unrelated to this branch.
Quality Report — ConductionNL/openregister @
|
| 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-06 08:48 UTC
Download the full PDF report from the workflow artifacts.
What this is
The tail of openregister's PHPMD debt, measured rather than assumed. The headline defect — a live top-level
<exclude-pattern>*/Migration/*</exclude-pattern>inphpmd.xmlhiding 79 findings — turned out to have already been fixed by the merged #2343. So the first job was to confirm that against the brief's numbers, and the second was to close what #2343 did not reach.Measured on PHPMD 2.15.0 / PHP 8.4.22, invoked exactly as
composer phpmddoes: cwd = repo root, relativelibargument. That detail is load-bearing —phpmd.baseline.xmlstores relative paths and baseline matching is an exact string match, so an absolute path argument silently matches nothing and every baselined finding reappears as a phantom. Rig positive-controlled before the first measurement: a plantedUnusedFormalParameterreported at exit 2 with correct attribution; a known-clean file exited 0.The shipped configuration is genuinely green
phpmd lib text phpmd.xml --baseline-file phpmd.baseline.xmlphpmd lib text phpmd-unusedparams.xml --baseline-file phpmd.baseline.xmlNot a dead gate: same rig, same invocation, planted probes flip it to exit 2 (below).
Confirming the 79, and that the 11 were fixed rather than baselined
lib/Migration, main ruleset, no baseline file at all:@SuppressWarningsactiveSuppressWarnings→SuppressWarninqs; every file byte-length identical, so no line number moved)Reconciled per rule against the brief's 79 and #2343's 11:
Every rule reconciles exactly. The 79 is confirmed. The 11 were genuinely fixed, not baselined —
phpmd.baseline.xmlcontains zerolib/Migrationentries, and the shipped tree exits 0 there with no baseline in play at all.The exclude pattern was still the broad form
phpmd-unusedparams.xmlcarried*/Migration/*. PDepend compiles an exclude-pattern into an unanchored regex —Input\ExcludePathFilter::__constructpreg_quote()s the pattern, then turns\*into.*— so that form matches any path containing a/Migration/segment:lib/Service/Migration/,lib/Command/Migration/, any futurelib/*/Migration/. Those are ordinary classes with no interface-mandated signature, so a real unused parameter in one would never be reported and the run would still look clean. openconnector carried exactly such a file.openregister has no
lib/*/Migration/directory today, which is precisely why this had to be fixed now: the broad form fails silently, and only on the day someone adds the directory. Now*/lib/Migration/*, matching the 19 repos already on the corrected shape, with the canonical rationale copied across.CountInLoopExpression retired entirely
All 3 baseline entries, all 3 findings — family to zero. Two are
do { … } while (count($page) === $limit)where the page is replaced wholesale each iteration and never mutated in the body, so the count is taken once per page into a variable. One isfor ($i = 1; $i < count($rings); $i++)over an array the body does not touch, so the count is hoisted. Behaviour identical in all three; the loops were re-counting on every iteration.Baseline burndown — 519 → 506, one family gone
A PHPMD baseline entry is scoped to
(rule, file)— optionally a method, never a line — so one entry covers every current and future violation of that rule in that file. It is an open licence. Shrinking the count is not the point; getting a family to zero is, because only then does a new violation of that rule fail CI.libwith no baseline in play: 3 before, 0 after.lib/Service/Flow/FlowActionService.php: 10 entries for a file that no longer exists. Deleted with the file, suppressing nothing.The other 509 entries are all live and were left alone. The first answer was wrong in an instructive way: matching baseline entries to the report by rule name reported five families — NPath (62), LongMethod (33), WeightedMethodCount (32), LongParameterList (14), LongClass (6) — as "entirely stale", 147 free deletions. They are not. The baseline stores the rule class (
PHPMD\Rule\Design\LongMethod) while the report writes the rule name (ExcessiveMethodLength), and those five differ. With the mapping applied the accounting closes exactly: 767 true findings, 767 suppressed by live entries, nothing unexplained. Uniformity across five independent families was the tell.Measuring "true" findings needs the baseline MOVED, not un-flagged
PHPMD auto-discovers
phpmd.baseline.xmlsitting next to the ruleset and applies it whether or not--baseline-fileis passed. Dropping the flag gives a silently baselined run that looks clean — I hit this and briefly had evidence that openregister's phpmd gate computed nothing at all. Deterministic, repeated 3×, identical md5 on both files:/repo/phpmd.xml, file inside/repo/tmp/copy.xml, same file/repo/phpmd.xml, baseline moved asideSo the
--baseline-fileflag incomposer.jsonis redundant, and any A/B that toggles it is comparing two identical arms. The un-baselined totals here (751 + 16 = 767) independently corroborate #2347's 749 + 16.Two generated artefacts
.phpmd.result-cache.phpuntracked + gitignored. Generated output, and a correctness hazard while committed:composer quality:phpmd-scorepasses--cache, so a stale committed cache makes PHPMD replay a verdict for code that has since changed — a gate reporting a result it never computed. Nothing in the tree references the path..coverage-baseline— left exactly as found, and whyI tried to fix this and backed it out. Worth recording precisely, because it is a race, not a number that needs correcting.
The same checked-in constant is asserted by two jobs that check opposite things:
coverage-guard.php clover.xmldevelopmentcoverage-guard.php clover.xml --update-baselinethengit diff --quietSo the committed value must equal what the tree measures at the moment the push job runs, but it is authored in a PR against a different tree — the merge base — and any unrelated merge in between moves the measurement. Measured here tonight:
developmentcommitted 58.87, its own CI recomputed 58.93 → Coverage Baseline Check red ("measured coverage has improved past it").baseline 58.93 / current 58.88 / FAIL: dropped by 0.05%— development had moved and the suite went 16030 → 16038 tests.baseline 58.87 / current 58.88→ pass.developmentmoved 2 more commits (test(e2e): guard the flow controls, the one layer that could have caught this #2353, feat(flow)!: a path ends deliberately or says it is broken, and a flow records its last run #2354), changing the measurement again.A baseline that is too low fails as hard as one too high — openconnector's file said 60.64 where CI measured 60.65, and the push job's
--update-baselinefailed on the non-emptygit diff.There is therefore no value a PR author can commit that is guaranteed correct when it lands, and widening it to make the check pass is the one thing that must not happen. The fix belongs in
coverage-guard.php: compare against the merge base (requirecurrent >= base) rather than against a checked-in constant, or let the push job commit the recomputed value instead of failing on drift. Filed here rather than chased; this PR leaves.coverage-baselinebyte-identical todevelopment.Tests
Unit suite before and after, same container and same vendor, PHPUnit 10.5.63 / PHP 8.4.22:
Byte-identical totals. 0 failures, 0 errors in both runs. The non-zero exit is pre-existing warnings/deprecations, unchanged by this PR.
phpcs on the changed
lib/files: 0 errors before, 0 after, same warning count.Not in scope here
SPDX-FileCopyrightText: 2024 Nextcloud GmbHresidue lines that chore(license): normalise licence declarations to EUPL-1.2 #2350 had wrongly relabelled EUPL-1.2. This branch originally carried its own fix for those 12; the merge takes development's version verbatim rather than duplicating it. ZeroNextcloud GmbHlines remain in any PHP file.phpmetrics-deps/is already untracked (chore: untrack phpmetrics-deps/, generated output .gitignore already lists #2352).No baseline entry, suppression, threshold, waiver,
continue-on-erroror assertion was weakened anywhere in this change.Flagged, not fixed
composer.phar(3.2 MB) andphpstan.phar(25 MB) are both tracked and neither is gitignored — 28 MB of vendored binaries in the tree, each bundling third-party code under its own licences.phpstan.pharis plausibly load-bearing for the phpstan job, so neither is touched here.