[Notifier] Remove default transport property in Transports class #35834
Conversation
d307e86 to
b6a2d24
Compare
| @@ -26,10 +25,6 @@ class ChatChannel extends AbstractChannel | |||
| { | |||
| public function notify(Notification $notification, Recipient $recipient, string $transportName = null): void | |||
| { | |||
| if (null === $transportName) { | |||
| throw new LogicException('A Chat notification must have a transport defined.'); | |||
| } | |||
There was a problem hiding this comment.
I don't see a reason for this restriction. All the other channels don't have it either.
b6a2d24 to
a5c207b
Compare
This PR was merged into the 5.0 branch. Discussion ---------- [Notifier] Add unit tests | Q | A | ------------- | --- | Branch? | 5.0 | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | - <!-- prefix each issue number with "Fix #", if any --> | License | MIT | Doc PR | - <!-- required for new features --> - [x] `AbstractChannel` - [x] `ChannelPolicy` - [x] `RecipientTest` (done in PR #35773) - [x] `EmailRecipientTest` (done in PR #35773) - [x] `SmsRecipientTest` (done in PR #35773) - [x] `Transports` (see PR #35834) Commits ------- 022c170 [Notifier] Add tests for AbstractChannel and ChannelPolicy
|
Hi @fabpot, what do you think about removing the default transport in the |
| foreach ($this->transports as $transport) { | ||
| if ($transport->supports($message)) { | ||
| $transport->send($message); | ||
| ++$supportedTransportsCount; |
There was a problem hiding this comment.
Instead of sending it through all available transports, I would only send it to the first one. That would be more consistent with my idea of a default transport and probably what I would expect. I would probably not expect to receive 2 SMSes for instance. When you want to dispatch to more than one transport, you need to be explicit.
There was a problem hiding this comment.
Hmm, ok. If I only define a channel name in the channel policy configuration, I actually expected the notification to be broadcasted to all supported transports defined in this channel. For me personally I wouldn't need a default transport. But I also see that this broadcasting approach might be confusing in some situations and a default channel is a good idea. Will change this.
a5c207b to
066990b
Compare
51e8352 to
96218d3
Compare
|
@fabpot friendly ping :-) I changed the code according to your comment. WDYT? |
96218d3 to
5c167b0
Compare
|
Thank you @jschaedl. |
At the moment the
Transportsclass uses the first element of the injected transports array as the default transport: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Notifier/Transport/Transports.php#L35If you try to send a message that doesn't define a transport (
!$message->getTransport()) the default transport is used. I see two main drawbacks with this solution that I try to fix with this PR:There is no check if the given message is supported by the default transport. What means that the transport is going to fail with an Exception, if it's not supporting the given message. E.g. the
SlackTransportonly supportsChatMessages with nullable options or options from typeSlackOptions. So as a default transport theSlackTransportcan't handle all types ofChatMessages.Why should we only send the message using the default transport if there are more possible transports which are probably supported?
I did the following to fix the mentioned drawbacks: