Skip to content

fix(quality): decompose Application bootstrap, Stuf and the oversized services (9 phpmd findings) - #717

Merged
rubenvdlinde merged 6 commits into
developmentfrom
quality/phpmd-zero-p3
Aug 4, 2026
Merged

fix(quality): decompose Application bootstrap, Stuf and the oversized services (9 phpmd findings)#717
rubenvdlinde merged 6 commits into
developmentfrom
quality/phpmd-zero-p3

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Clears 8 of the 9 structurally-hardest PHPMD architectural findings in procest by decomposition — no @SuppressWarnings added, no baseline created or extended, no threshold loosened in phpmd.xml. Two pre-existing suppressions that decomposition made unnecessary were removed, and one dead one was deleted.

Repo-wide phpmd: 25 → 17 findings, i.e. exactly the 8 cleared and zero new findings anywhere in lib/ (verified by diffing the full finding list against the baseline — see Measured results).


Before → after, per finding

file rule before after
lib/AppInfo/Application.php CouplingBetweenObjects 92 / 13 cleared
lib/Repair/SeedVthWorkflowTemplates.php ExcessiveClassComplexity 85 / 50 cleared
lib/Controller/StufController.php ExcessiveClassComplexity 81 / 50 cleared
lib/Service/SettingsService.php ExcessiveClassLength 1356 / 1000 cleared (810 lines)
lib/Service/AiService.php ExcessiveClassLength 1265 / 1000 cleared (~845 lines)
lib/Service/AiService.php TooManyPublicMethods 12 / 10 ⚠️ 12 — NOT cleared, see below
lib/Service/StufMessageBuilder.php TooManyPublicMethods 14 / 10 cleared (8)
lib/Service/Stuf/StufAdapterService.php ExcessiveClassComplexity 51 / 50 cleared
lib/Service/Stuf/StufAdapterService.php CouplingBetweenObjects 15 / 13 cleared

New classes and what each owns

lib/AppInfo/Registrar/ — 21 classes (Application coupling 92 → cleared)

Application is a Nextcloud bootstrap class: its register()/boot() named ~90 listener, widget, adapter, middleware and service classes directly. Every registration now lives in a dedicated registrar, so each subsystem's class references sit with that subsystem instead of accumulating on the bootstrap class. Application is left with the three phases and five framework references.

Composites: ServiceRegistrar, ListenerRegistrar, BootRegistrar, ExternalRegisterRegistrar.
Leaves: AppHostRegistrar (OpenRegister AppHost + the 7 dashboard widgets + MCP provider) · BespokeServiceRegistrar (the 6 re-asserted Settings/Dashboard plumbing factories) · MiddlewareRegistrar (the ordered SaaS chain — order is behaviour, and is now reviewable on one screen) · SaasServiceRegistrar (the two config-string-constructed services) · BeschikkingAdapterRegistrar · AuthAdapterRegistrar · one registrar per external base register (KvkRegistrar, BrpRegistrar, BagRegistrar, BrkRegistrar, WozRegistrar) so each binding's fail-closed default is auditable on its own · ExternalZgwRegistrar · ObjectListenerRegistrar · BezwaarListenerRegistrar · WorkflowListenerRegistrar · BezwaarSubscriptionRegistrar (boot-time, keeps the register()/boot() split and its reasoning) · MapCspRegistrar.

lib/Repair/Vth/ — 3 classes (SeedVthWorkflowTemplates 85 → cleared)

  • VthSeedLookup — the three OpenRegister reads (caseType resolution, idempotency probe, statusType map) plus the system-principal elevation, behind one soft-fail query() seam so "missing service", "unconfigured schema" and "search threw" collapse to the same empty result in one place.
  • VthSeedRowReader — result-row coercion (plain array / {results:[…]} envelope / ObjectEntity).
  • VthWorkflowGraphResolver — the steps/transitions translation and the deterministic UUID5 ids.

SeedVthWorkflowTemplates keeps orchestration only.

lib/Service/Stuf/ — 6 classes (StufController 81, StufMessageBuilder 14, StufAdapterService 51+15 → all cleared)

