Add symfony/mailer into core

There are already existing issues for html2text, I'm not sure about mixing the two.

The question is if we add html2text, what do we do with our own html 2 text code. It would allow us to replace a massive, ugly, untested chunk of code in core. But there is no standard for this transformation afaik, so the result of the library is completely different. see #2830384: Deprecate MailFormatHelper in favor of html2text/html2text library. So we need to figure out if the exact structure of the result is an API (and we need a BC setting or something) or if we can just switch it out.

And yes, we should use the symfony mailer component, which is stable since symfony 4.4: https://symfony.com/doc/4.4/mailer.html

And we should reach out the the current swiftmailer module maintainer to come up with a plan and what exactly would be in core. Because the module mostly does quite a lot of HTML mail related processing that we'll likely not be able to bring into core entirely.

Here's a proof of concept. I'm sure I didn't properly add the mailer component as a dependency ;)

I'm using the new native transport factory, that will automatically use sendmail or smtp configuration, so that should also work on windows.

I was able to send a mail locally with mailhog sendmail wrapper configured in php.ini. Only problem was that symfony checks that you pass in -t and I hadn't configured it like that in php.ini.

This could be a first step, and a follow-up could then add a similar transport configuration settings UI that swiftmailer.module currently provides.

AFAICS, changing the html2text implementation could be a completely separate issue, as well as supporting HTML mails. The goal of this would be to just switch the transport with similar behavior as we have now (it's not the same though, it does not use mail() but sendmail directly).

Obviously the mailer and transport should be services, this is just a quick & dirty proof of concept.

+++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/SymfonyMailer.php
@@ -0,0 +1,73 @@
+      }
+      $line_endings = Settings::get('mail_line_endings', PHP_EOL);
+      // Prepare mail commands.
+      $mail_subject = (new UnstructuredHeader('subject', $message['subject']))->getBodyAsString();
+      // Note: email uses CRLF for line-endings. PHP's API requires LF
+      // on Unix and CRLF on Windows. Drupal automatically guesses the
+      // line-ending format appropriate for your system. If you need to
+      // override this, adjust $settings['mail_line_endings'] in settings.php.
+      $mail_body = preg_replace('@\r?\n@', $line_endings, $message['body']);
+      // For headers, PHP's API suggests that we use CRLF normally,
+      // but some MTAs incorrectly replace LF with CRLF. See #234403.
+      $mail_headers = str_replace("\r\n", "\n", $headers->toString());
+      $mail_subject = str_replace("\r\n", "\n", $mail_subject);
+
+      $email

I wasn't sure if any of this is necessary, possibly we can just drop it, making this even simpler.

One thing to check would be the Return-Path header. I saw that I ended up with two return-path e-mails.

One interesting thing on replacing hook_mail is that right now, drupal core doesn't care about HTML, but you _can_ alter the mail and make it a nice HTML mail with swiftmailer for example. When we move to directly creating Email objects as the API, then we lose that ability as the Email object has explicit text() and html() methods. It's designed for a different use case where the application/you decide what kind of mail you want to send and there is no need to alter.

We could keep MailManager and add a method like send(Email $email, array $context) or whatever, that would invoke an event that would still alllow you to customize mails sent by other modules. And that's a problem for several steps/issues further down the line.

@Berdir #11,

One thing to check would be the Return-Path header. I saw that I ended up with two return-path e-mails.

The Return-Path header is added by the final delivery agent and set to the envelope sender email address (SMTP MAIL FROM command). Drupal also adds the Return-Path header directly, presumably in an attempt to ensure delivery on some mail systems, however this isn't correct and is in violation of the RFC. See: https://datatracker.ietf.org/doc/html/rfc5321#section-4.4

I've seen duplicate Return-Path headers with plain Drupal, SMTP auth module and SwiftMailer.

With the change of the mail system, this might be a good time to look at how Drupal handles this header. See #3055296: Setting the "Return-Path" header doesn't follow RFC 5321

Here is a new contrib module Symfony Mailer that prototypes a more ambitious approach that could eventually be adopted into Drupal Core.

It defines an entirely new mail interface that would allow native Drupal Core support for advanced features including HTML mails, templating, theming with inline CSS, file attachments and more. There could be a back-compatibility shim (not yet written).

Comments welcome😃.

This isn't really postponed on anything afaik.

#10 no longer applies, NW for that and the issue summary update.

The patch in this issue has the advantage of being very simple, but also it doesn't yet add much advantage. It would technically use symfony/mailer, but it would bypass most of the features - for example the headers are pre-built then added as UnstructuredHeader. It would not support HTML email. It would retain some bugs and oddities in MailManager and PhpMail. There are no configuration options, and the transport is hard-coded as NativeMailer (which BTW is documented as Not recommended, prefer Sendmail).

The Symfony Mailer module does a lot more. It could eventually be integrated in part into Core. I feel it would be valuable to have a discussion on the direction for Core to move forward, probably in the parent META issue.

Reroll of #10 and switch to MR workflow, still needs work.

