Skip to content

[Mailer] Change the DSN semantics #33424

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

Merged
merged 1 commit into from
Sep 2, 2019
Merged

[Mailer] Change the DSN semantics #33424

merged 1 commit into from
Sep 2, 2019

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented Sep 2, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR symfony/symfony-docs#12258

I'm not very satisfied with the current DSNs for the Mailer. First, the scheme/protocol should use the provider name, then, there is no way to change the host, which would be nice to debug more easily (using a requestb.in service for instance).

Before:

smtp://USERNAME:PASSWORD@mailgun
http://KEY:DOMAIN@mailgun

After:

mailgun+smtp://USERNAME:PASSWORD@default
mailgun+http://KEY:DOMAIN@default

# New
mailgun+http://KEY:DOMAIN@somewhere.com:99

SMTP DSNs did not change, but the special sendmail one did:

Before:

smtp://sendmail

After:

sendmail+smtp://default

And for the null transport:

Before:

smtp://null

After:

null://null

@stof
Copy link
Member

stof commented Sep 2, 2019

to make it easier to upgrade, should we deprecate the old syntax instead of dropping it directly (which would even transform them to supported smtp DSNs using an invalid host for some of them, instead of failing cleanly to warn people) ?

@fabpot fabpot force-pushed the mailer-dsns branch 2 times, most recently from f6c6f99 to 6e67fa9 Compare September 2, 2019 13:36
@fabpot
Copy link
Member Author

fabpot commented Sep 2, 2019

@stof The component is marked as experimental. Moving to the next syntax is easy enough IMHO.

fabpot added a commit that referenced this pull request Sep 2, 2019
This PR was merged into the 4.4 branch.

Discussion
----------

[Mailer] Change the DSN semantics

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | yes
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | symfony/symfony-docs#12258

I'm not very satisfied with the current DSNs for the Mailer. First, the scheme/protocol should use the provider name, then, there is no way to change the host, which would be nice to debug more easily (using a requestb.in service for instance).

Before:

```
smtp://USERNAME:PASSWORD@mailgun
http://KEY:DOMAIN@mailgun
```

After:

```
mailgun+smtp://USERNAME:PASSWORD@default
mailgun+http://KEY:DOMAIN@default

# New
mailgun+http://KEY:DOMAIN@somewhere.com:99
```

SMTP DSNs did not change, but the special sendmail one did:

Before:

```
smtp://sendmail
```

After:

```
sendmail+smtp://default
```

And for the `null` transport:

Before:

```
smtp://null
```

After:

```
null://null
```

Commits
-------

469c989 [Mailer] Change the DSN semantics
@fabpot fabpot merged commit 469c989 into symfony:4.4 Sep 2, 2019
@stof
Copy link
Member

stof commented Sep 2, 2019

My concern is that some of the existing DSNs will change meaning without warning the user while still being accepted (all the smtp one) and so they might now know that they need to migrate.
I know that an experimental component allows us to break BC. But in the past (messenger), we already took care of the upgrade path of experimental components to avoid making it too disruptive.

@fabpot
Copy link
Member Author

fabpot commented Sep 2, 2019

We did that for Messenger as we kept the "experimental" status for more than one version.

@fabpot fabpot deleted the mailer-dsns branch September 6, 2019 10:41
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Sep 16, 2019
This PR was squashed before being merged into the master branch (closes #12258).

Discussion
----------

Move to the new DSN format for Mailer

Documents symfony/symfony#33424

Commits
-------

c9febee Move to the new DSN format for Mailer
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Oct 16, 2019
…uz, tucksaun)

This PR was merged into the 4.4 branch.

Discussion
----------

[Mailer] Move to the new DSN format

Documents symfony/symfony#33424

Cherry-picking #12258 that should have target 4.4 and also add a missing DSN update.

Commits
-------

a2c607b [Mailer] Move remaining DSN to new format
ca56502 Minor tweaks
be392bc Move to the new DSN format for Mailer
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
This was referenced Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants