8000 Replace symfony/swiftmailer-bundle with symfony/mailer by fritzmg · Pull Request #1829 · contao/contao · GitHub
[go: up one dir, main page]

Skip to content

Replace symfony/swiftmailer-bundle with symfony/mailer #1829

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 20 commits into from
Jul 1, 2020

Conversation

fritzmg
Copy link
Contributor
@fritzmg fritzmg commented Jun 14, 2020

This PR replaces the usage of symfony/swiftmailer-bundle with symfony/mailer. It serves as a basis for a follow-up PR for the available mailer selection based on the Symfony Mailer Component (which would replace the existing draft #1469 based on the Swiftmailer Bundle).

The switch is pretty straight forward actually. However there are some things to consider which I will comment on in the files view.

@fritzmg fritzmg added the feature label Jun 14, 2020 8000
@fritzmg fritzmg requested a review from a team June 14, 2020 23:57
@fritzmg fritzmg self-assigned this Jun 14, 2020
Copy link
Member
@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Looks great for a start!

@m-vo
Copy link
Member
m-vo commented Jun 15, 2020

Nice!

Imo MAILER_DSN and MAILER_URL aren't compatible formats. Would this force everyone to change their config?

@fritzmg
Copy link
Contributor Author
fritzmg commented Jun 15, 2020

Regarding MAILER_DSN and MAILER_URL - what if we:

  1. Use MAILER_DSN instead by default.
  2. Check if there is a MAILER_URL env and assume it is a Swiftmailer DSN.
  3. Parse the MAILER_URL DSN and rewrite it to a Symfony Mailer DSN and save it to MAILER_DSN.

@fritzmg fritzmg marked this pull request as draft June 15, 2020 18:09
@fritzmg fritzmg marked this pull request as ready for review June 15, 2020 21:15
@fritzmg
Copy link
Contributor Author
fritzmg commented Jun 15, 2020

I have added the fallback for MAILER_URL, using a similar, but reduced implementation of SwiftmailerTransportFactory::resolveOptions.

Note: the default SMTP mailer of the Symfony Mailer Component does not allow as many options as Swiftmailer did. The following options in a DSN will be ignored:

  • timeout
  • source_ip
  • local_domain
  • auth_mode
  • command

I deprecated \Contao\Email too - but this causes the tests to fail now.

@leofeyer
Copy link
Member

I deprecated \Contao\Email too - but this causes the tests to fail now.

Please revert it. The Contao 3 framework is already deprecated, therefore we do not need to deprecate every class separately.

@fritzmg
Copy link
Contributor Author
fritzmg commented Jun 16, 2020

Please revert it. The Contao 3 framework is already deprecated, therefore we do not need to deprecate every class separately.

Done.

One problem I now noticed is, that symfony/mailer currently does not allow to set the local domain for the SMTP HELO command (see symfony/symfony#37300). This is something that can be configured with Swiftmailer and people might rely on this. We have also used this feature (albeit temporarily) in order to be able to send E-Mails with arbitrary from address domains via the Google G Suite SMTP Relay.

I can make a PR for symfony/mailer, so that it can be set via the DSN again - but that will only make it into symfony/mailer 5.2.0, presumably.

@fritzmg fritzmg force-pushed the feature/symfony-mailer branch from a43f722 to c905ddc Compare June 16, 2020 15:12
@fritzmg fritzmg requested a review from Toflar June 17, 2020 13:18
@fritzmg fritzmg requested a review from leofeyer June 17, 2020 13:18
Toflar
Toflar previously approved these changes Jun 17, 2020
Copy link
Member
@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Looks good to me, excellent work!

@fritzmg
Copy link
Contributor Author
fritzmg commented Jun 24, 2020

In #1830 we briefly discussed, whether Contao should use its own mailer, rather than using the one from the framework bundle. This would of course have to implemented in this PR first. However, we decided against it, as I think it would still make more sense to use the existing mailer service from the framework bundle. Otherwise Contao would have to implement a lot of the mailer component on its own, which I think defeats the purpose.

@leofeyer
Copy link
Member

I think so, too. Using our own mailer instead of the default one is not a good idea IMHO.

@fritzmg
Copy link
Contributor Author
8000 fritzmg commented Jun 26, 2020

Note to self: check whether sendmail parameters can be set via Symfony Mailer too.

Ok wir haben die Lösung gefunden.

Swiftmailer nutzt in den default parameters das flag "-bs" was für Stand-alone SMTP server mode steht. https://github.com/swiftmailer/swiftmailer/blob/master/lib/classes/Swift/Transport/SendmailTransport.php

Wir haben das Flag in der config.yml auf -t gesetzt

swiftmailer:
    default_mailer: default
    mailers:
        default:
            url: '%env(MAILER_URL)%'
            transport: sendmail
            command: '/usr/sbin/sendmail -t'

Da nur -bs oder -t zulässig sind. Jetzt funktioniert das zumindest auch bei unserem mittwald server per sendmail

https://contao.slack.com/archives/CK4J0KNDB/p1593163889306900?thread_ts=1593073731.285600&cid=CK4J0KNDB

@fritzmg
Copy link
Contributor Author
fritzmg commented Jun 26, 2020

I created a PR for the missing options: symfony/symfony#37432

composer.json Outdated
@@ -99,14 +99,14 @@
"symfony/http-foundation": "4.4.*",
"symfony/http-kernel": "4.4.*",
"symfony/lock": "4.4.*",
"symfony/mailer": "^4.4 || 5.0.* || 5.1.*",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
6D40
"symfony/mailer": "^4.4 || 5.0.* || 5.1.*",
"symfony/mailer": "4.4.*",

Because that is how you have requested the package in the core-bundle/composer.json file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the dependencies now. Since Symfony 5.0 is EoL, I have changed it to ^4.4 || 5.1.*. Note: I also updated the extra.symfony.require accordingly. This is something we have to change for contao/managed-edition as well.

composer.json Outdated
@@ -99,14 +99,14 @@
"symfony/http-foundation": "4.4.*",
"symfony/http-kernel": "4.4.*",
"symfony/lock": "4.4.*",
"symfony/mailer": "^4.4 || 5.1.*",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"symfony/mailer": "^4.4 || 5.1.*",
"symfony/mailer": "4.4.*",

This is still wrong. We are not yet supporting Symfony 5, therefore please use the same version constraint as we do for all other Symfony packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we could support individual components of Symfony 5, in my opinion.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe. We can leave the PR open until we have discussed it or we can adjust it and merge it now. 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather have it merged now, so that we can move on to #1830 😁 . Changed in aa46267

Copy link
Member
@bytehead bytehead left a comment

Choose a reason for hiding this comment

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

Nice! 💪

@leofeyer leofeyer merged commit d36c2d9 into contao:master Jul 1, 2020
@leofeyer
Copy link
Member
leofeyer commented Jul 1, 2020

Thank you @fritzmg.

@fritzmg fritzmg mentioned this pull request Jul 1, 2020
8 tasks
leofeyer added a commit that referenced this pull request Jul 17, 2020
Description
-----------

| Q                | A
| -----------------| ---
| Fixed issues     | Fixes contao/core-bundle#1613
| Docs PR or issue | contao/docs#465

_Note:_ this PR depends and is based on #1829. It will be rebased, once #1829 got merged.

This is the alternative version of #1469, based on the [Symfony Mailer Component](https://symfony.com/doc/4.4/mailer.html) instead of the Swiftmailer Bundle. It makes the configured `framework.mailer.transports` selectable in the back end. Example:

```yml
# config/config.yml
framework:
  mailer:
    transports: 
      app: smtps://app@example.org:foobar@example.org:465
      page: smtps://page@example.org:foobar@example.org:465
      forms: smtps://forms@example.org:foobar@example.org:465
      newsletter: smtps://newsletter@example.org:foobar@example.org:465

contao:
  mailer:
    transports:
      page: ~
      forms: ~
      newsletter: ~
```

<img src="https://user-images.githubusercontent.com/4970961/84607561-e3a06d00-aea5-11ea-9f89-daac495b8a85.png" alt="mailer_transport_01" width="576">

_Note:_ only the transports configured in `contao.mailer.transports` will be available for selection.

You can also provide translations:

```yml
# translations/mailer_transports.en.yml
page: 'Page'
forms: 'Forms'
newsletter: 'Newsletters'
```

<img src="https://user-images.githubusercontent.com/4970961/84607577-f7e46a00-aea5-11ea-9cd1-59271843609b.png" alt="mailer_transport_02" width="576">

And you can override the `From` address for each transport in the Contao configuration:

```yml
contao:
  mailer:
    transports:
      page:
        from: Contao Page <page@example.org>
      forms:
        from: Contao Forms <forms@example.org>
      newsletter:
        from: Contao Newsletter <newsletter@example.org>
```

<img src="https://user-images.githubusercontent.com/4970961/84607478-670d8e80-aea5-11ea-9bdd-1cb2b090fd5a.png" alt="mailer_transport_03" width="576">

_Note:_ only the transports configured in `contao.mailer.transports` will be available for selection.

Using the Symfony Mailer Component for this seems more elegant to me, since it requires no change whatsoever in the `\Contao\Email` class ([see the comparison](fritzmg/contao@feature/symfony-mailer...feature/available-symfony-mailers)). With the Symfony Mailer Component, the transport to be used is simply chosen with an `X-Transport` header in the email message itself.

This PR decorates the `mailer` service and automatically sets an `X-Transport` header based on the website root settings - and automatically overrides the `From` address based on the chosen transport.

Commits
-------

fd3da56 switch to symfony/mailer
6c21d67 change MAILER_DSN back to MAILER_URL
eeaea32 dynamically add default mailer
6ad300c add \Swift_Mailer fallback
71a5553 use MAILER_DSN with MAILER_URL fallback
cc34824 remove Email deprecation
8c2480a increase symfony/mailer dependency
3926766 switch to symfony/mailer
3fd2799 dynamically add default mailer
43aa11c provide mailer transport selection and from override
c38e864 add missing model property
8cee6db fix AvailableTransportsTest
08c9b20 fix code style
a8d9b59 fix yml style
ca47801 only show configured mailer transports within Contao
e4f774f change translation domain
1722e81 restore previous version requirement
21b55d2 code style fix
1abfc38 rename mailer_transport DCA field to mailerTransport
af8563c use Annotations for mailerTransport options callback
058da89 implement some early outs
4585e9d add missing model methods
a2da52c fix code style
f802003 Merge remote-tracking branch 'origin/master' into feature/available-symfony-mailers
a5d7c74 add more unit tests
94203d7 use assertSame
38c3de9 improve testAnnotatedCallbacks test
3ca8e02 Rearrange the form fields in the back end
33f38c4 add missing methods
b7159fe change wording to 'mailer transport'
ba4f423 merge with master
b8ffb36 Apply suggestions from code review

Co-authored-by: Leo Feyer <github@contao.org>
AlexejKossmann pushed a commit to AlexejKossmann/contao that referenced this pull request Apr 6, 2021
Description
-----------

This PR replaces the usage of `symfony/swiftmailer-bundle` with `symfony/mailer`. It serves as a basis for a follow-up PR for the available mailer selection based on the [Symfony Mailer Component](https://symfony.com/doc/4.4/mailer.html) (which would replace the existing draft contao#1469 based on the Swiftmailer Bundle).

The switch is pretty straight forward actually. However there are some things to consider which I will comment on in the files view.

Commits
-------

38ae891 switch to symfony/mailer
f7ac5f4 make port optional
d3243e2 change MAILER_DSN back to MAILER_URL
73a7fd5 dynamically add default mailer
6ddec29 fix code style
d820da6 add \Swift_Mailer fallback
c793448 fix automatic embedding of images
525dfd6 add comment
db5e1ba use MAILER_DSN with MAILER_URL fallback
193821a remove Email deprecation
44536cd allow query options from MAILER_URL
c905ddc increase symfony/mailer dependency
8906865 add comment about why we add default mailer dynamically
399d6b5 add comment about the gmail transport
21983dc add invalid argument exception
5821447 code style fix
3981e8c update dependencies
181a9c3 update dependencies
c8b8f42 update dependencies
aa46267 only allow Symfony 4.4
AlexejKossmann pushed a commit to AlexejKossmann/contao that referenced this pull request Apr 6, 2021
…tao#1830)

Description
-----------

| Q                | A
| -----------------| ---
| Fixed issues     | Fixes contao/core-bundle#1613
| Docs PR or issue | contao/docs#465

_Note:_ this PR depends and is based on contao#1829. It will be rebased, once contao#1829 got merged.

This is the alternative version of contao#1469, based on the [Symfony Mailer Component](https://symfony.com/doc/4.4/mailer.html) instead of the Swiftmailer Bundle. It makes the configured `framework.mailer.transports` selectable in the back end. Example:

```yml
# config/config.yml
framework:
  mailer:
    transports:
      app: smtps://app@example.org:foobar@example.org:465
      page: smtps://page@example.org:foobar@example.org:465
      forms: smtps://forms@example.org:foobar@example.org:465
      newsletter: smtps://newsletter@example.org:foobar@example.org:465

contao:
  mailer:
    transports:
      page: ~
      forms: ~
      newsletter: ~
```

<img src="https://user-images.githubusercontent.com/4970961/84607561-e3a06d00-aea5-11ea-9f89-daac495b8a85.png" alt="mailer_transport_01" width="576">

_Note:_ only the transports configured in `contao.mailer.transports` will be available for selection.

You can also provide translations:

```yml
# translations/mailer_transports.en.yml
page: 'Page'
forms: 'Forms'
newsletter: 'Newsletters'
```

<img src="https://user-images.githubusercontent.com/4970961/84607577-f7e46a00-aea5-11ea-9cd1-59271843609b.png" alt="mailer_transport_02" width="576">

And you can override the `From` address for each transport in the Contao configuration:

```yml
contao:
  mailer:
    transports:
      page:
        from: Contao Page <page@example.org>
      forms:
        from: Contao Forms <forms@example.org>
      newsletter:
        from: Contao Newsletter <newsletter@example.org>
```

<img src="https://user-images.githubusercontent.com/4970961/84607478-670d8e80-aea5-11ea-9bdd-1cb2b090fd5a.png" alt="mailer_transport_03" width="576">

_Note:_ only the transports configured in `contao.mailer.transports` will be available for selection.

Using the Symfony Mailer Component for this seems more elegant to me, since it requires no change whatsoever in the `\Contao\Email` class ([see the comparison](fritzmg/contao@feature/symfony-mailer...feature/available-symfony-mailers)). With the Symfony Mailer Component, the transport to be used is simply chosen with an `X-Transport` header in the email message itself.

This PR decorates the `mailer` service and automatically sets an `X-Transport` header based on the website root settings - and automatically overrides the `From` address based on the chosen transport.

Commits
-------

fd3da56 switch to symfony/mailer
6c21d67 change MAILER_DSN back to MAILER_URL
eeaea32 dynamically add default mailer
6ad300c add \Swift_Mailer fallback
71a5553 use MAILER_DSN with MAILER_URL fallback
cc34824 remove Email deprecation
8c2480a increase symfony/mailer dependency
3926766 switch to symfony/mailer
3fd2799 dynamically add default mailer
43aa11c provide mailer transport selection and from override
c38e864 add missing model property
8cee6db fix AvailableTransportsTest
08c9b20 fix code style
a8d9b59 fix yml style
ca47801 only show configured mailer transports within Contao
e4f774f change translation domain
1722e81 restore previous version requirement
21b55d2 code style fix
1abfc38 rename mailer_transport DCA field to mailerTransport
af8563c use Annotations for mailerTransport options callback
058da89 implement some early outs
4585e9d add missing model methods
a2da52c fix code style
f802003 Merge remote-tracking branch 'origin/master' into feature/available-symfony-mailers
a5d7c74 add more unit tests
94203d7 use assertSame
38c3de9 improve testAnnotatedCallbacks test
3ca8e02 Rearrange the form fields in the back end
33f38c4 add missing methods
b7159fe change wording to 'mailer transport'
ba4f423 merge with master
b8ffb36 Apply suggestions from code review

Co-authored-by: Leo Feyer <github@contao.org>
@fritzmg fritzmg deleted the feature/symfony-mailer branch October 24, 2021 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0