Added mailer and transport to core services. I suggest to make the mailer a lazy service.

Tentatively marking mailer.transport as non-public. Also added mail_dsn key in system.site config. This is used by the mailer transport factory to construct any transport supported by symfony mailer out of the box. Modules providing even more specialized transports might want to replace the factory and the mailer transport service.

Use the following commands to enable the mail plugin (and change the transport to mailhog):

drush config:set system.mail interface.default symfony_mailer
drush config:set system.site mail_dsn smtp://localhost:1025

(n.b. drush doesn't seem to work for me right now with the 11.x branch, use config export / import web interface if that problem persists...)

Seeking feedback on the service layout and the config now. Leaving state at needs work. As pointed out by @AdamPS the code in the plugin definitely needs some more love.

Applied the fix from #3226117: Uncaught RfcComplianceException when email From name contains a comma and cleaned up the plugin code a bit. Still needs work (plugin test similar to PhpMailTest).

Removed the lazy flag. Will try to readd it after the other pieces are in place.

So, took a step back on this and reduced the changes a bit: I feel that we shouldn't expose the mailer component as part of the initial patch in the service container. Doing so has some tricky consequences. From this point in time, the mailer service is (official) API and even more important the MessageEvent is as well.

MessageEvent will be a perfect candidate to eventually replace hook_mail and hook_mail_alter. However, I'd like to avoid mixing call paths from the old API (hooks acting on mail arrays) and the new API (event subscribers acting on subclasses of Email).

The Symfony Mailer component will bring many benefits to Drupal core over time. But let's concentrate on tackling those issues one by one.

The minimalistic mail plugin in the current PR can be used as a drop-in replacement for php mail. With the additional benefit that we can start using any transport supported by Symfony Mailer.

Let's get that in rather sooner than later. Needs review.

Issue summary still needs to be updated I believe.

with the change to the schema possible we made need an upgrade path also.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased

Great the new code seems much better, thanks.

1)

HTML email is out-of-scope for this issue. I suggest to tackle that in a follow-up.

I feel that would be a mistake. Supporting HTML is a simple change to make now - it would hardly even increase the code complexity at all. It seems desirable for the vast majority of sites who take the trouble to enable a non-standard plug-in. Why not do it from the start?? Changing from plain text emails to HTML would be a disruptive (non-BC) change.

2) On the other hand, from my experience developing Drupal Symfony Mailer, I would say that writing the code for transports is complex and I can see various potential problems with the current code. IMO this is the part that we should tackle in a follow-up. If the consensus of opinion doesn't agree then I could post my comments here.

3) These comments apparently haven't been addressed.

We should skip some headers and allow symfony mailer to set them. See LegacyMailerHelper::SKIP_HEADERS in Drupal Symfony Mailer module.

Suggest we mark this somehow as experimental, so we keep freedom to make non-BC changes.

4) str_getcsv() is an improvement but it still seems rather too simplistic. Compare swiftmailer_parse_mailboxes(), which even itself has had some recent problems. Using a proper external maintained library seems even better.

Why not do it from the start?? Changing from plain text emails to HTML would be a disruptive (non-BC) change.

No. Adding features is not considered a BC-break.

Tagging for framework manager review, especially concerning approach and scope. Hope to get that unblocked with some consideration from a core maintainer.

I am excited to finally see the mail system improving. I think that small incremental improvements are the way to go though I am still concerned about how we provide BC and how contrib/custom modules will be able to integrate with a new mail system while still supporting the old one.

One question regarding the config, shouldn't the DSN be stored in system.mail instead of system.site?

No. Adding features is not considered a BC-break.

True we can add a new feature with a config option to enable it. However if we change the default behaviour that's a BC-break. The current patch has default plain text, whereas I imagine most people want HTML - this might become difficult to change later.

Furthermore the patch copies the bug in PhpMail #3174760: Mails resembling HTML are corrupted which seems unlikely to be fixed because it would be a BC-break. Here is what @catch said on that issue:

I kind of think we should do that, there's not really an easy way to communicate the change of behaviour here, whereas with a brand new API we could fix the bug as a side-effect and people will consciously need to switch over and (hopefully) check output then. Going to tentatively mark this postponed on that issue.

I started Drupal Symfony Mailer project to address some limitations of the existing Drupal Core mail system, and of Swiftmailer (which I am also a maintainer for). Here we have a golden opportunity to get things right, yet there seems to be a strange insistence on this issue to make the new code worse than those contrib modules, ignoring the lessons learnt there.

Anyway I've made my points, and I'm not going to keep repeating them. If needs be I can continue to use and maintain Drupal Symfony Mailer so whatever happens here it's not a problem for me.

Left some comments on the MR.

Clear +1 on not changing anything in regards to HTML mails here. It really is not simple. HTML mails need a wrapper HTML template which I'm sure would come with endless bikeshedding, it needs tests, and you need ways for the opposite of what this does, meaning converting non-HTML mail content to HTML which has security aspects, it would change how mails look on existing sites and so on.

