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

EU API and SMTP endpoints #141

Merged
merged 4 commits into from Apr 25, 2018

Conversation

@trys
Copy link
Contributor

commented Apr 17, 2018

After encountering an error when sending test emails, I contacted SparkPost support who informed me that there were different API & SMTP endpoints for SparkPost EU.

I've added a new API location field to the admin area (defaulting to Worldwide) and set up a new filter on the two endpoints.

One thing to note, and I'm not sure if this is a SparkPost internal thing or something else that needs to be worked on, SMTP emails throw the following error on PHP 7.1.

Warning: stream_socket_enable_crypto(): Peer certificate CN=`*.sparkpostmail.com' did not match expected CN=`smtp.eu.sparkpostmail.com' in /wp-includes/class-smtp.php on line 368

The support team advised that the SMTP host should be: smtp.eu.sparkpostmail.com, so I'm not certain as to why this error is occurring. If it's an internal error, I can always update the PR to drop SMTP support in EU locations.

@rajumsys

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2018

@trys Thanks for the PR. We'll review it soon.

And for the certificate issue, we've escalated this to appropriate team and they're looking at it now.

@@ -262,6 +262,7 @@ public function admin_page_init()
add_settings_field('sending_method', 'Method*', array($this, 'render_sending_method_field'), 'sp-options-basic', 'general');
add_settings_field('password', 'API Key*', array($this, 'render_password_field'), 'sp-options-basic', 'general');
add_settings_field('template', 'Template', array($this, 'render_template_field'), 'sp-options-basic', 'general');
add_settings_field('location', 'API Location', array($this, 'render_location_field'), 'sp-options-basic', 'general' );

This comment has been minimized.

Copy link
@rajumsys

rajumsys Apr 19, 2018

Contributor

Please move it before templates.

@trys

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2018

@rajumsys I've just heard back from your support team who have confirmed the certificate error has been resolved on your side. I've just re-tested the plugin changes and can confirm the error has gone.

@rajumsys

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2018

@trys
Your PR looks good and works as expected. Just thinking about a more definitive way of replacing hostname than search/replace. Let me know if you've any thought. But I guess it's fine anyway.

Also, can you add some test coverage for this? Seems the coverage is reduced a bit. We're trying to enhance it.

@trys

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2018

@rajumsys Agreed, search replace isn't great. Do SparkPost have any other regions/plans to expand? It would be worth writing something reasonably future proof. An array of endpoints would probably make the most sense at the point. I'll jump on that later this evening, along with adding some tests.

@rajumsys

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2018

SparkPost will keep expanding, I believe. But it's tough to predict what hostnames and/or regions it'll be (and even whether they will have different hostname). So, IMO, we can't make it future proof but can make it easily extensible.

Like, we can keep an nested assoc array of location and hostname as you said. From the dropdown, we can store the location in db and select the endpoints based on that.

{
  'us': {
     'api': 'api.sparkpost.com',
     'smtp': 'smtp.sparkpost.com'
  }, 
  'eu': { 
     'api': 'api.eu.sparkpost.com',
     'smtp': 'smtp.eu.sparkpost.com
  }
}

that's an idea but may not be exactly same. we want to always fallback to us/international (as plugin users who are upgrading from previous version won't have that value).

also please keep apply_filter support as you're doing it now in case we needed to change (that's also useful for enterprise customers as their hostnames may be different).

@trys

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2018

@rajumsys I've refactored the hostnames and added tests for the two methods added - but test coverage is showing as going down still. How shall I proceed?

@rajumsys

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2018

That's awesome. thanks for addressing this. I think we're almost there. I've some minor simplification ideas.

I can't think of an use case for sp_location filter. so, may be we can get rid of that and do something like this

mailer.http.class.php

//$location = apply_filters('sp_location', 'us'); //remove this line 
$this->endpoint = SparkPost::get_hostname('api') . '/api/v1/transmissions';

mailer.smtp.class.php

// $location = apply_filters('sp_location', 'us'); //remove this line
$host = SparkPost::get_hostname('smtp');

then, in sparkpost.class.php

 public function hostname_location()
    {
       $default_location = 'us';
        return !empty($this->settings['location']) ? esc_attr($this->settings['location']) : $default_location;
    }
    static function get_hostname($type = 'api')
    {
        $location = this.hostname_location();
        return apply_filters('sp_hostname', self::$hostnames[$location][$type], $location);
    }

benefits, IMO

  • Only one filters to register. filter callback will receive current location and endpoint and can return their own. currently it can be done with one filter too but then the location filter becomes redundant and may be confusing.
  • all the mailers & template classes won't need to pass the location as sparkpost class/instance already knows about it

let me know what you think.

also if you can't fix that fraction of test coverage, leave it for now. thanks a lot

@trys

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2018

@rajumsys I've made get_hostname public instead of static, then moved the location code inside given it's only required there. Then, instead of calling it with SparkPost::get_hostname, I'm using apply_filters to get $this injected in. Hopefully that's all cool, the mailer and template classes don't have the SparkPost instance readily available so this seemed like the neatest way to jump into that class.

@rajumsys

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

@trys that's all fine. however, now not sure how other people (users of the plugin) can't override the endpoints! i added the following line in my previous comment for that

        return apply_filters('sp_hostname', self::$hostnames[$location][$type], $location);

anyway, that can be another improvement if needed; which is fine. I'm going to merge your PR. Thank you so much.

@rajumsys rajumsys merged commit 466df1f into SparkPost:master Apr 25, 2018
1 of 2 checks passed
1 of 2 checks passed
coverage/coveralls Coverage decreased (-0.3%) to 39.823%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@trys

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2018

@rajumsys I doubt calling apply_filters('sp_hostname') within a hook that's been called from sp_hostname will ever run - at worse, could it become recursive?

Other users can edit the hostname by adding their own filter like: add_filter('sp_hostname', 'other_function', 20); which'll run after our filter. The benefit of running the original method through the filter is that it can be removed/edited/added onto by other users, without adding another filter within the method.

@rajumsys

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2018

sorry, i see the confusion. i put the hook's name as example only. but we're good for now. earlier we had many hooks. it's ok if we've no or less hook as we can add later as improvement. having something and removing later will mean breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.