Skip to content

refactor(settings): extract readEffectiveConfiguration() — clears the last phpmd finding - #725

Merged
rubenvdlinde merged 1 commit into
developmentfrom
chore/shorten-load-configuration
Aug 4, 2026
Merged

refactor(settings): extract readEffectiveConfiguration() — clears the last phpmd finding#725
rubenvdlinde merged 1 commit into
developmentfrom
chore/shorten-load-configuration

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Context

Follow-up to #722. That PR cleared AiService TooManyPublicMethods, which was the only phpmd finding at the time. Re-measuring on the merged development (773a8bfc4) turned up a different single finding, introduced by #721 while #722 was open:

lib/Service/SettingsService.php:531  ExcessiveMethodLength
The method loadConfiguration() has 102 lines of code. Current threshold is set to 100.

#721 replaced the +frag.<md5> version suffix with a long explanatory comment, which pushed the method 2 lines over. This PR clears it, so phpmd on development reaches 0 findings.

How

loadConfiguration() was doing two jobs:

  1. resolve the effective register configuration — read procest_register.json, parse it, deep-merge the ADR-037 register.d/*.json fragments
  2. import it via OpenRegister's ConfigurationService

Job 1 moves into a private readEffectiveConfiguration() returning ['data' => …] on success or ['error' => …] carrying the caller-facing failure shape. loadConfiguration() stays a single import flow instead of also being a file reader.

Behaviour is unchanged — the same two failure shapes (Configuration file not found, Invalid JSON in configuration file) are returned to the caller in the same order, with the same log calls.

Test gap this exposed

The extracted path had no test at all. The only loadConfiguration test covers the OpenRegister-unavailable early return — which returns before any of the read/parse/merge code runs. So the whole path could have been broken (wrong file, unparsed JSON, fragments silently dropped) with the suite still green.

Added a test that drives loadConfiguration() through to importFromApp() and asserts it receives:

Mutation-checked: making readEffectiveConfiguration() return an empty config turns it red (0.13.20.0.0).

Verification

Worktree pinned to origin/development @ 773a8bfc4, phpmd over 597 files in lib/, PHP 8.4 container (host PHP is 8.2 and dies in platform_check.php with exit 255, which greps as a clean run).

before after
phpmd exit 2, 1 finding exit 0, 0 findings
positive control (injected probe) exit 2, 3 probe findings exit 2, 3 probe findings

Findings were matched on the path:line Rule message shape — phpmd also emits 13 Deprecated: PDepend\... lines naming rule classes, and a rule-name grep would manufacture phantom hits from those.

composer check:strict, real exit codes:

step before after
lint 0 0
phpcs 0 0
phpmd 2 0
psalm 0 0
phpstan 0 0
phpunit 0 — 1691 tests, 5649 assertions 0 — 1692 tests, 5659 assertions

PHPUnit was run under Xdebug on purpose

phpunit.xml sets beStrictAboutCoverageMetadata="true" + failOnRisky="true". That check is completely inert without a coverage driver — on #722 it caught nothing locally and then failed all 4 CI legs with 7 risky tests. PCOV does not drive it either; CI uses Xdebug. Running under Xdebug here flagged a missing @uses RegisterFragmentMerger immediately, before pushing.

Not done

  • No @SuppressWarnings, @psalm-suppress, phpcs:ignore, phpstan-ignore, markTestSkipped or .skip( added
  • No baseline file created
  • phpmd.xml, phpcs.xml, psalm.xml, phpstan.neon, phpunit.xml, composer.json untouched — the threshold stays at 100

…figuration()

phpmd on development flagged the last remaining finding:

  lib/Service/SettingsService.php:531  ExcessiveMethodLength
  The method loadConfiguration() has 102 lines of code. Current threshold
  is set to 100.

It crossed the line in #721, which replaced the `+frag.<md5>` version suffix
with a long explanatory comment.

loadConfiguration() was doing two jobs: resolving the effective register
configuration (read procest_register.json, parse it, deep-merge the ADR-037
register.d fragments) and importing it. Extract the first into a private
readEffectiveConfiguration() returning ['data' => …] or ['error' => …], so
loadConfiguration() stays a single import flow. Behaviour is unchanged — the
same two failure shapes are returned to the caller in the same order.

The extracted path had NO test: the only loadConfiguration test covers the
OpenRegister-unavailable early return, which returns before any of this runs,
so read/parse/merge could have broken silently. Add a test driving
loadConfiguration() through to importFromApp, asserting it receives the
on-disk version and parsed content, and that the version carries no `+`
build suffix (the #721 invariant). Mutation-checked: returning an empty
config array turns it red (0.13.2 -> 0.0.0).

phpmd lib/ (597 files): exit 2 / 1 finding -> exit 0 / 0 findings.
Positive-controlled both arms (injected probe yields 3 findings, exit 2).
check:strict green: lint 0, phpcs 0, phpmd 0, psalm 0, phpstan 0,
phpunit 0 (1691 -> 1692 tests, 5649 -> 5659 assertions).

PHPUnit was run under Xdebug, because phpunit.xml sets
beStrictAboutCoverageMetadata + failOnRisky and that check is INERT without a
coverage driver — it silently caught nothing locally on #722 and only fired in
CI. It flagged the missing @uses RegisterFragmentMerger here immediately.
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/procest @ 8f108ce

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

Quality workflow — 2026-08-04 10:24 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Merging. This takes procest from red to fully green — measured at job level, not from the rollup:

job development this PR
PHP Quality (phpmd) failure success
Quality Report failure success
PHPUnit ×4 legs success success
total 29 jobs, 2 failing 29 jobs, 0 failing

phpmd had been procest's only blocker for the whole session — a single finding, with Quality Report red purely because phpmd was. +159/−40 across 2 files clears it.

Worth noting the seam chosen here is not the one I suggested. I proposed extracting the three audit methods out of AiService; this extracts readEffectiveConfiguration() instead. The measured result is what matters, and it is green — including all four PHPUnit legs, so the refactor is behaviour-preserving where it counts.

No suppression, no baseline, no threshold change — the finding is cleared, not silenced.

@rubenvdlinde
rubenvdlinde merged commit 0c6d87b into development Aug 4, 2026
33 checks passed
@rubenvdlinde
rubenvdlinde deleted the chore/shorten-load-configuration branch August 4, 2026 10:26
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