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

[9.x] Allow overriding transport type on Mailgun transporter #41309

Merged

Conversation

jnoordsij
Copy link
Contributor

In #41255 the Mailgun transport type was changed from api to https. This breaks my application, as I add some custom headers which were working fine with the API transporter, but are ignored on the messages.mime endpoint. This MR allows overriding the transport type you want to use for Mailgun, while leaving the default option unchanged.

If wanted, I can also provide a PR to add this config key to the laravel/laravel repo.

@jnoordsij jnoordsij force-pushed the mailgun-customisable-transport-type branch from a498765 to bad9722 Compare March 3, 2022 10:12
@driesvints
Copy link
Member

Thanks @jnoordsij. I still feel this change was warranted as we used the /message.mime endpoint in Laravel v8.

Just to be clear: you weren't using these custom headers in Laravel 8, correct?

src/Illuminate/Mail/MailManager.php Outdated Show resolved Hide resolved
@driesvints
Copy link
Member

driesvints commented Mar 3, 2022

A PR to laravel/laravel could be useful as well.

'mailgun' => [
    'transport' => 'mailgun',
    'scheme' => 'https',
],

No env is needed as this should be a hardcoded value that's the same across environments.

@jnoordsij
Copy link
Contributor Author

Thanks @jnoordsij. I still feel this change was warranted as we used the /message.mime endpoint in Laravel v8.

Yeah, the change itself seems logical, its just even more flexible this way I feel.

Just to be clear: you weren't using these custom headers in Laravel 8, correct?

I was using them in Laravel 8, but had to rewrite the implementation in Laravel 9. I used to extend the Illuminate\Mail\Transport\MailgunTransport to alter the HTTP payload, but this transport was removed. As this was removed in Laravel 9, I had to rewrite things once already. Adding text headers for the testmode in Laravel 9 worked fine with the API endpoint, but I can't seem to get it working again with the messages.mime endpoint.

For reference, I tried to obtain the request body before sending it in both my Laravel 8 and Laravel 9 apps, and they seem to be quite different:

Laravel 8

--553506d1f480caca62c55a8678ed647e36bcd10a\r\n
Content-Disposition: form-data; name="to"\r\n
Content-Length: 20\r\n
\r\n
[email protected]\r\n
--553506d1f480caca62c55a8678ed647e36bcd10a\r\n
Content-Disposition: form-data; name="message"; filename="message.mime"\r\n
Content-Length: 324\r\n
\r\n
Message-ID: <[email protected]>\r\n
Date: Thu, 03 Mar 2022 10:41:04 +0100\r\n
Subject: Test message\r\n
From: Example <[email protected]>\r\n
To: [email protected]\r\n
MIME-Version: 1.0\r\n
Content-Type: text/html; charset=utf-8\r\n
Content-Transfer-Encoding: quoted-printable\r\n
\r\n
<p>Hello world!</p>\r\n
--553506d1f480caca62c55a8678ed647e36bcd10a\r\n
Content-Disposition: form-data; name="o:testmode"\r\n
Content-Length: 3\r\n
\r\n
yes\r\n
--553506d1f480caca62c55a8678ed647e36bcd10a\r\n
Content-Disposition: form-data; name="o:tag"\r\n
Content-Length: 26\r\n
\r\n
App Name (local)\r\n
--553506d1f480caca62c55a8678ed647e36bcd10a--\r\n
"""

Laravel 9

'body' => 'From: Example <[email protected]>
To: [email protected]
Subject: Test message
o:testmode: yes
X-Mailgun-Tag: App Name (local)
Message-ID: <[email protected]>
MIME-Version: 1.0
Date: Thu, 03 Mar 2022 10:57:09 +0100
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: quoted-printable

<p>Hello world!</p>'

@driesvints
Copy link
Member

@jnoordsij all of that is being handled by Symfony Mailer so there's nothing we can really do there.

@jnoordsij
Copy link
Contributor Author

@jnoordsij all of that is being handled by Symfony Mailer so there's nothing we can really do there.

Yeah I figured, hence having the option to just stick with the api transporter for myself seems to be the best solution for now.

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.

3 participants