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] Add ability to override default api endpoint #23865

Closed
wants to merge 2 commits into from
Closed

[5.6] Add ability to override default api endpoint #23865

wants to merge 2 commits into from

Conversation

Miguel-Serejo
Copy link

@Miguel-Serejo Miguel-Serejo commented Apr 12, 2018

Fixes #23864 by allowing a user to set a "mail.endpoints.sparkpost" config key to choose between available endpoints found at https://developers.sparkpost.com/api/

example addition to config/mail.php:

/*
|--------------------------------------------------------------------------
| Mail Service Endpoints
|--------------------------------------------------------------------------
|
| When using an email service, you might need to override the default API
| endpoints. This allows you to configure different endpoints based on your
| environment.
|
*/
'endpoints' => [
  'sparkpost' =>  env('SPARKPOST_ENDPOINT', 'https://api.sparkpost.com/api/v1'),
],

I'm not sure how to test this, but I would argue that any test to this feature would just be a test of the config helper.

I'm also unsure if other mail drivers should get the same treatment as I don't use any of them. It's an easy enough change to add to all the drivers for this PR if that makes sense at all.

It's a fairly simple change, and would only be a BC break if someone was using "mail.endpoints.*" already in their configuration. At a guess I would say that's unlikely, but if an argument is made for it we could easily change the config key at this stage.

Fixes #23864 by allowing a user to set a "mail.endpoints.sparkpost" config key to choose between available endpoints found at https://developers.sparkpost.com/api/
@Miguel-Serejo Miguel-Serejo changed the title Add ability to override default api endpoint [5.6] Add ability to override default api endpoint Apr 12, 2018
@@ -54,7 +54,7 @@ public function send(Swift_Mime_SimpleMessage $message, &$failedRecipients = nul

$message->setBcc([]);

$response = $this->client->post('https://api.sparkpost.com/api/v1/transmissions', [
$response = $this->client->post(config('mail.endpoints.sparkpost', '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.

This should be services.sparkpost.endpoint.

Copy link
Member

Choose a reason for hiding this comment

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

You can't just call the foundation config function from the mail component. It doesn't exist.

Copy link
Member

Choose a reason for hiding this comment

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

What @GrahamCampbell means is: The helper config() is not available if you use the package illuminate/mail individually.

@@ -54,7 +54,7 @@ public function send(Swift_Mime_SimpleMessage $message, &$failedRecipients = nul

$message->setBcc([]);

$response = $this->client->post('https://api.sparkpost.com/api/v1/transmissions', [
$response = $this->client->post(config('mail.endpoints.sparkpost', 'https://api.sparkpost.com/api/v1/transmissions'), [
Copy link
Member

Choose a reason for hiding this comment

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

You can't just call the foundation config function from the mail component. It doesn't exist.

Previous approach assumed the existence of the `config()` helper.
This approach uses the the existing `options` configuration array in an attempt to maintain full BC while also making it clear that an endpoint can now be set.
@Miguel-Serejo
Copy link
Author

Miguel-Serejo commented Apr 13, 2018

Thank you for the feedback.

Changed the implementation to fetch the endpoint from the options array. This requires adding the options array with the endpoint element to the services.sparkpost configuration as such:

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

This is the minimal change I could find to avoid using the config helper. No changes to method signatures or constructor parameters. Could also be extended to other Transports if necessary.

One thing that I couldn't decide on was if setOptions should now also change the endpoint if it is present in the passed array or if explicit getter/setter should be declared. On one hand, I hesitate to change the public API mid-release, on the other hand, explicit setters would make it easier for devs to change only the endpoint. Could use some more opinions on this.

@taylorotwell
Copy link
Member

Your constructor doesn't make sense to me. What if endpoint isn't in the options?

@Miguel-Serejo Miguel-Serejo deleted the patch-1 branch April 17, 2018 12:42
@Miguel-Serejo
Copy link
Author

You're absolutely right. I'll give this another shot later today.

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.

6 participants