Skip to content

fix(quality): scope the Migration phpmd exclude to lib/, retire CountInLoopExpression, untrack the phpmd result cache - #2359

Merged
rubenvdlinde merged 7 commits into
developmentfrom
chore/phpmd-baseline-burndown
Aug 6, 2026
Merged

fix(quality): scope the Migration phpmd exclude to lib/, retire CountInLoopExpression, untrack the phpmd result cache#2359
rubenvdlinde merged 7 commits into
developmentfrom
chore/phpmd-baseline-burndown

Conversation

@rubenvdlinde

@rubenvdlinde rubenvdlinde commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

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> in phpmd.xml hiding 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 phpmd does: cwd = repo root, relative lib argument. That detail is load-bearing — phpmd.baseline.xml stores 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 planted UnusedFormalParameter reported at exit 2 with correct attribution; a known-clean file exited 0.

The shipped configuration is genuinely green

leg exit
phpmd lib text phpmd.xml --baseline-file phpmd.baseline.xml 0
phpmd lib text phpmd-unusedparams.xml --baseline-file phpmd.baseline.xml 0

Not 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:

exit findings
shipped tree, the 65 @SuppressWarnings active 0 0
suppressions neutralised by a length-preserving rename (SuppressWarningsSuppressWarninqs; every file byte-length identical, so no line number moved) 2 68

Reconciled per rule against the brief's 79 and #2343's 11:

rule brief (79) #2343 fixed measured now
ExcessiveMethodLength 39 2 37 39−2 ✓
NPathComplexity 14 3 11 14−3 ✓
CyclomaticComplexity 12 1 11 12−1 ✓
StaticAccess 11 2 9 11−2 ✓
ElseExpression 3 3 0 3−3 ✓
total 79 11 68 79−11 ✓

Every rule reconciles exactly. The 79 is confirmed. The 11 were genuinely fixed, not baselinedphpmd.baseline.xml contains zero lib/Migration entries, and the shipped tree exits 0 there with no baseline in play at all.

The exclude pattern was still the broad form

phpmd-unusedparams.xml carried */Migration/*. PDepend compiles an exclude-pattern into an unanchored regex — Input\ExcludePathFilter::__construct 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 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 is for ($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.

  • CountInLoopExpression: all 3 entries removed, family retired. The 3 findings were fixed in this PR, not suppressed — verified with a single-rule ruleset over all of lib with 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.xml sitting next to the ruleset and applies it whether or not --baseline-file is 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:

findings
ruleset /repo/phpmd.xml, file inside /repo 0
byte-identical copy at /tmp/copy.xml, same file 10
/repo/phpmd.xml, baseline moved aside 10
baseline restored 0

So the --baseline-file flag in composer.json is 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.php untracked + gitignored. Generated output, and a correctness hazard while committed: composer quality:phpmd-score passes --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 why

I 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:

job command fails when
on a PR coverage-guard.php clover.xml current < committed (a drop)
on a push to development coverage-guard.php clover.xml --update-baseline then git diff --quiet committed recomputed (stale — in either direction)

So 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:

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-baseline failed on the non-empty git 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 (require current >= 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-baseline byte-identical to development.

Tests

Unit suite before and after, same container and same vendor, PHPUnit 10.5.63 / PHP 8.4.22:

Tests: 16030, Assertions: 35963, PHPUnit Warnings: 1, Warnings: 8,
Deprecations: 31, PHPUnit Deprecations: 6, Skipped: 27.

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

No baseline entry, suppression, threshold, waiver, continue-on-error or assertion was weakened anywhere in this change.

Flagged, not fixed

composer.phar (3.2 MB) and phpstan.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.phar is plausibly load-bearing for the phpstan job, so neither is touched here.

…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.
@rubenvdlinde
rubenvdlinde requested a review from a team as a code owner August 5, 2026 21:17
#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.
@rubenvdlinde rubenvdlinde changed the title fix(quality): scope the Migration phpmd exclude to lib/, stop relicensing a third party's copyright, retire CountInLoopExpression fix(quality): scope the Migration phpmd exclude to lib/, retire CountInLoopExpression, untrack the phpmd result cache Aug 5, 2026
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ fae052c

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.
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ c95d508

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.
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 62352e0

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.
@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 13cc857

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.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ e698fda

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.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ e698fda

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.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

E2E: flow-controls.spec.ts is flaky — not a regression from this PR

quality / E2E Tests (Playwright) is red here and green on development, which is a reasonable reason to suspect this branch. It does not hold up. Evidence, in order of strength:

1. A documentation-only PR fails the identical spec. #2358 (docs(apphost): correct the laziness claim…) failed tests/e2e/ci/flow-controls.spec.ts:105:5 twice (23.9s, 20.7s) in run 31050141324. Documentation cannot affect flow execution. That is a control this branch does not need to explain.

2. It fails at a different assertion every time — the signature of an unstable spec, not a defect at one place:

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 155waitForURL 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 from triggerInitialImport(), its own unit test, and ContextChatReindexCommand (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.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ e54f11c

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.

@rubenvdlinde
rubenvdlinde merged commit 9504009 into development Aug 6, 2026
30 checks passed
@rubenvdlinde
rubenvdlinde deleted the chore/phpmd-baseline-burndown branch August 6, 2026 08:48
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