This is _not_ introducing a new mail API, it's a drop-in replacement with the existing mail API that just uses a more modern API internally. That it's behavior is as close as possible to PhpMail is a good thing, as it allows sites that are not using swiftmailer/symfony_mailer modules to switch. The goal of this is issue is not to replace either of those modules. We're many steps away from that. If anything, what we can realistically replace is modules like smtp with a bit of UI, that could live in mailsystem.module for now until core exposes that setting.

Once this first step is done, then we can look into opt-in support of HTML mails through the existing system but it IMHO probably makes more sense to design a new Mail API that bypasses the current hook system entirely.

Marked the plugin as experimental in the label plugin property. This will show up in the contrib Mail System configuration.

The Drupal Symfony Mailer integration module doesn't provide a @Mail plugin but instead replaces plugin.manager.mail entirely. Thus, when Drupal Symfony Mailer module is enabled, @Mail plugins do not have any effect at all. That makes things easier for core, since there is no need to further specify whether the plugin in the current MR is from core or from contrib.

Took a closer look at the header issue brought up by @AdamPS after #26:

We should skip some headers and allow symfony mailer to set them. See LegacyMailerHelper::SKIP_HEADERS in Drupal Symfony Mailer module.

Symfony Mailer requires control over Content-Type and Content-Transfer-Encoding headers. Regrettably it lacks support for soft wrapped text (format=flowed). Thus I had to remove the call to MailFormatHelper::wrapMail().

Adding issue credits for @AdamPS for insightful comments.

With the new service and new composer dependency think it could use a change record.

Don't mind marking that after and putting in committer queue.

CR draft.

Great many thanks @znerol for the great work. Please I hope you can accept my apologies for the tone of my earlier comments - I get myself so fixed on a single idea sometimes that I can't see any other options.

@Berdir thanks for explaining why we don't want to do HTML email here.

Here is my summary of things we could still consider.

1) I now understand that we want a drop-in replacement for PhpMail - but do we definitely want to copy it's bug #3174760: Mails resembling HTML are corrupted? Once we have released this new mail plugin, there is a BC impact to change later. Swiftmailer and Drupal Symfony Mailer have both fixed this bug. The linked issue has a simple patch that we could take. @catch marked the linked issue as postponed on this one, suggesting it be fixed here.

2) Transport DSNs have security implications. With sendmail transport you can pass the command parameter to run any system command. Of course the risk all depends on who has access to control the DSN, however it seems that someone is likely to write a GUI to expose it in contrib/custom software. In Drupal Symfony Mailer we protected against this using a variable in settings.php. It would be great if the core and contrib variants could agree on a common security mechanism (could be a follow-up issue).

3) The code as it stands doesn't allow for custom mailer transports beyond the ones hard-coded into the Transport class. In Drupal Symfony Mailer we created a way to allow that. Again it would be great if the core and contrib variants could agree on the mechanism (could be a follow-up issue).

4) Drupal Symfony Mailer includes a detailed GUI for configuring transport DSN, with form plug-ins for each of the different underlying transports. What we found is that the natural structure for storing DSN configuration is to use a separate field for each of the component parameters (see below). We also ended up with the possibility for multiple configured transports, which could be used for loadbalancing/failover or for different email types. This suggests that in the end we would need more than a single mailer_dsn config setting. OK so how does this all affect us now? Maybe not at all. Maybe we could after all call the parameter mailer_default_dsn and create a hook to alter it (the hook could be a follow-up issue). Maybe something else???

$ <strong>d cget symfony_mailer.mailer_transport.swiftmailer</strong>
langcode: en
status: true
dependencies: {  }
id: swiftmailer
label: 'Imported from swiftmailer'
plugin: smtp
configuration:
  user: XXX
  pass: XXX
  host: XXX
  port: 465
  query:
    verify_peer: true
    local_domain: ''
    restart_threshold: null
    restart_threshold_sleep: null
    ping_threshold: null

Great many thanks @znerol for the great work. Please I hope you can accept my apologies for the tone of my earlier comments - I get myself so fixed on a single idea sometimes that I can't see any other options.

No worries, @AdamPS you are doing fantastic work in contrib and in the core issue queue.

1) I now understand that we want a drop-in replacement for PhpMail - but do we definitely want to copy it's bug #3174760: [PP-1] Mails resembling HTML are corrupted? Once we have released this new mail plugin, there is a BC impact to change later. Swiftmailer and Drupal Symfony Mailer have both fixed this bug. The linked issue has a simple patch that we could take. @catch marked the linked issue as postponed on this one, suggesting it be fixed here.

The goal of this issue is to keep the cost of switching away from PHP mail plugin to Symfony mailer transports to zero for most sites. The Symfony mailer mail plugin is not new API but just provides access to Symfony mailer transports to code using the legacy API.

That said, I think it is sensible to fix the plain text conversion here. We already ditched the soft wrapping anyway.

2) Transport DSNs have security implications. With sendmail transport you can pass the command parameter to run any system command. Of course the risk all depends on who has access to control the DSN, however it seems that someone is likely to write a GUI to expose it in contrib/custom software. In Drupal Symfony Mailer we protected against this using a variable in settings.php. It would be great if the core and contrib variants could agree on a common security mechanism (could be a follow-up issue).