Three findings, one shape: single classes owning both directions of StUF-ZKN/BG at once.

  • StufResponseBuilder — the inbound half of StufMessageBuilder (SOAP envelope, stuurgegevens, Bv01, Fo01, SOAP Fault). The two halves share no caller — the controller only ever answers, the adapter only ever asks — and not even an XML style (DOMDocument stuf: vs string-concatenated zkn:). StufMessageBuilder keeps the outbound builders and remains the canonical home of the NS_* constants that StufMessageParser, StufController and StufResponseBuilder all read.
  • StufSoapRequestDispatcher — size ceiling, XXE-safe parsing, locating the StUF message element; every refusal answered with a SOAP Fault rather than an HTTP error page, because the caller only speaks SOAP.
  • StufZknMessageResponder — the per-message-type responses (zakLk01 / zakLv01 / npsLv01 / edcLk01 / unknown).
  • StufEnvelopeInspector — the raw-envelope reads the async-confirmation webhook needs (endpoint identity, WSSE verification, bericht-soort / crossRef / functie), behind one firstMatch() helper that also collapses five identical PHPMD.UndefinedVariable suppressions into one.
  • StufOutboundTransport — everything that happens after an envelope is built and logged: send, 2xx bevestiging, Fo02/transport-fout classification, circuit-breaker bookkeeping, retry scheduling and the permanent-failure needs-input signal. The backoff schedule moves with it, next to the only code that reads it.
  • StufCaseMappingStore — the ZaaksysteemMapping row and the (bronEntiteit, bronId, endpointId) identity triple that was previously spelled out at three call sites.

lib/Service/Ai/ — 4 classes (AiService 1265 lines → cleared)

  • AiPromptFactory — the six prompt templates, so the wording of what we ask a model and the JSON shape we demand back are reviewable as a set.
  • AiPiiRedactor — the ONE definition of deterministically-detectable PII plus both of its consumers (span reporting for human review, prompt scrubbing), so the two can never drift apart on which patterns count as PII.
  • AiEndpointGuard — the SSRF decision: CIDR deny-list, cloud/local rules, IPv4 and IPv6 range arithmetic.
  • AiAuditLog — the Algoritmeregister oversight trail, with the write and the read resolving the same register/schema config in one place and degrading in one place.

AiService keeps orchestration and the single outbound model call; it no longer needs the SearchesObjects or SuppressesWarnings traits, nor the DI container.

lib/Service/Settings/ — 4 classes (SettingsService 1356 lines → 810)

  • RegisterFragmentMerger — the ADR-037 deep-merge and the fragment-set hash that forces a re-import. A pure data transformation, so it is now instance-based and public: the six fragment test suites that reached it through ReflectionMethod on a private static now just call it.
  • SchemaSlugMap — the slug → appconfig-key table, the owned x-openregister-* annotation block names, and the workflow_definition_schema alias constants that were previously repeated as string literals at two call sites.
  • SchemaKeyReconciler — both paths that write a *_schema key: the ConfigurationService import result, and the direct SchemaMapper slug lookup (the path that exists because an idempotent re-import returns an empty schemas list, which silently broke the status-name lookup and the WorkflowBoard on a fresh deploy).
  • SchemaAnnotationReconciler — the declarative x-openregister-* merge onto live schemas.

Public API and the (appConfig, appManager, container, logger) constructor signature are unchanged, so the bespoke factory and the ~180 injection sites are untouched; the collaborators are constructed internally.


⚠️ Not cleared: AiService TooManyPublicMethods (12 / 10)

Residual: 12, unchanged. Reported rather than suppressed.

PHPMD ignores getter-shaped names (^(set|get|is|has|with)), so the 12 it counts are __construct plus classifyDocument, extractData, askQuestion, summarize, suggestRouting, suggestNextStep, recordUserAction, listAuditEntries, testHealth, detectDeterministicPiiSpans, recordAssistantAuditEntry. Reaching ≤ 9 requires removing three of them from the class, not just moving their bodies.

The decomposition is already done — AiAuditLog and AiPiiRedactor exist and hold the real logic — but every one of those methods has callers outside the file set this change was scoped to, and this branch ran concurrently with other procest quality branches. The remaining step is:

move to call sites to update
recordUserAction AiAuditLog lib/Controller/AiController.php
listAuditEntries AiAuditLog::list() lib/Controller/AiController.php, lib/Controller/AiAuditExportController.php (+ AiControllerAuditIndexTest, AiServiceAuditListTest)
recordAssistantAuditEntry AiAuditLog::record() lib/Service/Assistant/CaseAssistantService.php, lib/Service/WOOAnonymisationAssistService.php

That is a coherent single story — the audit trail is a service, not a method bag on AiService — and it lands 12 → 9. It is left as a follow-up so this branch does not edit files another concurrent branch may hold. (detectDeterministicPiiSpans can also move to AiPiiRedactor::detectSpans(), callers HermiqAnonymisationClient + WOOAnonymisationAssistService, if more headroom is wanted.)


Suppressions

