-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
[5.6] Allow user-defined Sparkpost endpoint #23910
Conversation
This allows the user to set an endpoint when using the Sparkpost transport.
@@ -54,7 +54,9 @@ public function send(Swift_Mime_SimpleMessage $message, &$failedRecipients = nul | |||
|
|||
$message->setBcc([]); | |||
|
|||
$response = $this->client->post('https://api.sparkpost.com/api/v1/transmissions', [ | |||
$endpoint = $this->getOptions()['endpoint'] ?? 'https://api.sparkpost.com/api/v1/transmissions'; |
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.
Don't you think it would be a breaking change if endpoint
is not set in service config? https://github.com/laravel/laravel/blob/master/config/services.php#L29
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.
"??" does an "isset" behind the scenes. If so, there should be no issue.
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 did forget about that detail on my first attempt at solving this, but this approach should prevent any errors from undefined configurations.
If someone were to backport this to an older version of laravel that doesn't require PHP 7, they'd need to manually check the options. Here's a little 3v4l snippet: https://3v4l.org/No8Mm
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 believe it should be set in service config just for the sake of making it obvious that it's customizable (which would mean moving the default endpoint from the transport class to the config array). It would save time for some developers.
Can you please add this below to the Laravel 5.6 documentation -
you can drop the Not sure if there is repo for the documentation page. |
@dmcbrn PR submitted. Example was changed to match the style of every other example for services. |
Fixes #23864.
This allows the user to set an endpoint when using the Sparkpost transport by setting the 'services.sparkpost.options.endpoint' configuration key.
Currently, the options array is empty by default, and not actually used in the class, but the TransportManager still passes it into the constructor.
To use this, a user must add the sparkpost.options.endpoint key in config/services.php, for example:
If the config option isn't set, or if it is null (default return from the
env()
helper), the default 'https://api.sparkpost.com/api/v1/transmissions' endpoint is used.This approach allows a user to modify the endpoint of an existing SparkPostTransport through
setOptions()
. It also allows a user to have more complex endpoint logic by simply extending the class and overridinggetOptions()
rather than having to overridesend()
or the constructor.