Good to know. Definitely needs to be considered when a configuration UI is built (no matter whether this happens in core or contrib).

3) The code as it stands doesn't allow for custom mailer transports beyond the ones hard-coded into the Transport class. In Drupal Symfony Mailer we created a way to allow that. Again it would be great if the core and contrib variants could agree on the mechanism (could be a follow-up issue).

Correct. The transports available from Symfony will cover the requirements of most sites. More exotic use can be solved in contrib or custom modules. For the moment by supplying their own mail plugin. Later by overriding the transport service (see below).

4) Drupal Symfony Mailer includes a detailed GUI for configuring transport DSN, with form plug-ins for each of the different underlying transports. What we found is that the natural structure for storing DSN configuration is to use a separate field for each of the component parameters (see below).

I guess many people would appreciate a plugin based way to configure the DSN.

We also ended up with the possibility for multiple configured transports, which could be used for loadbalancing/failover or for different email types. This suggests that in the end we would need more than a single mailer_dsn config setting. OK so how does this all affect us now? Maybe not at all. Maybe we could after all call the parameter mailer_default_dsn and create a hook to alter it (the hook could be a follow-up issue). Maybe something else???

A glimpse of how the services structure could look like when the new API is introduced in a follow-up is shown in commit 0da26475. With this structure, the only thing contrib or custom code needs to do in order to customize the transport is to replace the mailer.transport service.

Note that Symfony provides wrapper transports for round-robin and fail-over scenarios. Something similar could be done in a contrib / custom module to redirect mails to one or the other actual transport depending on their origin, type or destination.

I very much hope it is enough if core provides only one transport (service) configured by only one DSN. More complicated use cases are better left to contrib and custom modules.

Thanks @znerol

Actions

1. I propose we document mailer_dsn to say that any special characters within passwords and keys must be URL encoded. See #3279696: For DSN transport, document to escape special characters in keys/passwords.
2. Apply the patch from #3174760: Mails resembling HTML are corrupted.
3. Raise an issue to factor out the transport service.

Discussion

So we share the vision of extensibility, with different possible ways forward.

Note that Symfony provides wrapper transports for round-robin and fail-over

Yes indeed, and we can use them with the patch in this issue using a special syntax in the transport dsn - it doesn't even need custom or contrib code.

Later by overriding the transport service (see below).

O, but I guess what we want to avoid is multiple contrib modules overriding the transport service, because then it's only possible to install one of them. Instead we'd like a single contrib override of the transport service (somewhat like mailsystem module) that provides a plug-in architecture for other modules to add a mailer transport back-end or GUI.

Drupal Symfony Mailer provides an excellent starting point for this, and I think it would be feasible to refactor the code into a separate module or sub-module that can be used for either with the core mail plug-in or with Drupal Symfony Mailer itself. This is a reasonably well-contained and small piece of code and in the longer term, I feel it would be advantageous to introduce it into core - however we can see😃.

Transport DSN security implications needs to be considered when a configuration UI is built (no matter whether this happens in core or contrib).

OK, but putting the security mechanism in the GUI layer is potentially risky as then badly written contrib/custom code can bypass it and introduce security issues. It feels much better to put the security into the core, i.e. into the above overridden transport service.

It is a Plugin, if anyone wants to do something fancy it's easy to subclass and overwrite the method. That can be a separate Plugin and mailsystem and even core provides mechanisms to use specific ones for specific cases. Please lets keep this simple. we can define entirely different mechanisms and configurations for a new mail API key without BC concerns.

Security concerns will need sign off from core maintainers and maybe the security Team. we can maybe leverage config validation API, but if it's protected by a restricted permission then being able to set an insecure value is not a security issue.

Added a security considerations section to the issue summary. Requesting a review by a security team member in order to decide whether or not paths to binaries are acceptable in config.

Thanks for the replies. @Berdir NB everything in the "discussion" part has no effect on this patch, and I have no BC concerns here. All I was saying is that:

1. This patch provides no GUI for transports
2. Symfony Mailer Library creates an architecture with transport plug-ins however this patch does not allow introducing a custom plug-in.

In both cases for the solution you would currently have to create a sub-classed mail plug-in (or override the upcoming mailer transport service). This seems inelegant, and creates problems with multiple modules clashing, e.g. module A creates transport override with smtp GUI, module B creates transport override with sendmail GUI, and a site wants failover between the two.

I suggested code that mostly already exists in Drupal Symfony Mailer can solve both problems (creating plug-ins for transport GUI), allowing commonality between the core and contrib integrations of Symfony Mailer. If you believe this doesn't belong in core, sure. Or are actually recommending that it isn't necessary even in contrib?

I spotted one more thing:

Drupal Symfony Mailer requires "symfony/mailer": "^5.3 || ^6.0"
Core recommended requires "symfony/mailer": "v6.3.0"

