Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:7.4fromKiloSierraCharlie:mailtrap-sandbox7.4
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.

QA
Branch?7.4
Bug fix?no
New feature?yes
Deprecations?no
Issuesn/a
LicenseMIT

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

Updates with appropriate tests and changelog.

@KiloSierraCharlie
Copy link
ContributorAuthor

Sorry about that@Chris53897. Mistake resolved with commit.

@kbond
Copy link
Member

Thanks for working on this@KiloSierraCharlie. There's still some logic needed to change the endpoint host if aninboxId is provided?

KiloSierraCharlie added a commit to KiloSierraCharlie/symfony that referenced this pull requestAug 5, 2025
Suggested changes made as part of PRsymfony#61315.
@KiloSierraCharlie
Copy link
ContributorAuthor

Apoligies@kbond &@nicolas-grekas - new commite767fd6 with all changes submitted to branch.

Copy link
Member

@kbondkbond 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 SMTPMAILER_DSN=mailtrap+sandbox+smtp://PASSWORD@default# SANDBOX APIMAILER_DSN=mailtrap+sandbox+api://INBOX_ID:TOKEN@default

@KiloSierraCharlie
Copy link
ContributorAuthor

@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 requestAug 5, 2025
Add documentation relating to the Mailtrap sandbox implementation in 7.4 (symfony/symfony#61315).
@KiloSierraCharlie
Copy link
ContributorAuthor

I'll get there eventually 😢

KiloSierraCharlie added a commit to KiloSierraCharlie/symfony that referenced this pull requestAug 6, 2025
Suggested changes made as part of PRsymfony#61315.
@KiloSierraCharlie
Copy link
ContributorAuthor

Accidentally merge-commit'd instead of rebasing hence the force-push...

KiloSierraCharlie added a commit to KiloSierraCharlie/symfony that referenced this pull requestAug 17, 2025
Suggested changes made as part of PRsymfony#61315.
KiloSierraCharlie added a commit to KiloSierraCharlie/symfony that referenced this pull requestAug 18, 2025
Suggested changes made as part of PRsymfony#61315.
Copy link
Member

@fabpotfabpot 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 commentedAug 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
ContributorAuthor

KiloSierraCharlie commentedAug 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, howeverit was suggested to implement a separate DSN to ensure that prod/sandbox enviornments are clearer to identify for users.

ro0NL reacted with thumbs up emoji

@fabpot
Copy link
Member

Thank you@KiloSierraCharlie.

@fabpotfabpot merged commit7ed3a4b intosymfony:7.4Aug 19, 2025
5 of 12 checks passed
@KiloSierraCharlieKiloSierraCharlie deleted the mailtrap-sandbox7.4 branchAugust 19, 2025 18:27
fabpot added a commit that referenced this pull requestAug 22, 2025
… 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       | MITI 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 requestSep 1, 2025
…SierraCharlie)This PR was squashed before being merged into the 7.4 branch.Discussion----------[Mailer] Document Mailtrap's sandbox compatibilityDocumentation forsymfony/symfony#61315 - sandbox compatibility for Mailtrap bridge.Commits-------8b5ef68 [Mailer] Document Mailtrap's sandbox compatibility
This was referencedOct 27, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@fabpotfabpotfabpot approved these changes

@kbondkbondAwaiting requested review from kbond

+1 more reviewer

@Chris53897Chris53897Chris53897 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

7.4

Development

Successfully merging this pull request may close these issues.

7 participants

@KiloSierraCharlie@kbond@ro0NL@fabpot@nicolas-grekas@Chris53897@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp