EU API and SMTP endpoints

[trys]

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]

@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.

[trys]

@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]

@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]

@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]

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]

@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]

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]

@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]

@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.

[trys]

@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]

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.