So this patch would force sites using core recommended and symfony/mailer v5.x to a major upgrade of symfony/mailer. This should presumably be documented in the release note, and we should likely introduce this patch in a minor release rather than a patch release.

Documented this in the issue summary. I'd like to delegate the decision about target releases to the people who are actually responsible for that part (i.e., core committers). Note that contrib modules may specify core compatibility using the core_version_requirement key in their info.yml file. Thus, a contrib module may provide different versions in order to ensure compatibility with various core versions.

> creates problems with multiple modules clashing, e.g. module A creates transport override with smtp GUI, module B creates transport override with sendmail GUI, and a site wants failover between the two.

If modules override the method to initialize the transport, they can also use a different setting, so two different plugins shouldn't clash then. And sure, it's not that elegant, but it's already way more elegant than what a module like smtp has to do now and a step forward.

> I suggested code that mostly already exists in Drupal Symfony Mailer can solve both problems (creating plug-ins for transport GUI), allowing commonality between the core and contrib integrations of Symfony Mailer. If you believe this doesn't belong in core, sure. Or are actually recommending that it isn't necessary even in contrib?

All I'm saying is that it doesn't belong in this issue, which we agree on I think. I haven't reviewed the specific implementation in symfony_mailer, but I agree that it makes sense to have an extensible system to configure the transport later, the hardcoded settings in swiftmailer always bugged me ;) I guess that will be one aspect of the replacement of the current mail API in core. Happy to discuss in later issues how much of that should be in core and how much in contrib. Similar to now, core could provide the plugin type with very basic options and a UI for SMTP and other configurations could be in contrib.

I'd like to go back to one earlier topic, native vs sendmail, per #21:

> the transport is hard-coded as NativeMailer (which BTW is documented as Not recommended, prefer Sendmail).

Yes, that is documented here: https://symfony.com/doc/current/mailer.html#using-built-in-transports and it says that, but it also says "if possible". I'm unsure if we can do that. My understanding is that sending mails would then not work on Windows without manually installing sendmail. I think now that works out of the box, so this might be a deal-breaker, maybe only once this is a stable/default replacement, maybe already now, I really don't know, that's not a decision that I can make, but I think it's one that needs to happen. Finding people who can test on Windows (not WSL2, actual native windows) is always very hard. A possible way forward might be to just document that in the change record (and a documentation page that we need to pass the documentation gate I think) and have a follow-up to discuss that further and see if anyone complains? ;)

> So this patch would force sites using core recommended and symfony/mailer v5.x to a major upgrade of symfony/mailer. This should presumably be documented in the release note, and we should likely introduce this patch in a minor release rather than a patch release.

Drupal 10 requires Symfony 6, so it makes sense to me to only allow that version like all other Symfony dependencies. Anyone on D10 is likely already using the 6.x version. But sure, doesn't hurt to specify that core requires symfony/mailer 6 in the change record. And recommended can always only require one specific version, that's exactly what core-recommended exists for.

Took a look at the mechanism how transports are constructed with the native scheme:

NativeTransportFactory follows this logic:

• If sendmail_path ini is set, then a SendmailTransport is instantiated with that command.
• Else throw unless the os is windows
• Construct an SmtpTransport with the values from smtp and smtp_port ini settings.

This logic is all sane. The problematic thing is that Symfony mailer prefers /usr/sbin/sendmail -bs and PHP defaults to /usr/sbin/sendmail -t -i.

Thanks @Berdir. I feel it would be great to have a small group of experts to advise on overall strategy for Drupal Symfony Mailer in contrib. As function is added to Core then Contrib should adapt, so that it can continue to be used and fill in the gaps that Core chooses not to include.

As a reminder, Symfony docs has this warning message:

When using native://default, if php.ini uses the sendmail -t command, you won't have error reporting and Bcc headers won't be removed. It's highly recommended to NOT use native://default as you cannot control how sendmail is configured (prefer using sendmail://default if possible).

Perhaps we could use Native only on Windows??

In my testing of splitting addresses, str_getcsv() is an improvement on explode() however it's not 100%:
- it works if the display name contains a comma
- if fails if the display name contains a less-than (because it removes the quotes within each address)

However I guess it's the same as already done elsewhere in Core.

The Symfony Mailer Framework Bundle is setting up transports like this:

        $loader->load('mailer.php');
        $loader->load('mailer_transports.php');
        if (!\count($config['transports']) && null === $config['dsn']) {
            $config['dsn'] = 'smtp://null';
        }

The $config variable is an array of container parameters with all subkeys of the mailer key. In absence of any configuration, a symfony based application defaults to smtp://null.

I think we should follow that example. SMTP on localhost works out of the box in many production environments. For development, people tend to lean towards something docker based (e.g., ddev which is capable of injecting the necessary mailhog settings already).

Regrettably smtp://null doesn't work at all (fails with Symfony\Component\Mailer\Exception\TransportException: Connection could not be established with host "ssl://null:465": stream_socket_client(): php_network_getaddresses: getaddrinfo for null failed: Name or service not known).

