Skip to content

[Mailer] Add compatibility for Mailtrap's sandbox #61315

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 19, 2025
Merged

[Mailer] Add compatibility for Mailtrap's sandbox #61315

merged 1 commit into from
Aug 19, 2025

Conversation

KiloSierraCharlie
Copy link
Contributor

Mailtrap's sandbox requires the Inbox ID to be passed as part of the URI, which has previously been neglected.

Q A
Branch? 7.4
Bug fix? no
New feature? yes
Deprecations? no
Issues n/a
License MIT

Previously #61305 but I messed up when re-basing to 7.4.

Updates with appropriate tests and changelog.
@carsonbot carsonbot added this to the 7.4 milestone Aug 3, 2025
Chris53897
Chris53897 reviewed Aug 4, 2025
View reviewed changes
@KiloSierraCharlie
Copy link
Contributor Author

Sorry about that @Chris53897. Mistake resolved with commit.
nicolas-grekas
nicolas-grekas reviewed Aug 5, 2025
View reviewed changes
@kbond
Copy link
Member

kbond commented Aug 5, 2025

Thanks for working on this @KiloSierraCharlie. There's still some logic needed to change the endpoint host if an inboxId is provided?
KiloSierraCharlie added a commit to KiloSierraCharlie/symfony that referenced this pull request Aug 5, 2025
@KiloSierraCharlie
[Mailer] Mailtrap sandbox changes
e767fd6 
Suggested changes made as part of PR symfony#61315.
@KiloSierraCharlie
Copy link
Contributor Author

Apoligies @kbond & @nicolas-grekas - new commit e767fd6 with all changes submitted to branch.
kbond
kbond requested changes Aug 5, 2025
View reviewed changes
Copy link
Member

@kbond kbond left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I wonder: it isn't super clear that when you have an inboxId, the sandbox is used. Could it lead to some confusion as to why the live isn't being used.

Would it make more sense to have separate DSN schemes for the sandbox?

# SANDBOX SMTP MAILER_DSN=mailtrap+sandbox+smtp://PASSWORD@default # SANDBOX API MAILER_DSN=mailtrap+sandbox+api://INBOX_ID:TOKEN@default
@KiloSierraCharlie
Copy link
Contributor Author

@kbond Thanks for the suggestion, I think you're right - adding a seperate DSN makes it clearer as to which Mailtrap environment is being used. I've created a new transport which extends the original.

Unit tests for this new DSN added also.
KiloSierraCharlie added a commit to KiloSierraCharlie/symfony-docs that referenced this pull request Aug 5, 2025
@KiloSierraCharlie
[Mailer] Mailtrap sandbox documentation
cface15 
Add documentation relating to the Mailtrap sandbox implementation in 7.4 (symfony/symfony#61315).
@KiloSierraCharlie KiloSierraCharlie mentioned this pull request Aug 5, 2025
Merged
@KiloSierraCharlie KiloSierraCharlie requested a review from kbond August 5, 2025 19:23
@KiloSierraCharlie
Copy link
Contributor Author

I'll get there eventually 😢
KiloSierraCharlie added a commit to KiloSierraCharlie/symfony that referenced this pull request Aug 6, 2025
@KiloSierraCharlie
[Mailer] Mailtrap sandbox changes
c9c56ff 
Suggested changes made as part of PR symfony#61315.
@KiloSierraCharlie
Copy link
Contributor Author

Accidentally merge-commit'd instead of rebasing hence the force-push...
fabpot
fabpot requested changes Aug 17, 2025
View reviewed changes
KiloSierraCharlie added a commit to KiloSierraCharlie/symfony that referenced this pull request Aug 17, 2025
@KiloSierraCharlie
[Mailer] Mailtrap sandbox changes
816ad7a 
Suggested changes made as part of PR symfony#61315.
fabpot
fabpot requested changes Aug 17, 2025
View reviewed changes
KiloSierraCharlie added a commit to KiloSierraCharlie/symfony that referenced this pull request Aug 18, 2025
@KiloSierraCharlie
[Mailer] Mailtrap sandbox changes
abe4cd2 
Suggested changes made as part of PR symfony#61315.
fabpot
fabpot requested changes Aug 18, 2025
View reviewed changes
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

LGTM. Let's make sure we make the inboxId required.
@ro0NL
Copy link
Contributor

ro0NL commented Aug 18, 2025 •
edited
Loading

why not a single DSN scheme + optional DSN inboxId parameter? https://github.com/railsware/mailtrap-php/tree/main/src/Bridge/Symfony#sandbox
@KiloSierraCharlie
Copy link
Contributor Author

KiloSierraCharlie commented Aug 18, 2025 •
edited
Loading

@ro0NL

why not a single DSN scheme + optional DSN inboxId parameter? https://github.com/railsware/mailtrap-php/tree/main/src/Bridge/Symfony#sandbox

The initial PR was with a single DSN and optional inboxId param, however it was suggested to implement a separate DSN to ensure that prod/sandbox enviornments are clearer to identify for users.
@fabpot fabpot force-pushed the mailtrap-sandbox7.4 branch from 5895393 to 3256633 Compare August 19, 2025 06:17
@fabpot
Copy link
Member

fabpot commented Aug 19, 2025

Thank you @KiloSierraCharlie.
@fabpot fabpot merged commit 7ed3a4b into symfony:7.4 Aug 19, 2025
5 of 12 checks passed
@KiloSierraCharlie KiloSierraCharlie deleted the mailtrap-sandbox7.4 branch August 19, 2025 18:27
@xabbuh xabbuh mentioned this pull request Aug 22, 2025
Merged
fabpot added a commit that referenced this pull request Aug 22, 2025
@fabpot
minor #61499 [Mailer] use the same transport for both Mailtrap's live…
9556568 
… and sandbox API (xabbuh) This PR was merged into the 7.4 branch. Discussion ---------- [Mailer] use the same transport for both Mailtrap's live and sandbox API | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | | License | MIT I agree with #61315 (review) on having distinct DSN schemes for both the live and the sandbox API, but I'd prefer not to increase the maintenance costs for us by using distinct transport classes. Commits ------- 5f577fb use the same transport for both Mailtrap's live and sandbox API
xabbuh added a commit to symfony/symfony-docs that referenced this pull request Sep 1, 2025
@xabbuh
minor #21270 [Mailer] Document Mailtrap's sandbox compatibility (Kilo…
e4f629c 
…SierraCharlie) This PR was squashed before being merged into the 7.4 branch. Discussion ---------- [Mailer] Document Mailtrap's sandbox compatibility Documentation for symfony/symfony#61315 - sandbox compatibility for Mailtrap bridge. Commits ------- 8b5ef68 [Mailer] Document Mailtrap's sandbox compatibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature Mailer Status: Reviewed

7 participants

@KiloSierraCharlie @kbond @ro0NL @fabpot @nicolas-grekas @Chris53897 @carsonbot