search

laravel-notification-channels / twilio

Twilio notifications channel for Laravel
https://laravel-notification-channels.com
227 stars 36 forks source link

Add 'twilio.to' config option allowing to set universal receiver #86

Closed klimov-paul closed 4 years ago

klimov-paul commented 4 years ago

Relates to #72 (fix partially).

Adds 'twilio.to' config option, allowing to set universal receiver phone number.

atymic commented 4 years ago

I think a log file would make more sense IMO.

klimov-paul commented 4 years ago

Wouldn't this be better handled by the end user in the routing?

It would require adjusting all models, which may represent receivers.

Such solition is consistent to Laravel mail one. See Mail & Local Development

atymic commented 4 years ago

It would require adjusting all models, which may represent receivers.

Such solition is consistent to Laravel mail one. See Mail & Local Development

Fair enough, but I think in this case a log is a much better idea - sending messages costs a fair bit.

klimov-paul commented 4 years ago

No one says there should not be a log driver. It is a matter for the another changeset.

For the manual QA it is sometims crucial to test actual SMS sending via live service, instead of logging. This changeset is needed for such case.

atymic commented 4 years ago

@klimov-paul

How would you feel merging these PRs (this one, plus the exception one) into a new branch for v3 (with a dedicated / documented config file)?

klimov-paul commented 4 years ago

This is your call. Although it might be better to re-implement feature from scratch in case new config approach to be implemented.

gregoriohc commented 4 years ago

Closing. This will be implemented in new v3 version as mentioned in PR #90