Also smtp://localhost:25 doesn't work neither reliably with a local postfix. The problem here is that it tries to use STARTLS and fails to verify the certificate CN (Symfony\Component\Mailer\Exception\TransportException: Unable to connect with STARTTLS: stream_socket_enable_crypto(): Peer certificate CN=`<my-hostname->' did not match expected CN=`localhost'). Thus, back to sendmail://default.

In that case I prefer to instruct windows users to specify the SMTP DSN directly in system.mail mailer_dsn.

@znerol,

Also smtp://localhost:25 doesn't work neither reliably with a local postfix. The problem here is that it tries to use STARTLS and fails to verify the certificate CN (Symfony\Component\Mailer\Exception\TransportException: Unable to connect with STARTTLS: stream_socket_enable_crypto(): Peer certificate CN=`' did not match expected CN=`localhost').

This can happen if the SMTP server responds that it supports STARTTLS but only has a self-signed certificate. Symfony Mailer (and other mail libraries) usually default to using STARTTLS if the server supports it.

There are a few ways around it.

1. Disable STARTTLS on your local SMTP server.
2. Disable verify peer in Symfony Mailer. E.g. smtp://localhost:25?verify_peer=0
3. Add a trusted certificate to your local SMTP server.

I don't think we should try changing the default transport here; if this is a near drop-in replacement for the PHP mail plugin then by default it should function as close to the original as it can.

Regarding Windows we are intending to drop official support for Windows in production, because as far as we can determine, there are very few users running Drupal on IIS/Windows, and we get very little help with Windows-related issues in this queue: #3358248: [policy, no patch] Drop support for IIS in Drupal 11

There are a few ways around it.

Sure, the correct way is obviously to run a mailserver with a proper certificate.

I tested this on a rather fresh Debian. Postfix runs with a self-signed TLS cert by default when installed from packages. That will be the case on many hosts. Hence, lots of people will run into that problem if Drupal ships with the DSN set to smtp://localhost:25.

I'd prefer Drupal to use a default transport which doesn't require people to mess with their MTA config.

I'd prefer Drupal to use a default transport which doesn't require people to mess with their MTA config.

Makes sense. Would disabling verify peer by default be a problem? Even if it was only applied to localhost.

Would disabling verify peer by default be a problem?

Yes. I feel that core shouldn't be disabling security settings in its defaults. I can only speak for myself, but I often look through core code for best practice if I'm unsure how to solve a particular problem in a contrib or a custom extension. If core promotes verify_peer=false, then silly people like me might start thinking that this is good thing.

Also sendmail://default is more likely to behave similar to the PHP mail() function on *nix platforms. Which is inline with the direction suggested by @longwave in #67.

Before 2016 Drupal did not verify peers on SSL connections, and this was fixed in #1081192: Verify peer on HTTPS if cURL available (but be careful of built-in cert bundles in the codebase). I don't think we should undo some of that here; SMTP is a bit different but as stated in #70 core should be demonstrating best practices and not provide insecure defaults.

I know I brought the topic up, but IMHO it's fine to default to sendmail for this initial/experimental/non-default mail plugin, if people who use this need to manually change configuration then I guess we can also expect them to read about windows and other special cases where the default doesn't work.

I think we do need a proper documentation page/section for this, not just the change record though.

It's a bit different when it will be the default option, but I think we can discuss that in a follow-up that blocks making it non-experimental and mention that in the meta.

@longwave: I think one bit where we need committer feedback is whether it's OK to have an experimental plugin in core just as-is, or if we need to put it in an experimental module.

Discussed the experimental plugin with @catch. We agreed that as it is not visible via UI (except with mailsystem installed), then it is OK to add an experimental plugin directly to core as long as we also mark it @internal for now.

An experimental module would mean later having to merge that into core, and also it would add a plugin that does nothing on its own as there is no UI to configure it yet, so it could be somewhat confusing to users.

@Berdir also asked about a dependency evaluation. symfony/mailer is part of Symfony which we already heavily depend on, and which shares the same maintainers, security policies and release cycle. There is some overlap with the existing PHP mailer plugin but the whole point of this issue is to eventually replace our plugin with an improved one. We do not have individual listings of Symfony components on https://www.drupal.org/about/core/policies/core-dependency-policies-and-... therefore I don't think any further action is needed here and consider that this component is OK to add to core in this issue.

We probably should transfer commit credits from #3174760: Mails resembling HTML are corrupted.

I didn't find time for a full review, however from a quick check it looks really good. I like the really solid tests, including for "resembles HTML".

I spotted an outstanding action from #50:

I propose we document mailer_dsn to say that any special characters within passwords and keys must be URL encoded. See #3279696: For DSN transport, document to escape special characters in keys/passwords.

What does anyone think about that?

Thanks @AdamPS, rebased and added some docs to the plugin.

One extra point against adding an experimental module here is it would mean exposing it in the module interface as an experimental module, which ironically gives it more exposure than just adding the plugin to core directly.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased and resolved a conflict in system.post_update.php.

