Skip to content

fix(ci): composer phpmd was a no-op — CI reported success while printing violations - #99

Merged
rubenvdlinde merged 1 commit into
developmentfrom
chore/measure-strict-gate-2026-08-02
Aug 2, 2026
Merged

fix(ci): composer phpmd was a no-op — CI reported success while printing violations#99
rubenvdlinde merged 1 commit into
developmentfrom
chore/measure-strict-gate-2026-08-02

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

What this does

Turns composer phpmd from a no-op into a real gate, and records — in the baseline file itself — why every entry is there.

Analysis-path verification (before believing any number)

  • Every analyser was run inside nextcloud:32-apache (PHP 8.3.32, imagick present), matching the php-version: "8.3" this repo's CI pins. Host PHP is 8.2 and nextcloud:34 is PHP 8.5, where Psalm 5.26.1 crashes.
  • vendor/ was produced by a real composer install from composer.lock in a fresh worktree (11s). No sibling repo's vendor tree was borrowed, so the absolute counts below are real, not approximate.
  • The OCP-stub trap was checked explicitly: vendor/nextcloud/ocp/OCP is a real directory with 132 entries, not a dangling symlink to /var/www/html/lib/public, and there is no OCP.bak/. So no unknown class OCP\* cascade is inflating the phpstan number.
  • The configured paths exist: phpcs.xmllib, phpstan.neonlib, psalm.xmllib, phpmdlib; lib/ holds 129 PHP files. No zero here is a zero-because-nothing-was-scanned.

Does CI actually run this gate? Yes — and that is the point

.github/workflows/code-quality.yml calls the shared ConductionNL/.github/.github/workflows/quality.yml@main, whose PHP-quality matrix runs composer phpmd, composer psalm, composer phpstan, composer phpcs. So the || echo did not just weaken a local convenience — it made a CI job lie.

Proof, on development today: run 30760172028 (2026-08-02), job quality / PHP Quality (phpmd)success, while its log prints violations such as

lib/Service/AppRepoParser.php:345   CyclomaticComplexity   The method parseChannels() has a Cyclomatic Complexity of 22.

(.forgejo/workflows/pre-merge-check-strict.yaml runs only composer lint + composer phpcs; unchanged in behaviour here, but its justification comment was factually stale and is corrected.)

Pre-repair measurements

tool findings exit
phpcs 0 0
phpmd 76 (82 with no baseline) 2
psalm 0 0
phpstan 0 0
composer check:strict 0 (green-but-dead)

phpmd top rules (as the script invokes it, with --exclude + old baseline): CyclomaticComplexity 16, ElseExpression 13, NPathComplexity 11, CouplingBetweenObjects 8, ExcessiveClassComplexity 7, TooManyMethods 3, ShortVariable 3, ExcessiveMethodLength 3, ExcessiveClassLength 3, LongVariable 2, ExcessiveParameterList 2, BooleanArgumentFlag 2, UnusedFormalParameter 1, StaticAccess 1, LongClassName 1.

composer.json shape chosen: (b) plain ./vendor/bin/X

Not if [ -f vendor/bin/X ]. Three reasons:

  1. Consistency — phpcs, psalm and phpstan in this repo are already plain. phpmd was the only outlier.
  2. Every path that runs this script runs composer install first, so "the tool is genuinely absent" is not a state we need to tolerate.
  3. An if [ -f ] guard re-creates the failure mode we are removing: a missing binary would silently skip instead of failing.

Baselines and their reasons

phpmd.baseline.xml regenerated: 174 entries across 48 files. The reasoning is written into the file as an XML comment, in short:

  • Every entry is a complexity- or naming-class rule (CyclomaticComplexity 42, NpathComplexity 26, WeightedMethodCount 23, ElseExpression 16, CouplingBetweenObjects 15, LongMethod 11, LongClass 7, BooleanArgumentFlag 7, LongVariable 5, ShortVariable 4, TooManyMethods 4, StaticAccess 4, UnusedFormalParameter 3, LongParameterList 3, MissingImport 2, LongClassName 1, TooManyPublicMethods 1). None of these rules asserts incorrect behaviour — they assert that a method is long or branchy.
  • Acting on them means restructuring 48 files: a refactor with real regression risk and no defect to point at. That does not belong in a change whose purpose is to make the gate report the truth.
  • The correctness tools are separately clean on the same tree (phpcs 0, psalm 0, phpstan level 5 0), which is why baselining the mess-detector output is not hiding a defect.
  • Burn-down tracked in Burn down phpmd.baseline.xml (174 entries, 48 files) now that composer phpmd is a real gate #98. The baseline is now a ratchet — any new violation fails the gate.