None added. Three pre-existing ones were touched:

  • SeedVthWorkflowTemplates — class-level PHPMD.CouplingBetweenObjects removed; verified by measuring with it gone.
  • SettingsService — class-level PHPMD.ExcessiveClassComplexity removed; verified by measuring with it gone.
  • StufAdapterService — its PHPMD.CouplingBetweenObjects sat in the file docblock, not the class docblock, so PHPMD never applied it. That is exactly why the coupling finding was reported despite it being there. Deleted as dead; the coupling is decomposed away rather than suppressed.

StufController keeps its pre-existing class-level CouplingBetweenObjects suppression: measured with it removed the class still reads 14/13, and that finding was not in scope here.


Behaviour changes (deliberate, called out)

  1. Duplicate listener registration dropped. VergunningaanvraagCreatedListener was registered twice on ObjectCreatedEvent in Application::register(), under two different comments — so it ran twice per created object. Now registered once.
  2. AiService::listAuditEntries() log level. Unchanged. (The VTH seed's statusType-listing failure kept its error level explicitly, via a PSR-3 level parameter on the new shared query() seam, rather than being flattened to debug with the other soft-fails.)

Everything else is behaviour-preserving: public APIs, constructor signatures visible to the container, and log messages are unchanged.

Test changes

  • BezwaarLegalHoldSchemaCoverageTest — the schema-coverage assertion followed the subscription filter into BezwaarSubscriptionRegistrar and now reads the named constant by reflection instead of regex-scraping Application.php. Added a negative control asserting the guarded constant is the list actually handed to the dispatcher, so the coverage assertion cannot pass against a constant nothing reads.
  • StufMessageBuilderOutboundTest — inbound assertions retargeted at StufResponseBuilder and widened to cover Fo01 and stuurgegevens, plus a new test asserting the inbound builders are gone from StufMessageBuilder rather than duplicated.
  • AI suites wire the real collaborators against the existing mocked boundaries rather than stubbing them, so AiServiceAuditLoggingCompletenessTest still proves an audit entry is actually written and AiServicePiiDetectionTest still exercises the real pattern set.
  • Six fragment suites drop ReflectionMethod in favour of the now-public RegisterFragmentMerger::merge().

Measured results

Run from the worktree root; phpmd is exactly what composer phpmd (and therefore CI) runs. Host PHP is 8.2 and the repo needs ≥ 8.3, so everything runs in a containervendor/bin/* on the host exits 255 and prints nothing, which is indistinguishable from "clean".

docker run --rm -v "$PWD":/app -w /app php:8.3-cli php vendor/bin/phpmd lib text phpmd.xml
  → exit=2  findings=17  stderr_bytes=0     (baseline on development: exit=2 findings=25)

docker run --rm -v "$PWD":/app -w /app php:8.3-cli php vendor/bin/phpcs --standard=phpcs.xml
  → exit=0

docker run --rm -v "$PWD":/app -w /app php:8.3-cli php vendor/bin/phpstan analyse --memory-limit=1G
  → exit=0   No errors

docker run --rm -v "$PWD":/app -w /app nextcloud:latest php vendor/bin/psalm --threads=1 --no-cache
  → exit=0   No errors found

docker run --rm -v "$PWD":/app -w /app nextcloud:latest php vendor/bin/phpunit --no-coverage
  → exit=0   Tests: 1686, Assertions: 5632, Failures: 0, Deprecations: 10, Skipped: 5

Notes on tooling:

  • psalm and phpunit run on nextcloud:latest, not php:8.3-cli — the latter lacks ext-zip and manufactures 25 spurious Class ZipArchive does not exist errors / 4 test errors that have nothing to do with this change.
  • phpunit baseline was 1684 tests / 0 failures; this branch is 1686 / 0 — the two extra tests are the negative controls added above.
  • No new finding was introduced anywhere in lib/: diffing the full finding list against the baseline shows 8 lines removed and 0 added. The phpmd command was re-run after every structural batch specifically because removing an else can raise NPath complexity (sequential guards multiply where if/else adds).
  • phpstan.neon was not touched: VthWorkflowGraphResolver's raw catalog params are now typed array<int, mixed> — which is what json_decode() actually yields — so the is_array() guards are real checks rather than PHPDoc-contradicting dead comparisons, and the ignoreErrors entry they used to need is no longer load-bearing.

🤖 Generated with Claude Code

…strars

Application had a CouplingBetweenObjects value of 92 (threshold 13): its
register()/boot() named ~90 listener, widget, adapter, middleware and service
classes directly.

Every registration now lives in a dedicated registrar under
lib/AppInfo/Registrar/, so each subsystem's class references sit with that
subsystem. Application keeps only the three phases (bind services, wire
listeners, boot) and is down to five framework references.

Also drops an accidental duplicate registration of
VergunningaanvraagCreatedListener on ObjectCreatedEvent — it was registered
twice under two different comments, so it ran twice per created object.

phpmd: lib/AppInfo/Application.php CouplingBetweenObjects 92 -> cleared; repo
total 25 -> 24 findings, no new finding anywhere in lib/.
… resolver and orchestration

SeedVthWorkflowTemplates had an overall class complexity of 85 (threshold 50):
one class owned the catalog walk, every OpenRegister read, the result-row shape
coercion and the steps/transitions translation.

- VthSeedLookup owns the three OpenRegister reads (caseType resolution,
  idempotency probe, statusType map) plus the system-principal elevation, with
  one soft-fail query() seam so 'missing service', 'unconfigured schema' and
  'search threw' all collapse to the same empty result in one place.
- VthSeedRowReader owns the result-row coercion (array / {results:[...]} /
  ObjectEntity).
- VthWorkflowGraphResolver owns the steps/transitions translation and the
  deterministic UUID5 ids.
- SeedVthWorkflowTemplates keeps orchestration only.

The pre-existing class-level @SuppressWarnings(PHPMD.CouplingBetweenObjects) is
removed: with the reads behind a seam the class no longer needs it (verified by
measuring with the suppression gone).

Also drops the phpstan ignoreErrors dependency for this code: the raw catalog
step/transition params are now typed array<int, mixed> — which is what
json_decode() actually yields — so the is_array() guards are real checks rather
than PHPDoc-contradicting dead comparisons.

phpmd: SeedVthWorkflowTemplates ExcessiveClassComplexity 85 -> cleared; repo
total 24 -> 23 findings, no new finding anywhere in lib/.
… dispatcher, responder, inspector

Three findings on the StUF stack came from one shape: single classes owning
both directions of StUF-ZKN/BG at once.

StufMessageBuilder (TooManyPublicMethods 14/10) owned inbound responses AND
outbound requests. The two halves share no caller — the controller only ever
answers, the adapter only ever asks — and not even an XML style (DOMDocument
'stuf:' vs string-concatenated 'zkn:'). The inbound half moves to
Service/Stuf/StufResponseBuilder; the outbound half keeps the class and the
canonical NS_* constants that StufMessageParser and StufController read from.

StufController (ExcessiveClassComplexity 81/50) owned the inbound SOAP path,
the async-confirmation webhook's envelope introspection and the admin REST
surface:
- StufSoapRequestDispatcher owns size ceiling, XXE-safe parsing and locating
  the StUF message element, answering every refusal with a SOAP Fault.
- StufZknMessageResponder owns the per-message-type responses.
- StufEnvelopeInspector owns the raw-envelope reads the webhook needs
  (endpoint identity, WSSE verification, bericht-soort / crossRef / functie),
  behind one firstMatch() helper that also collapses five identical
  PHPMD.UndefinedVariable suppressions into one.

StufController keeps its pre-existing class-level CouplingBetweenObjects
suppression: measured with it removed the class still reads 14/13, and that
finding was not in scope here.

Tests: StufMessageBuilderOutboundTest's inbound assertions now target
StufResponseBuilder, widened to cover Fo01 and stuurgegevens, plus a new
negative control asserting the inbound builders are GONE from
StufMessageBuilder rather than duplicated.

phpmd: StufController 81 -> cleared, StufMessageBuilder 14 public methods ->
8, repo total 23 -> 21 findings, no new finding anywhere in lib/.
…and mapping store

StufAdapterService carried both findings at once: overall complexity 51 and a
coupling of 15. It built envelopes, sent them, classified every possible answer,
drove the circuit breaker, scheduled retries and owned the case-to-zaak mapping.

- StufOutboundTransport owns everything that happens AFTER an envelope is built
  and logged: send, 2xx bevestiging, Fo02/transport fout classification, retry
  scheduling and the permanent-failure needs-input signal. The backoff schedule
  moves with it, next to the only code that reads it.
- StufCaseMappingStore owns the ZaaksysteemMapping row and the identity triple
  (bronEntiteit, bronId, endpointId) that used to be spelled out at three call
  sites.
- StufAdapterService keeps WHAT to send and what to report back.

Removes a DEAD suppression: the @SuppressWarnings(PHPMD.CouplingBetweenObjects)
sat in the FILE docblock, not the class docblock, so PHPMD never applied it —
which is exactly why the coupling finding was reported despite it being there.
The coupling is now decomposed away rather than suppressed.

Public surface is unchanged (creeerZaak / actualiseerZaak / geefZaakDetails /
vrijBericht / genereerZaakIdentificatie / retrySend / RETRY_BACKOFF_SECONDS).

phpmd: StufAdapterService complexity 51 -> cleared and coupling 15 -> cleared;
repo total 21 -> 19 findings, no new finding anywhere in lib/.
…endpoint guard and audit log

AiService was 1265 lines (threshold 1000). It orchestrated AI calls AND owned
four self-contained concerns:

- AiPromptFactory: the six prompt templates, so the wording of what we ask a
  model and the JSON shape we demand back are reviewable as a set.
- AiPiiRedactor: the ONE definition of deterministically-detectable PII plus
  both of its consumers (span reporting and prompt scrubbing), so detection and
  scrubbing can never drift apart on which patterns count.
- AiEndpointGuard: the SSRF decision — CIDR deny-list, cloud/local rules, IPv4
  and IPv6 range arithmetic.
- AiAuditLog: the Algoritmeregister oversight trail, with the write and the read
  resolving the same register/schema config in one place and degrading in one
  place.

AiService keeps orchestration and the single outbound model call. It no longer
needs the SearchesObjects or SuppressesWarnings traits, nor the DI container.

Tests wire the REAL collaborators against the existing mocked boundaries rather
than stubbing them out, so AiServiceAuditLoggingCompletenessTest still proves an
audit entry is actually written and AiServicePiiDetectionTest still exercises
the real pattern set.

phpmd: AiService ExcessiveClassLength 1265 -> cleared; repo total 19 -> 18.
TooManyPublicMethods (12/10) is NOT cleared — see the PR body.
…two reconcilers

SettingsService was 1356 lines (threshold 1000). Alongside the settings CRUD it
owned the ADR-037 fragment merge, the schema-slug data table, and both schema
reconcilers.

- RegisterFragmentMerger owns the ADR-037 deep-merge and the fragment-set hash
  that forces a re-import. It is a pure data transformation, so it is now
  instance-based and PUBLIC — the six fragment test suites that used to reach it
  through ReflectionMethod on a private static now just call it.
- SchemaSlugMap holds the slug -> appconfig-key table and the owned
  x-openregister-* annotation block names, plus the workflow_definition_schema
  alias constants that were previously repeated as string literals at two call
  sites.
- SchemaKeyReconciler owns both paths that write a *_schema key: the import
  result and the direct SchemaMapper slug lookup.
- SchemaAnnotationReconciler owns the declarative x-openregister-* merge onto
  live schemas.

Public API and the (appConfig, appManager, container, logger) constructor
signature are unchanged, so the bespoke factory and the ~180 injection sites are
untouched; the collaborators are constructed internally.

The pre-existing class-level @SuppressWarnings(PHPMD.ExcessiveClassComplexity)
is removed — verified by measuring with it gone.

phpmd: SettingsService ExcessiveClassLength 1356 -> cleared (1356 -> 810 lines);
repo total 18 -> 17 findings, no new finding anywhere in lib/.
@rubenvdlinde
rubenvdlinde merged commit 24b79f7 into development Aug 4, 2026
24 of 25 checks passed
@rubenvdlinde
rubenvdlinde deleted the quality/phpmd-zero-p3 branch August 4, 2026 01:50
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/procest @ 81321bb

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 01:53 UTC

Download the full PDF report from the workflow artifacts.

rubenvdlinde pushed a commit that referenced this pull request Aug 4, 2026
… pairs

The first PR run was still red — 223 risky had dropped to 49, not 0 — and the
reason was a stale base, not a wrong fix. A `pull_request` workflow builds
`refs/pull/N/merge`, i.e. this branch merged into the CURRENT development. Five
PRs (#712 #713 #714 #715 #717, plus #709) landed while this was in flight, and
they split several services into new sub-namespaces. CI was therefore measuring
a tree with collaborators that did not exist on the base this branch was cut
from — Service\Relation\*, Service\Sharing\*, Service\Transfer\*,
Service\Email\*, Service\Settings\*, Service\Ai\*, Service\Cmmn\CasePlanRepository,
PlanItemCascade, PlanItemStateMachine, PlanItemTree, Consultation\*,
Zaakdossier\InformatieobjectStatusLifecycle, Beschikking\LibresignResultAssembler.

Rebased onto 53670a0 and re-measured from scratch. 41 residual pairs across
21 test files, declared here. The measurement now agrees across all three
sources: the per-file isolated sweep and the full-suite run produced an
identical 41 pairs, and every one of CI's 20 was contained in them — the
CI-only set is empty this round, which confirms the earlier local/CI divergence
was entirely the stale base and not an environment difference.

Tests: 1686, Assertions: 5632, Skipped: 5, Risky: 82 -> 0 locally. Exit 0.
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