Rebased and fixed spelling of Symfony in doc comments.

MR appears to be unmeragable.

Also is this just waiting on security review? Would be nice to get this into 11.x early for 10.2 testing.

Rebased.

Going to mark it and post in #security-discussion if anyone could take a look.

Tagging with needs framework manager review since the issue is introducing a new API.

Rebased and resolved a merge conflict in system.post_update.php.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Going to restore previous status before the bot.

I raised #3384844: [PP-1] Add security checking for Symfony Mailer transports for the security concerns. I don't think we should even fix that as part of this issue - it should be after #3379794: Add symfony mailer transports to Dependency Injection Container (mail delivery layer) which is postponed on this.

So I suggest we could remove "Needs security review" from this issue, and tackle the security later. Note that core doesn't provide any GUI to change the config setting yet anyway.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Going to restore status from before. To keep in front of committers

Rerolled.

Left a review. I only touched on the single file.

If this is committed as-is it would make it difficult to override to add compatibility for Symfony Messenger (which we are actively developing in contrib — see MR's).

Also note about event dispatcher.

No, @dpi. Injecting event dispatcher and the message bus is out of scope in this issue. The goal is to get the composer dependencies in here. And additionally add an experimental mail plugin so people can start to try things out.

Proper dependency injection is worked on in #1803948: [META] Adopt the symfony mailer component. #3379794: Add symfony mailer transports to Dependency Injection Container (mail delivery layer).

Edit: correct issue link.

See also #30 for the reasoning behind leaving out event dispatcher injection.

Rerolled.

Thanks @znerol sounds good to me

This was RTBC before, so lets put it back to that.

For the record, combining adding the dependency and the mail plugin was my idea to have testable usage as well as allowing sites to actually use it for the limited cases that it does support. For example combined with an exposed UI for the setting in the mailsystem module.

But if that actually hinders the main goal (which is developing a new mail API in #1803948: [META] Adopt the symfony mailer component) and delays this from being committed then I'm more than happy to drop that and just get the library in.

@Berdir it makes sense to me. It doesn't add a lot of code, and it gives the potential for using any symfony mailer transport (with GUI in contrib). It doesn't appear to hinder the main goal.

Regarding security, I raised #3384844: [PP-1] Add security checking for Symfony Mailer transports which would be fixed after (or part of) #3379794: Add symfony mailer transports to Dependency Injection Container (mail delivery layer) which is postponed on this. On that basis do you think we can remove the "Needs security review" tag??

Yes, it makes sense to me as as well, but it's not a deal-breaker so if it helps to get this in, we could remove it. In fact, I recently found https://www.drupal.org/project/symfony_mailer_lite which sites could use in the meantime, and it does much more than just the basic transport and is specifically aimed to allow people to move away from the long deprecated swiftmailer dependency.

This issue does 3 things:

1. the composer dependency, obviously essential
2. the mailer_dsn config setting leading to #3379794: Add symfony mailer transports to Dependency Injection Container (mail delivery layer) and #3384844: [PP-1] Add security checking for Symfony Mailer transports
3. the mail plug-in, leading to #3392879: Add support for HTML email to Symfony Mailer plug-in and #3392879: Add support for HTML email to Symfony Mailer plug-in

2) Allows a UI in new contrib module Mailer Transport that can be used with Core, Drupal Symfony Mailer or even Drupal Symfony Mailer Lite. The code already exists in Drupal Symfony Mailer, it just needs to be split out.

@Berdir, this module can potentially be seen as the successor to mailsystem - would you be willing to join me as a maintainer??

3) Allows the HTML mail template to be agreed, and then it can also be used in the new mail system. Also it means we can actually send HTML email from core without any contrib modules at all.

These are all important for the meta goal of #1803948: [META] Adopt the symfony mailer component. Most of what we develop in items 2) and 3) will still be used even after the @Mail plug-in is deprecated and removed. This issue is the starting point of stage 1 of the META, and it puts in place key building blocks to make the following stages easier. Otherwise stage 2 is trying to do too much at once, it becomes even harder.

So I feel that everything in this patch is important. I guess if people feel it's too big we could split it in half. It would definitely be useful to understand what needs to be done to allow this issue to be committed. The overall META issue will likely end up as 20 separate issues, so we might need support from the committers to streamline/prioritise if we want to get it in any time soon.

I removed the "Needs security review" as per #102, and instead added it to #3384844: [PP-1] Add security checking for Symfony Mailer transports - if someone objects then please put it back.

Yes I also just discovered symfony_mailer_lite. It's useful to have as a piece of the overall picture. It allows sites using swiftmailer to move to something supported with minimal change.

I would expect that it can gradually be simplified as the code it contains moves elsewhere, then eventually becomes obsolete along with other @Mail plug-ins.

• The biggest part of the code looks like a straight copy of the transport code from Drupal Symfony Mailer, and this can instead be shared code in Mailer Transport module
• Once this issue is in, then the Lite module can derive from it by alter the plug-in definition and sub-classing
• After #3392879: Add support for HTML email to Symfony Mailer plug-in, more code moves out of the Lite module and into Core

