Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Notifier] Remove default transport property in Transports class#35834
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
d307e86 tob6a2d24Compare| { | ||
| if (null ===$transportName) { | ||
| thrownewLogicException('A Chat notification must have a transport defined.'); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't see a reason for this restriction. All the other channels don't have it either.
b6a2d24 toa5c207bCompareThis 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
jschaedl commentedMar 24, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hi@fabpot, what do you think about removing the default transport in the |
| foreach ($this->transportsas$transport) { | ||
| if ($transport->supports($message)) { | ||
| $transport->send($message); | ||
| ++$supportedTransportsCount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 to066990bCompare51e8352 to96218d3Comparejschaedl commentedJun 12, 2020
@fabpot friendly ping :-) I changed the code according to your comment. WDYT? |
96218d3 to5c167b0Comparefabpot commentedJun 12, 2020
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: