From ec257989324f18f19ef25e96cd23be9825783431 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Mon, 24 Aug 2026 11:50:24 +0200 Subject: [PATCH 1/2] =?UTF-8?q?fix(decidiq):=20contract=20delegation=20was?= =?UTF-8?q?=20silently=20off=20=E2=80=94=20the=20event=20namespace=20moved?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ContractApprovalService` pinned `\OCA\Decidesk\Event\DecisionRequestedEvent`. The decision app renamed to `OCA\Decidiq` with no compatibility alias, so `isDelegationConfigured()` began returning false on instances where the app was installed, and contract approvals stopped delegating. Nothing reported it, and nothing could: `class_exists` going false is exactly what an uninstalled optional app looks like, which is the case the guard was written for. The feature reads as "not configured" rather than "broken". Measured on a running instance: OCA\Decidesk\Event\DecisionRequestedEvent MISSING OCA\Decidiq\Event\DecisionRequestedEvent EXISTS The constant is now a LIST, newest first, resolved to the first that exists. The old spelling stays until no supported install ships it — dropping it would break the integration in the other direction during a staggered upgrade, which is the window this broke in to begin with. An app cannot move another app's class name; it can only follow it. So the test asserts that property rather than a literal: the list names the current namespace, tries it first, and still names the old one. Pinning one spelling fails a test now instead of silently disabling a feature later. NOT changed: `SOURCE_APP = 'softwarecatalog'`. That is this app's id AS THE DECISION APP KNOWS IT and is echoed back on the conclusion event, so it moves only when both sides move together. Found by a fleet sweep for stale cross-app namespaces after the same defect was confirmed in dossiq; openregister (→ Keepiq) and decidiq (→ Filinq) carried it too and are fixed in their own repos. --- lib/Service/ContractApprovalService.php | 43 ++++++++++++++++--- .../Service/ContractApprovalServiceTest.php | 35 +++++++++++++-- 2 files changed, 69 insertions(+), 9 deletions(-) diff --git a/lib/Service/ContractApprovalService.php b/lib/Service/ContractApprovalService.php index f8388c27..5f38f778 100644 --- a/lib/Service/ContractApprovalService.php +++ b/lib/Service/ContractApprovalService.php @@ -57,10 +57,24 @@ */ class ContractApprovalService { /** - * The fully-qualified decidesk request-event class — the in-process - * existence guard (decidesk installed + autoloaded) for delegation. + * The fully-qualified request-event class spellings, NEWEST FIRST — the + * in-process existence guard (the decision app installed + autoloaded) for + * delegation. + * + * TWO SPELLINGS because a cross-app event class name is a RUNTIME lookup + * this app can only follow, never move. The decision app renamed from + * OCA\Decidesk to OCA\Decidiq with no compatibility alias, and this constant + * named only the old one — so `isDelegationConfigured()` returned false on an + * instance where the app was installed, and contract approvals silently + * stopped delegating. Measured: OCA\Decidiq\Event\DecisionRequestedEvent + * EXISTS, OCA\Decidesk\Event\DecisionRequestedEvent MISSING. + * + * @var array */ - public const DECISION_REQUESTED_EVENT = '\\OCA\\Decidesk\\Event\\DecisionRequestedEvent'; + public const DECISION_REQUESTED_EVENTS = [ + '\\OCA\\Decidiq\\Event\\DecisionRequestedEvent', + '\\OCA\\Decidesk\\Event\\DecisionRequestedEvent', + ]; /** * This consumer app id, stamped on the request event as `sourceApp` and @@ -163,9 +177,28 @@ public function __construct( * @spec openspec/specs/contract-decision-delegation/spec.md */ public function isDelegationConfigured(): bool { - return class_exists(self::DECISION_REQUESTED_EVENT); + return ($this->resolveRequestEventClass() !== null); }//end isDelegationConfigured() + /** + * The first request-event class that actually exists, or null. + * + * Resolving rather than assuming: this app can only follow the decision + * app's namespace, and a hard-coded spelling turns a rename over there into + * a silently disabled feature over here. + * + * @return string|null The event FQN, or null when the decision app is absent. + */ + private function resolveRequestEventClass(): ?string { + foreach (self::DECISION_REQUESTED_EVENTS as $candidate) { + if (class_exists($candidate) === true) { + return $candidate; + } + } + + return null; + }//end resolveRequestEventClass() + /** * Raise a decidesk Decision for a contract approval or renewal. * @@ -209,7 +242,7 @@ public function submitForApproval(string $contractUuid, bool $isRenewal = false) $decisionType = self::DECISION_TYPE_RENEWAL; } - $eventClass = self::DECISION_REQUESTED_EVENT; + $eventClass = $this->resolveRequestEventClass(); $event = new $eventClass( self::SOURCE_APP, self::SUBJECT_REGISTER, diff --git a/tests/Unit/Service/ContractApprovalServiceTest.php b/tests/Unit/Service/ContractApprovalServiceTest.php index 4ad9e0ab..47990c58 100644 --- a/tests/Unit/Service/ContractApprovalServiceTest.php +++ b/tests/Unit/Service/ContractApprovalServiceTest.php @@ -80,18 +80,45 @@ private function makeService(): ContractApprovalService { }//end makeService() /** - * Delegation is NOT configured when decidesk's event class is absent. + * Delegation is NOT configured when NO spelling of the event class exists. * - * The decidesk app is not installed in the unit-test autoload scope, so the - * class_exists guard returns false — delegation is reported unconfigured. + * The decision app is not installed in the unit-test autoload scope, so + * every candidate misses and delegation is reported unconfigured. * * @return void */ public function testDelegationNotConfiguredWhenDecideskAbsent(): void { - $this->assertFalse(class_exists(ContractApprovalService::DECISION_REQUESTED_EVENT)); + foreach (ContractApprovalService::DECISION_REQUESTED_EVENTS as $candidate) { + $this->assertFalse(class_exists($candidate)); + } + $this->assertFalse($this->makeService()->isDelegationConfigured()); }//end testDelegationNotConfiguredWhenDecideskAbsent() + /** + * The candidate list names the CURRENT namespace and still names the old one. + * + * Pinning one spelling is what broke this: the decision app renamed to + * OCA\Decidiq without an alias, `isDelegationConfigured()` went false on an + * instance where the app was installed, and contract approvals silently + * stopped delegating. An app cannot move another app's class name — it can + * only follow it — so the property worth holding is that this LISTS the + * spellings, newest first, rather than pinning one. + * + * @return void + */ + public function testTheEventCandidatesFollowTheRename(): void { + $candidates = ContractApprovalService::DECISION_REQUESTED_EVENTS; + + $this->assertGreaterThanOrEqual(2, count($candidates)); + $this->assertStringContainsString('Decidiq', $candidates[0], 'The current namespace must be tried first.'); + $this->assertStringContainsString( + 'Decidesk', + implode(' ', $candidates), + 'The old namespace must stay listed until no supported install ships it.' + ); + }//end testTheEventCandidatesFollowTheRename() + /** * Submitting fails CLOSED (throws) when decidesk's event class is absent — * the contract is never advanced and status is never set to Actief, and the From 411ab2f077548ea1956e3b8dd0b3a0b8d029870b Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Mon, 24 Aug 2026 12:01:19 +0200 Subject: [PATCH 2/2] fix(psalm): resolve the event class ONCE, and narrow it where it is used MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Psalm rejected the previous commit: `Type null cannot be called as a class` at `new $eventClass(...)`. It was right, and the shape it caught is worth naming. The method called `isDelegationConfigured()` — which resolves the class and throws away the answer — and then resolved AGAIN at the call site with no guard. Two lookups of the same thing, only the first of them checked. The second had a `?string` reaching `new`, so an instance with no decision app installed would have gone from a clear "delegation is not available" to a fatal on a null class name. Now it resolves once, narrows, and fails closed on the null — the same refusal as before, from the value actually used. ⚠️ I did not run psalm locally before pushing; I ran phpunit, phpcs and phpstan, and phpstan does not flag this. One tool green is not the gate green, and the tools disagree by design. psalm "No errors found", phpcs 0, phpstan [OK] on the touched file. --- lib/Service/ContractApprovalService.php | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/Service/ContractApprovalService.php b/lib/Service/ContractApprovalService.php index 5f38f778..87d8ac76 100644 --- a/lib/Service/ContractApprovalService.php +++ b/lib/Service/ContractApprovalService.php @@ -225,9 +225,17 @@ private function resolveRequestEventClass(): ?string { * It is a payload discriminator, not a second responsibility. */ public function submitForApproval(string $contractUuid, bool $isRenewal = false): string { - if ($this->isDelegationConfigured() === false) { - // Fail closed — never auto-approve when decidesk is not installed. - throw new RuntimeException('Contract approval delegation is not available (decidesk event contract not installed).'); + // Resolve ONCE and narrow here, rather than calling + // isDelegationConfigured() and resolving again below. Two lookups of the + // same thing can disagree — and the second one has no guard, so a null + // would reach `new $eventClass(...)` as a call on null. Psalm caught + // exactly that shape. + $eventClass = $this->resolveRequestEventClass(); + if ($eventClass === null) { + // Fail closed — never auto-approve when the decision app is absent. + throw new RuntimeException( + 'Contract approval delegation is not available (decision event contract not installed).' + ); } $contract = $this->loadContract(contractUuid: $contractUuid); @@ -242,7 +250,6 @@ public function submitForApproval(string $contractUuid, bool $isRenewal = false) $decisionType = self::DECISION_TYPE_RENEWAL; } - $eventClass = $this->resolveRequestEventClass(); $event = new $eventClass( self::SOURCE_APP, self::SUBJECT_REGISTER,