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

[5.6] Allow user-defined Sparkpost endpoint #23910

Merged
merged 1 commit into from
Apr 26, 2018
Merged

[5.6] Allow user-defined Sparkpost endpoint #23910

merged 1 commit into from
Apr 26, 2018

Conversation

Miguel-Serejo
Copy link

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:

'sparkpost' => [
        'secret' => env('SPARKPOST_SECRET'),
        'options' => [
                'endpoint' => env('SPARKPOST_ENDPOINT');
        ]
    ],

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 overriding getOptions() rather than having to override send() or the constructor.

This allows the user to set an endpoint when using the Sparkpost transport.
@taylorotwell taylorotwell merged commit 09aadce into laravel:5.6 Apr 26, 2018
@@ -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';
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Author

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

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.

@dmcbrn
Copy link

dmcbrn commented Jul 9, 2018

Can you please add this below to the Laravel 5.6 documentation - SparkPost Driver section:

'sparkpost' => [
    'secret' => env('SPARKPOST_SECRET'),
    'options' => [
        'endpoint' => env('SPARKPOST_ENDPOINT','https://api.eu.sparkpost.com/api/v1')
    ]
],

you can drop the .eu part for default value.

Not sure if there is repo for the documentation page.

@Miguel-Serejo
Copy link
Author

@dmcbrn PR submitted. Example was changed to match the style of every other example for services.

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.

None yet

7 participants