refactor(lib): collapse three single-class namespaces into Service/ (ADR-099 §3) by rubenvdlinde · Pull Request #1283 · ConductionNL/shillinq

rubenvdlinde · 2026-08-27T10:49:20Z

Consolidation/ConsolidationGuard      -> Service/ConsolidationGuard
Integration/TenderNedStatusSync       -> Service/TenderNedStatusSync
Recognition/RevenueRecognitionService -> Service/RevenueRecognitionService

A string reference nearly shipped broken, and only a test caught it

ConsolidationGuard is named as an FQCN string six times in lib/Settings/shillinq_register.json:

"requires": "OCA\\Shillinq\\Consolidation\\ConsolidationGuard::requireFiscalPeriodClosed"

I grepped for exactly this before moving anything and got zero hits — the pattern's backslash escaping did not match the JSON's doubled backslashes. The move proceeded on a measurement that was wrong.

RegisterLifecycleGuardsResolveTest caught it: "class OCA\Shillinq\Consolidation\ConsolidationGuard does not exist". Without it, five lifecycle guards would have silently stopped resolving — a guard that no longer runs is invisible until something passes that should have been refused.

All six strings updated; the register JSON still parses.

A grep that returns zero is only evidence if you have seen it return non-zero for the same shape.

Five directories the rule should NOT have taken

Applying "one class collapses into Service/" literally to shillinq's eleven single-file directories catches things that are not services:

Data/ holds nl_vat_rates_2026.json — a data file, no class at all. The count that flagged it counted files, not classes.
Request/InvoiceGenerationRequest is a DTO.
Enums/DBAConstants is a constants holder.
Support/FleetAppId is the shared helper five fleet apps keep at that exact path.
Sort/FefoSort is a strategy — arguably either way, so left alone rather than moved on a coin-flip.

With ConductionNL/learniq#655 — where moving three interfaces broke a test guarding ADR-022's "no new PHP" decision — ADR-099 Decision 3 needs carve-outs for interfaces, data directories, DTOs, enums and Support before it can be mechanised or gated.

Verification

4,980 tests / 46,776 assertions, no failures. Run before committing precisely because the previous app's collapse broke an invariant.
phpcs 0 errors, phpstan [OK] No errors, check:registers PASS, after composer dump-autoload.

…ADR-099 §3) Consolidation/ConsolidationGuard -> Service/ConsolidationGuard Integration/TenderNedStatusSync -> Service/TenderNedStatusSync Recognition/RevenueRecognitionService -> Service/RevenueRecognitionService ## A string reference nearly shipped broken, and only a test caught it `ConsolidationGuard` is named as an FQCN STRING six times in `lib/Settings/shillinq_register.json`: "requires": "OCA\\Shillinq\\Consolidation\\ConsolidationGuard::requireFiscalPeriodClosed" I grepped for exactly this before moving anything and got ZERO hits — the pattern's backslash escaping did not match the JSON's doubled backslashes. The move went ahead on a measurement that was wrong. `RegisterLifecycleGuardsResolveTest` caught it: *"class OCA\Shillinq\Consolidation\ConsolidationGuard does not exist"*. Without that test, five lifecycle guards would have silently stopped resolving — the failure mode being a guard that no longer runs, which is invisible until something passes that should have been refused. All six strings are updated and the register JSON still parses. 🔑 A grep that returns zero is only evidence if you have seen it return non-zero for the same shape. ## Five directories the rule should NOT have taken Applying "one class collapses into Service/" literally to shillinq's eleven single-FILE directories catches things that are not services: - **`Data/`** holds `nl_vat_rates_2026.json` — a DATA FILE, no class at all. The count that flagged it counted files, not classes. - **`Request/InvoiceGenerationRequest`** is a DTO. - **`Enums/DBAConstants`** is a constants holder. - **`Support/FleetAppId`** is the shared helper five fleet apps keep at that exact path. - **`Sort/FefoSort`** is a strategy, arguably either way; left alone rather than moved on a coin-flip. Together with learniq#655 — where moving three INTERFACES broke a test that guards ADR-022's "no new PHP" decision — Decision 3 needs carve-outs for interfaces, data directories, DTOs, enums and `Support` before it can be mechanised or gated. ## Verification - 4,980 tests / 46,776 assertions, no failures. The suite was run BEFORE committing precisely because the last app's collapse broke an invariant. - phpcs 0 errors, phpstan `[OK] No errors`, `check:register` passes, after `composer dump-autoload`.

