[mailgun-mailer] support EU-endpoint #31897
Conversation
6ad523c to
8e3ba0b
Compare
| return self::SMTP_DOMAIN_EU; | ||
| } | ||
|
|
||
| return self::SMTP_DOMAIN_US; |
There was a problem hiding this comment.
Should we make sure that the region is always either EU or US? US might be a reasonable default, but silently falling back to US in case we receive garbage does not feel right.
There was a problem hiding this comment.
just in case to preserve previous behaviour. Before changes it was used US
There was a problem hiding this comment.
Keeping US as default if no region is set is probably reasonable. But I would expect an error if I set the region to MiddleEarth, Asia, Europe or any string other than EU or US.
There was a problem hiding this comment.
is it enough to have just error as undefined index during access mapping const? or you're expecting like native exception?
There was a problem hiding this comment.
undefined index will only be a notice in production, and won't fail properly. So please add an exception (which also allows providing a better message in dev mode, so better DX).
There was a problem hiding this comment.
ok I hope RuntimeException will be fine for you
| * | ||
| * @experimental in 4.3 | ||
| */ | ||
| class Mailgun |
There was a problem hiding this comment.
Can we find a more expressive name? MailgunRegions maybe?
There was a problem hiding this comment.
me too confused with that naming
d5e48b6 to
27ccf5b
Compare
done |
27ccf5b to
2f93b85
Compare
[mailgun-mailer] support EU-endpoint. fix tests [mailgun-mailer] support EU-endpoint. fix tests without region [mailgun-mailer] support EU-endpoint. fix notices [mailgun-mailer] support EU-endpoint. improve test [mailgun-mailer] support EU-endpoint. Default region in transport declaration
9470747 to
359e214
Compare
src/Symfony/Component/Mailer/Bridge/Mailgun/MailgunRegionConfiguration.php
Outdated
Show resolved
Hide resolved
…tion and throw exception in constuctors
|
I would be glad if someone assist to fix deps builds in travis. I can't figure out how to fix it. |
| { | ||
| $this->key = $key; | ||
| $this->domain = $domain; | ||
| $this->endpoint = MailgunRegionConfiguration::resolveApiEndpoint($domain, $region); |
There was a problem hiding this comment.
I don't think the MailgunRegionConfiguration class is really needed.
You can, just change the ENDPOINT constant in each Mailgun Transports (or add it for the smtp transport) to add the region endpoint, something like : private const ENDPOINT = 'https://%region_endpoint%.mailgun.net/v3/%domain%/messages'
And store the region the same way as the domain, $this->regionEndpoint = 'us' === $region ? 'api' : 'api.eu';
Then (at line 51) replace the domain and the region endpoint at the same time
$endpoint = str_replace(['%domain%', '%region_endpoint%'], [urlencode($this->domain), $this->regionEndpoint], self::ENDPOINT);
What do you think ?
There was a problem hiding this comment.
that might be become tricky as soon as new region for example Asia might have host name https://mailgun.asia/
Me too don't like that helper class but we have to resolve endpoint in 3 different transports so sooner or later we have met case at which that will require to be extracted
There was a problem hiding this comment.
I think they would use the same pattern instead of using something different. But anyway, that's speculation at this point. I prefer #31998, which implements it the same as what we have in SesTransport, so it's more consistent across the board.
…transport default region
e6c83b8 to
565cfe4
Compare
| self::REGION_US => 'smtp.mailgun.org', | ||
| ]; | ||
|
|
||
| private const ENDPOINT_DOMAINS = [ |
There was a problem hiding this comment.
API_HOSTS for consistency with SMTP_HOSTS.
|
Closing in favor of #31998 |
Provide support for regions for mailer-railgun bridge