Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

klimov-paul
Copy link

Relates to #72 (fix partially).

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

Copy link
Member

@atymic atymic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I.e. in your model:

// user model 

public function routeNotificationsForTwilio() {
	if (app()->environment('local')) {
		return config('twilio.default_to', $this->phone)
	}

	return $this->phone;
}

@atymic
Copy link
Member

atymic commented Mar 10, 2020

I think a log file would make more sense IMO.

@klimov-paul
Copy link
Author

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
Copy link
Member

atymic commented Mar 11, 2020

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
Copy link
Author

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
Copy link
Member

atymic commented Mar 11, 2020

@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
Copy link
Author

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

@gregoriohc gregoriohc mentioned this pull request Apr 8, 2020
5 tasks
@gregoriohc
Copy link
Member

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

@gregoriohc gregoriohc closed this Apr 8, 2020
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.

3 participants