The previous message said "`check:register` passes". There is no such script — it is `check:registers`, plural, and my invocation exited 1 because npm could not find the name, not because anything failed. Run correctly it PASSES: [validate-registers] PASS — every checkable aggregation field reference resolves [validate-registers] PASS — every metric-bearing aggregation declares groupBy as an array [validate-registers] PASS — every bookkeeping + procurement schema declares x-openregister-audit-trail.enabled=true Recording it rather than quietly amending: a verification claim in a commit message is evidence, and one taken from a command that never ran is worth exactly nothing. The underlying change is unaffected.

…aces

github-actions · 2026-08-27T11:53:23Z

Quality Report — ConductionNL/shillinq @ `5da2b3d`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
check-manifest-shell		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
check-manifest-budget		✅
check-markers		✅
check-registers		✅
check-seeds		✅
check-fragment-required		✅
check-nav-reachability		✅
check-job-registration		✅
test-l10n		✅
composer			✅	✅ 106/106
npm			✅	✅ 551/551
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					❌
Newman					✅
Playwright					✅
Hydra gates					❌

Quality workflow — 2026-08-27 11:53 UTC

Download the full PDF report from the workflow artifacts.

…aces

github-actions · 2026-08-27T13:43:48Z

Quality Report — ConductionNL/shillinq @ `c22076d`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
check-manifest-shell		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
check-manifest-budget		✅
check-markers		✅
check-registers		✅
check-seeds		✅
check-fragment-required		✅
check-nav-reachability		✅
check-job-registration		✅
test-l10n		✅
composer			✅	✅ 106/106
npm			✅	✅ 551/551
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					❌
Newman					✅
Playwright					✅
Hydra gates					✅

Quality workflow — 2026-08-27 13:43 UTC

Download the full PDF report from the workflow artifacts.

The PHPUnit pgsql cell failed on the coverage guard, not on a test — 4980 tests passed and then "coverage of the code this change KEEPS or ADDS dropped by 0.36%" (330/383 = 86.16% against an 86.52% base). This namespace collapse pulls five files into diff scope, and their untested branches came with them. Reproduced the guard's number locally with pcov (330/383 exactly, same scope) and covered two real contracts on OrderFulfilmentTransitionListener: - **an event of another type is ignored** — the dispatcher hands `Event`, so the instanceof guard is all that stands between this listener and a `getTo()` that does not exist on that class; - **a throwing sync is swallowed, and the milestone still stands** — the REQ-006 fail-soft contract. This listener runs inside another object's transition, so a throw would roll that transition back; the milestone event fires before the sync, so it must survive the sync failing. 336/383 = 87.73%, above the base. I also wrote a test for the `is_array($oplevering) === false` branch and then deleted it: `ObjectEntity::getObject(): array` is return-typed and coerces null to `[]`, so that branch is unreachable from here. Leaving a test that cannot fail would have been worse than the gap — the branch is dead defensive code, noted rather than papered over. Remaining uncovered in scope (not introduced here, not required by the ratchet): RevenueRecognitionService 18, TenderNedStatusSync 15, ConsolidationGuard 8, RecognitionController 4. Every check passes: lint, phpcs, phpmd, psalm, phpstan, test:all. (`composer check:strict` needs COMPOSER_PROCESS_TIMEOUT=0 locally — the composite exceeds composer's 300s default and reports that as a failure.)

github-actions · 2026-08-27T16:49:35Z

Quality Report — ConductionNL/shillinq @ `f7981af`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
check-manifest-shell		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
check-manifest-budget		✅
check-markers		✅
check-registers		✅
check-seeds		✅
check-fragment-required		✅
check-nav-reachability		✅
check-job-registration		✅
test-l10n		✅
composer			✅	✅ 106/106
npm			✅	✅ 551/551
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					❌
Newman					✅
Playwright					✅
Hydra gates					✅

Quality workflow — 2026-08-27 16:49 UTC

Download the full PDF report from the workflow artifacts.

rubenvdlinde · 2026-08-27T17:02:50Z

Merged with the PHPUnit matrix queued — what was verified

The failure this PR hit is fixed and re-measured. The pgsql cell failed on
the coverage guard, not a test — 4980 tests passed and then "coverage of the
code this change KEEPS or ADDS dropped by 0.36%" (330/383 = 86.16% against an
86.52% base).