The MR looks great. Great work!

Committed and pushed to 11.x and 10.2.x (after also making a mistake in the 10.2.x cherry-pick).

Excited to see what happens next with the mail system!

Amazing, thanks @longwave. I reckon what happens next is #3379794: Add symfony mailer transports to Dependency Injection Container (mail delivery layer)

Amazing!

> 2) Allows a UI in new contrib module Mailer Transport that can be used with Core, Drupal Symfony Mailer or even Drupal Symfony Mailer Lite. The code already exists in Drupal Symfony Mailer, it just needs to be split out.
> @Berdir, this module can potentially be seen as the successor to mailsystem - would you be willing to join me as a maintainer??

I feel I maintain more than enough modules already that I don't have enough time for so I won't be too sad if mailsystem eventually gets 100% replaced and deprecated, but I'll think about it, can try to do some reviews.

I can see how it could be the base for both drupal symfony mailer and drupal symfony mailer lite, but it would require config/API changes for both of them, assuming it would then manage those transport settings in its own config?

Not sure how that would work for core though in the current form, I think core would need to make selecting/deciding the Transport an official API that can be replaced before that's feasible, so after some of the follow-up issues? Or would you write the configured default transport into the core config setting?

Not sure how that would work for core though in the current form

MT (Mailer Transport) module would override one of the services introduced in #3379794: Add symfony mailer transports to Dependency Injection Container (mail delivery layer), perhaps mailer.transports. I guess it's similar to mailsystem overriding MailManager.

I can see how it could be the base for both drupal symfony mailer and drupal symfony mailer lite, but it would require config/API changes for both of them, assuming it would then manage those transport settings in its own config?

Currently the transport code from DSM is copied with minimal change into DSM-L. MT would contain that copied code which would then be removed from the other two.

Correct there would be some minor adjustments to DSM to use the mailer.transports service - which could be the Core version or the MT version (in practice I expect 99% of sites to choose the latter). I plan to do this in a new major version 2.x. Probably it's similar for DSM-L although I didn't look.

This broke HEAD on 10.2.x with a friendly

Running PHPStan on *all* files.
 ------ ----------------------------------------------------------------------- 
  Line   core/lib/Drupal/Core/Mail/Plugin/Mail/SymfonyMailer.php                
 ------ ----------------------------------------------------------------------- 
  89     Parameter $mailer of method                                            
         Drupal\Core\Mail\Plugin\Mail\SymfonyMailer::__construct() has invalid  
         type Symfony\Component\Mailer\MailerInterface.                         
  89     Property Drupal\Core\Mail\Plugin\Mail\SymfonyMailer::$mailer has       
         unknown class Symfony\Component\Mailer\MailerInterface as its type.    
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols    
  143    Method Drupal\Core\Mail\Plugin\Mail\SymfonyMailer::getMailer() has     
         invalid return type Symfony\Component\Mailer\MailerInterface.          
  157    Call to static method fromDsn() on an unknown class                    
         Symfony\Component\Mailer\Transport.                                    
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols    
  158    Instantiated class Symfony\Component\Mailer\Mailer not found.          
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols    
 ------ ----------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------- 
  Line   core/tests/Drupal/Tests/Core/Mail/Plugin/Mail/SymfonyMailerTest.php  
 ------ --------------------------------------------------------------------- 
  49     Class Symfony\Component\Mailer\MailerInterface not found.            
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  100    Class Symfony\Component\Mailer\MailerInterface not found.            
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  101    Trying to mock an undefined method send() on class                   
         Symfony\Component\Mailer\MailerInterface.                            
 ------ --------------------------------------------------------------------- 

 [ERROR] Found 8 errors      

Did not had a thorough look at it, but by the looks of the errors, it seems the 2nd attempt on the commit on 10.2.x doesn't contain the changes in composer.json and composer.lock.

Committed 75edd5e and pushed to 10.2.x. Thanks!

The dependency was added to the root composer.json instead of core/composer.json.

-    "plugin-api-version": "2.3.0"
+    "plugin-api-version": "2.6.0"

Is that an intentional change in composer.lock?

I think that is because different core developers are using different Composer versions. Not sure what difference it makes.

This is noted at https://stackoverflow.com/questions/72102111/plugin-api-version-keeps-on... but there is no explanation of what problems it can actually trigger.

Me neither, just checkin' :)

I'm sorry this is causing so much trouble.

https://getcomposer.org/changelog/1.10.0 documents it as just some metadata for 3rd party tools, so I'll proceed for now with the assumption that raising it to reflect the version of Composer used by the person who made the change won't cause any problems.

Therefore, pushed the fix to 11.x and 10.2.x.

I published the CR.

Follow-up on mailer_dsn config: #3399645: Use structured DSN instead of URI in system.mail mailer_dsn

This seems like a big deal! There's lots of sites relying on the SMTP contrib module that seemingly no longer need to? I created #3424400: Deprecate module in favor of core's built in SMTP mail handling capabilities offered in 10.2.x.