Harden the notification dispatcher and the channel gate - #2001
Merged
markus-moser merged 3 commits intoAug 18, 2026
Merged
Conversation
The gate removed the channel's service definition when no registered type allows external delivery. Aliases and references are not rewritten by removeDefinition(), so the first bundle to alias ChannelInterface to its own channel — exactly what the extending doc shows — got a ServiceNotFoundException at compile time as soon as the gate closed. Untagging achieves the same thing without touching the graph: the registry collects by tag, and an untagged private service nothing references is dropped by Symfony's own unused- definition pass, so no dead mailer is instantiated either way. The new test aliases the interface to a gated channel and compiles the container; it fails with removeDefinition() and passes with clearTag(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ons testable Two problems with one cause: NotificationDispatcher::write() called Notification::save(), which goes through a Dao, so the dispatcher could not be unit tested at all — and the write was the only step in the fan-out that was not isolated. The untestability was not theoretical. testBrokenChannelDoesNotPreventOtherChannelsFrom- Delivering never called dispatch(); it built a TestChannel with throwOnSend and then asserted on ChannelRegistry. TestChannel::$sent was written by the fixture and asserted nowhere in the suite. The resilience guarantee ChannelInterface::send() documents in capitals was unverified, as were the permission skip, the unsubscribed skip and the unknown-recipient skip. Extract NotificationWriterInterface so the dispatcher holds only routing decisions, then wrap the per-recipient body so a failed write is logged and the fan-out continues. Previously a failure part-way through delivered to the recipients before it, silently skipped everyone after it, and surfaced as an exception the producer could do nothing with — while deliver() immediately below already logged and continued. The interface promised the latter behaviour. Eight dispatcher tests now call dispatch() and assert on what the writer and the channels actually received. Both fixes are mutation-checked: removing the guard fails the fan-out test, and the pre-existing behaviour fails the isolation test. The EffectiveSubscription and DispatchableNotification cases move to their own files, so NotificationDispatcherTest is about the dispatcher. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
markus-moser
force-pushed
the
feat/notification-dispatch-hardening
branch
from
August 18, 2026 09:39
f24d490 to
22e5171
Compare
|
markus-moser
marked this pull request as ready for review
August 18, 2026 09:46
markus-moser
merged commit Aug 18, 2026
71f3e57
into
feat/notification-subscriptions
13 of 14 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #1959 — base is
feat/notification-subscriptions, so this diff is only the hardening. Retarget to2026.xif #1959 merges first.Addresses the three findings I would have blocked #1959 on.
1. The channel gate broke the extension path it documents
NotificationDispatchPassremoved a channel's service definition when no registered type allows external delivery.removeDefinition()does not rewrite aliases or references, so the first bundle to aliasChannelInterfaceto its own channel — exactly what14_Extending_Notifications.mdshows — got aServiceNotFoundExceptionat compile time the moment the gate closed. Core was safe only becauseEmailChannelhas no alias.Clearing the tag achieves the same gate without touching the graph: the registry collects by tag, and an untagged private service nothing references is dropped by Symfony's own unused-definition pass, so no dead mailer is instantiated either way.
2. The dispatcher had no tests, and one test claimed otherwise
testBrokenChannelDoesNotPreventOtherChannelsFromDeliveringnever calleddispatch()— it built aTestChannelwiththrowOnSend: true, then asserted onChannelRegistry.TestChannel::$sentwas written by the fixture and asserted nowhere in the suite. So the guaranteeChannelInterface::send()documents in capitals was unverified, along with the permission skip, the unsubscribed skip and the unknown-recipient skip — on a class with no in-repo caller, which other bundles are meant to build against.The blocker was
write()callingNotification::save(), which goes through a Dao. Doubling that is worse than useless — Dao-proxied calls silently no-op on a mock, so tests pass while asserting nothing. ExtractingNotificationWriterInterfaceleaves the dispatcher holding only routing decisions.Eight tests now call
dispatch()and assert on what the writer and the channels actually received.3. One bad recipient aborted the whole fan-out
write()threwDatabaseExceptionout of the per-recipient loop, so a failure part-way through delivered to the recipients before it, silently skipped everyone after it, and surfaced as an exception the producer could do nothing with — whiledeliver()immediately below already logged and continued.NotificationDispatcherInterfacepromised the latter. The loop body is now wrapped; an unregistered type id still raises, since that is a wiring mistake caught before any recipient is processed.Verification
removeDefinition()fails the container-compiles test, and removing the per-recipient guard fails the fan-out test. Neither test is vacuous.Not included
The compiler pass still scans every container definition rather than using
findTaggedServiceIds()— a performance and robustness outlier, not a correctness bug, and a larger change than these three.🤖 Generated with Claude Code