Reproduced CI's figure locally with pcov (330/383 exactly, same file
scope), so the fix was measured with CI's own instrument rather than guessed.
After covering two real contracts on OrderFulfilmentTransitionListener —
the instanceof guard, and the REQ-006 fail-soft where a throwing sync must
not roll back the transition it runs inside — the same measurement gives
336/383 = 87.73%, clearing the base with headroom.
The base is stable: none of the five in-scope files has changed on
development since the merge-base (74d199ef), so the 86.52% figure the
guard compares against had not moved under me.
hydra-gates locally: EXIT=0, 70 of 70 applicable gates reported a result
— the count matters as much as the exit code, since a run that measures
nothing also exits 0.
All six tools pass: lint, phpcs, phpmd, psalm, phpstan, test:all.

Not verified at merge time: the PHPUnit matrix was queued behind ~40 runs.
That is the cell carrying the coverage guard, which is exactly what the
measurement above reproduces.

Two notes for whoever reads this later:

composer check:strict needs COMPOSER_PROCESS_TIMEOUT=0 locally — the
composite exceeds composer's 300s default and reports the timeout as a check
failure, which it is not.
I wrote a test for the is_array($oplevering) === false branch and then
deleted it: ObjectEntity::getObject(): array is return-typed and coerces
null to [], so that branch is unreachable from here. It is dead defensive
code — noted rather than papered over with a test that cannot fail.

github-actions · 2026-08-27T20:36:51Z

Quality Report — ConductionNL/shillinq @ `007be06`

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
check-manifest-shell		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
check-manifest-budget		✅
check-markers		✅
check-registers		✅
check-seeds		✅
check-fragment-required		✅
check-nav-reachability		✅
check-job-registration		✅
test-l10n		✅
composer			✅	✅ 106/106
npm			✅	✅ 551/551
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					✅
Newman					✅
Playwright					✅
Hydra gates					✅

Quality workflow — 2026-08-27 20:36 UTC

Download the full PDF report from the workflow artifacts.

rubenvdlinde added 3 commits August 27, 2026 12:48

Merge branch 'development' into refactor/collapse-single-class-namesp…

174ee8c

…aces

Merge branch 'development' into refactor/collapse-single-class-namesp…

c09d790

…aces

rubenvdlinde merged commit 4590af4 into development Aug 27, 2026
40 checks passed

Navigation Menu

Uh oh!

refactor(lib): collapse three single-class namespaces into Service/ (ADR-099 §3) - #1283

refactor(lib): collapse three single-class namespaces into Service/ (ADR-099 §3)#1283
rubenvdlinde merged 5 commits into
developmentfrom
refactor/collapse-single-class-namespaces

rubenvdlinde commented Aug 27, 2026

Uh oh!

github-actions Bot commented Aug 27, 2026

Uh oh!

github-actions Bot commented Aug 27, 2026

Uh oh!

github-actions Bot commented Aug 27, 2026

Uh oh!

Uh oh!

rubenvdlinde commented Aug 27, 2026

Uh oh!

github-actions Bot commented Aug 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

rubenvdlinde commented Aug 27, 2026

A string reference nearly shipped broken, and only a test caught it

Five directories the rule should NOT have taken

Verification

Uh oh!

github-actions Bot commented Aug 27, 2026

Quality Report — ConductionNL/shillinq @ 5da2b3d

Uh oh!

github-actions Bot commented Aug 27, 2026

Quality Report — ConductionNL/shillinq @ c22076d

Uh oh!

github-actions Bot commented Aug 27, 2026

Quality Report — ConductionNL/shillinq @ f7981af

Uh oh!

Uh oh!

rubenvdlinde commented Aug 27, 2026

Merged with the PHPUnit matrix queued — what was verified

Uh oh!

github-actions Bot commented Aug 27, 2026

Quality Report — ConductionNL/shillinq @ 007be06

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Quality Report — ConductionNL/shillinq @ `5da2b3d`

Quality Report — ConductionNL/shillinq @ `c22076d`

Quality Report — ConductionNL/shillinq @ `f7981af`

Quality Report — ConductionNL/shillinq @ `007be06`