Skip to content

fix: accept the concluded-decision event under both of the decision app's namespaces - #1015

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/stale-fleet-namespace-lookups
Sep 9, 2026
Merged

fix: accept the concluded-decision event under both of the decision app's namespaces#1015
rubenvdlinde merged 1 commit into
developmentfrom
fix/stale-fleet-namespace-lookups

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

What this is

The decision app renamed its PSR-4 root from OCA\Decidesk to OCA\Decidiq with no compatibility alias. This repo's outbound half already carries both spellings (ContractApprovalService::DECISION_REQUESTED_EVENTS, with a comment explaining exactly why). The inbound half did not, and that is the half that closes the loop.

file:line old name decidiq now target exists guard done
lib/AppInfo/Application.php:23 + :812 use OCA\Decidesk\Event\DecisionConcludedEvent registered as ::class OCA\Decidiq\Event\DecisionConcludedEvent yes silent, ::class resolves at compile time to a string nothing dispatches registers both spellings, class_exists-guarded
lib/EventListener/DecisionConcludedListener.php:33 + :74 same import, used for instanceof same yes silent, instanceof false, method returns as if the event were somebody else's accepts both spellings

Two independent failures on the same path, either one sufficient. Both are silent by construction: an event with no listener and a listener on no event are indistinguishable from inside this app. The visible symptom is a contract approved in the decision app that never leaves In onderhandeling, with nothing thrown and nothing logged.

Target verification

Read from decidiq development d72839c, not inferred. OCA\Decidiq\Event\DecisionConcludedEvent exists and publishes the whole getter surface this listener reads: getSourceApp(), getSubjectId(), getExternalReference(), getDecisionId(), getStatus(). OCA\Decidesk\Event\DecisionConcludedEvent is gone from that repo.

The old spelling is kept anyway. An instance pinned to a release from before that rename still dispatches it, and dropping it here re-breaks the integration in the other direction. Order is the contract: newest first.

Fix shape

No new resolver class, and no port of dossiq's FleetAppId. This repo already had the pattern in the right place: one FQN-string constant per event, on the service that owns the delegation boundary. The conclusion event gets the sibling constant next to the request one, so the two halves of the same boundary are declared together and a future rename is one edit. Porting a Support\FleetAppId here would have added a second way to say the same thing, plus a namespace map for nine apps stackiq has no binding to, and its id half would have had no caller at all.

instanceof against a class that is not installed is simply false: it neither autoloads nor errors. So naming both spellings costs nothing on an instance that runs only one, which is why the listener can be typed against both imports rather than duck-typed through method_exists.

SOURCE_APP stays stackiq. It is persisted on the decision app's Decision rows when an approval is raised, and the listener filters inbound events against it. Its existing docblock already says so.

Static analysis

tests/analysis-stubs/decidiq-events.stub.php is new, the sibling of the decidesk-events.stub.php that was already here, registered with both phpstan (scanFiles) and psalm (<stubs>). Signatures mirror the real class rather than this app's assumptions about it: a stub written from the call site agrees with the call site by construction and cannot fail. It stays out of the tests/Stubs/ glob for the reason its decidesk sibling documents, so it never shadows a real event at runtime.

Tests

DecisionConcludedListenerNamespaceTest covers three things: the outcome is projected for an event dispatched under either spelling, another consumer's decision is still ignored (widening the accepted classes must not widen the sourceApp filter), and the registration list carries both spellings newest first.

Mutation-checked, twice. Narrowing handle() back to the single instanceof fails with "the listener ignored a real conclusion event dispatched as OCA\Decidiq\Event\DecisionConcludedEvent". Dropping the Decidiq entry from the constant fails the ordering assertion. Restored, 3/3 green.

The event doubles live in tests/Unit/EventListener/decision-conclusion-event-doubles.php and load the two nextcloud/ocp base declarations when nothing else has. That package ships them as real, self-contained classes but declares no autoload section, so in pure-unit mode handle(Event $event) could not be called at all and the one test that proves this fix would have had to markTestSkipped — the same as not having it. Every declaration is guarded, so on a booted Nextcloud the real classes win and the file is inert.

Checks

lint, phpcs, phpmd, psalm and phpstan all exit 0.

test:all cannot build locally, before this branch as well as after: MigrateAppConfigKeysTest aborts the whole suite with Interface "OCP\Migration\IRepairStep" not found because this checkout is not inside an installed Nextcloud root. Confirmed identical on unmodified development. The new test was therefore run directly against tests/bootstrap.php, which is also how it was mutation-checked; CI boots a real root and runs it with everything else.

Note for whoever owns gate-114

It still reports DecisionConcludedListener.php:33. That line is now one half of a two-line aliased import pair, use OCA\Decidiq\… as Decidiq… directly above use OCA\Decidesk\… as Decidesk…, which is a dual-spelling list in every sense except that the statement-scoped exclusion cannot see across two statements. A use pair is probably worth treating the way an array literal is.

…pp's namespaces

The decision app renamed its PSR-4 root from OCA\Decidesk to OCA\Decidiq with no
compatibility alias. Stackiq's OUTBOUND half already carries both spellings
(ContractApprovalService::DECISION_REQUESTED_EVENTS). The INBOUND half did not,
and that is the half that closes the loop.

Application.php imported OCA\Decidesk\Event\DecisionConcludedEvent and registered
`::class`, which resolves at COMPILE TIME to a string nothing dispatches any
more, so the listener attached to a name that never fires. The listener then
tested `instanceof` against that same name, so even a correctly registered event
would have been rejected as somebody else's. Both fail in total silence: an
event with no listener and a listener on no event are indistinguishable from
here. The visible symptom is a contract approved in the decision app that never
leaves `In onderhandeling`, with nothing logged and nothing thrown.

Verified 2026-09-09 against decidiq development d72839c:
OCA\Decidiq\Event\DecisionConcludedEvent EXISTS and publishes the whole getter
surface this listener reads (getSourceApp, getSubjectId, getExternalReference,
getDecisionId, getStatus). The old spelling is kept regardless — an instance
pinned to a release from before that rename still dispatches it, and dropping it
re-breaks the integration in the other direction.

No new resolver class: this repo already had the pattern, one FQN-string
constant per event beside the service that owns the boundary, so the conclusion
event gets the sibling constant rather than a second way of doing the same
thing. `instanceof` against an absent class is simply false — it neither
autoloads nor errors — so naming both costs nothing on an instance running one.

SOURCE_APP stays `stackiq`: it is persisted on the decision app's rows and the
listener filters inbound events against it.

tests/analysis-stubs/decidiq-events.stub.php mirrors the real signatures so
phpstan and psalm see both spellings rather than proving the newer half dead.
Analysis-only, and deliberately out of the tests/Stubs glob for the reason its
decidesk sibling records.
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/stackiq @ 1e9baad

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
check-vue-demi
test-l10n
format
check-schema-l10n
check-l10n-js
composer ✅ 130/130
npm ✅ 704/704
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-09-09 11:21 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 989022e into development Sep 9, 2026
50 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/stale-fleet-namespace-lookups branch September 9, 2026 11:22
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