No entry was added that resembles a real defect.

test:all — measured, deliberately not flipped

Left ending || echo 'Tests require Nextcloud environment, skipping...' per the fleet-wide decision. But the story is wrong and worth recording: the suite passes standaloneTests: 754, Assertions: 2277 — and PHPUnit exits 1 only because of No code coverage driver available. So this || echo is masking a coverage-driver warning, not a Nextcloud dependency, and is a candidate for removal once a driver is available on the runner. Not done here because it is a separate, measurable change.

Vacuous-green control

Before opening this PR I dropped a deliberately broken lib/ProbeGateLive.php into the tree — an undefined method call, an unused local variable, and non-conforming formatting — and confirmed the repaired analysers fail on it, including that the freshly generated baseline does not mask it (it cannot: the baseline is keyed on file+rule and the probe file is new).

tool exit with probe names the probe file
phpcs 2 yes (3 lines)
phpmd 2 yes
phpstan 1 yes
psalm 2 yes (UndefinedMethod)

The probe was then deleted and git status confirmed only the three intended files modified, with no untracked leftovers.

The control also found something. The first probe used \OCP\IRequest and psalm did not catch it — psalm never resolves the OCP stubs, and psalm.xml suppresses UndefinedClass for ~60 OCP\* names, so method calls on OCP types are unanalysable. It also suppresses InvalidReturnType, InvalidReturnStatement, InvalidArgument, InvalidCast, InvalidMethodCall, InvalidArrayOffset and more. Re-running with those handlers restored yields 17 findings. So "psalm 0/exit 0" above is honest about what the current config reports, but that config is narrower than it looks. Filed as #97 with the triage of all 17 (none is a live defect; two are docblock drift, one is a psalm false positive, one is worth a caller check). Not changed here, because widening psalm changes what the gate reports and deserves its own review.

Real defects found

None in this repo. Specifically checked for the openregister-style PHP 8 named-argument fatals: lib/ contains no class-level @method docblocks, so there is no stale annotation for phpstan/psalm to resolve named arguments against, and phpstan (level 5) resolves the named-argument call sites — e.g. ApplicationCreationService's register:/schema:/uuid: calls — against real declarations with 0 errors.

Not done / not verified

Recent merges on this repo land on GitHub (squash style, (#NN)); Codeberg is a mirror. This PR targets development, the default branch.

…ing violations

`composer phpmd` ended in

    || echo 'PHPMD reported violations beyond the baseline — ... NOT yet a hard gate.'

so it always exited 0. That was not only a local convenience: the shared
workflow ConductionNL/.github/.github/workflows/quality.yml invokes
`composer phpmd`, so the "PHP Quality (phpmd)" job reported SUCCESS while
printing the violations — run 30760172028 (2026-08-02) is green with dozens
of CyclomaticComplexity findings in its log.

Measured first, in nextcloud:32-apache (PHP 8.3.32, matches the php-version
8.3 CI pins): phpcs 0/exit 0, psalm 0/exit 0, phpstan 0/exit 0, phpmd 76
findings/exit 2 (82 with no baseline). `composer check:strict` exited 0.

- drop the `|| echo` so phpmd is a real gate, matching phpcs/psalm/phpstan
- regenerate phpmd.baseline.xml (174 entries, 48 files) with the reason for
  every family recorded in the file; burn-down tracked in #98
- correct the stale justification comment in the forgejo pre-merge workflow,
  which claimed psalm/phpstan/phpmd were all `|| echo`-wrapped

test:all is deliberately left as-is. Measured anyway: the suite passes
standalone (754 tests, 2277 assertions) and exits 1 only on a "no code
coverage driver" warning — so its "requires Nextcloud" story is wrong and
it is a candidate for removal separately.

Refs #97, #98
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openbuild @ ac17851

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 100/100
npm ✅ 662/662
PHPUnit
Newman ⏭️
Playwright ⏭️

Coverage: 89.4% (17/19 statements)


Quality workflow — 2026-08-02 19:39 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 0eafa11 into development Aug 2, 2026
30 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.

2 participants