Add symfony mailer transports to Dependency Injection Container (mail delivery layer)

We have very similar code already in Drupal Symfony Mailer, added in #3332398: Allow custom TransportFactory, see TransportFactoryManager. I believe that existing code has some advantages.

1) The symfony Transports class assumes a fixed set of transports at initialisation time. I agree this could often be true, but not always - for example a hook might wish to discard a mail by setting a null transport, however the site might not have configured one.

Also looking ahead to #1803948: [META] Adopt the symfony mailer component (where hopefully we can use this same service) it seems awkward to rely on an X-Transport header when we could have a function on the Email class.

In Drupal Symfony Mailer we have Email::getTransportDsn() and use Transport::fromString(). If we are concerned about performance, then we could add caching of transports already created. This avoids the need for a mailer.transport_configuration service.

2) Transport::getDefaultFactories() already knows about the 4 factories that you added, plus 9 more. This list will likely grow over time. We can automatically load these without needing service definitions.

3) If we end up creating a new class (which Drupal Symfony Mailer did), then we could use the service_collector tag to avoid creating a new compiler class.

for example a hook might wish to discard a mail by setting a null transport

Actually for that we have SkipMailException so maybe it's OK.

Here are my updated comments (including ones from before not yet resolved keeping the same numbers):

1) The aim here is a service mailer.transport that implements TransportInterface.

We've agreed that Core supports only a single transport, with a single config setting and no UI. So it can create a very simple class that always returns the same transport. The Transports class is complex and unhelpful - it doesn't even have a default. The mailer.transport_configuration service is again over-complex for this need.

Sites that want configuration of transports should use contrib, which will replace the service with a new class. IMHO as maintainer of Drupal Symfony Mailer, again the Transports class is not helpful. It requires a fixed list of transports at initialisation, it creates them all (even though we likely only use one). It forces use of the 'X-Transport' header when we could just call something like $email->getTransportDsn(). The mailer.transport_configuration service is similarly assuming a fixed list. So we would likely ignore both classes.

My head is spinning round trying understand all the services😃: mailer.transport_factory_collection, mailer.transport_factory (which is not an implementation of TransportFactoryInterface), mailer.transport_configuration and mailer.transports I believe it could work well combined into one simple class MailerTransport. Something like this:

function createTransportFactory() { // as existing}
function getDefaultDsn() {
  // Contrib overrides this function.
  return this->config('system.mail')->get('mailer_dsn'))
}
function initialize() {
  if (!$this->transportCollection) {
    $this->transportCollection = new Transport($this->transportFactories);
    $this->defaultTransport = $this->transportCollection->fromString($this->getDefaultDsn());
  }
}
function send(......) {
  // Contrib overrides this function.
  $this->initialize();
  return $this->defaultTransport->send();
}

What do you think?

2) Transport::getDefaultFactories() already knows about the 4 factories that you added, plus 9 more. This list will likely grow over time. We can automatically load these without needing service definitions by calling it from TransportFactoryCollection::createTransportFactory().

3) Thanks for fixing.

4) As @lucashedding has already commented, we should create our own TransportInterface to give flexibility. We might create our own Email class in #3380041: Create a new email interface that decorates symfony.

Does this definitely need to be postponed? The service would be used by both #3380041: Create a new email interface that decorates symfony and #3165762: Add symfony/mailer into core. We'd have to go back and edit the code created there to use this service.

[I realise this is postponed, so perhaps it's not considered ready for review.]

My comments from above that are not yet done:

1) This MR apparently forces a fixed list of transports known at initialisation time. This would prevent setting of the transport programmatically - for example a module that wishes to force a null/test transport on a dev site.

We can instead

• Find a way to store the transport DSN on a email. This could be a class EmailWithDsn that extends the base Email class, adding a tranportDsn variable with accessors.
• For the mailer.transports service (or perhaps better named mailer.transport), create our own variation on the Transports class. It checks if the email contains a DSN, and uses that, otherwise uses the default. It calls Transport::fromString() with this DSN.
• Delete mailer.transport_configuration service, or simplify it to a mailer.transport_default service that returns the default DSN. Otherwise we create all possible transports but only use one of them.

2) Symfony provides Transport::getDefaultFactories() which would allow registration of 15 transport factories without writing code in a services.yml file. Core would not include the 3rd-party transports, however a site-builder would only need to add them to composer.json and then they would automatically work. Otherwise we'll need Drupal wrappers for each 3rd-party transport with just the services file.

For someone who doesn't know, what does "DIC" stand for?

what does "DIC" stand for?

Good question, let's make it clearer. I would say Dependency Injection Container

I think this issue is trying to do too much at once. Already the issue title is an important and complex topic. It would be amazing if we could get it in 10.2 because it would help a lot with DSM (Drupal Symfony Mailer), DSM-L, the upcoming Mailer Transport module (that they hopefully can share) and the end game of swiftmailer.

In addition this issue contains code for creating the email test framework for use with Symfony Mailer. This is another important and complex topic as what we do here affects the ease and efficiency of writing tests in Core and many contrib modules for years to come. As maintainer of both DSM and Simplenews this affects me a lot. We need to get it right in the beginning as it will become hard or even impossible to change later due to existing test code using it. I could easily write 20 points of feedback about it - starting with "How will it work with the Mailer Service (rather than this @Mail plug-in), because potentially we have lost access to the Drupal Email class that wrapped the Symfony Email class.

Please can we make the second part a follow-on issue? I propose roughly like this:

1. Keep TransportFactoryCollectionTest here as it tests the code of this issue
2. Keep SymfonyMailerTest here as it tests the code of #3165762: Add symfony/mailer into core
3. Defer MailerTransportTest, mailer‎_transport_test‎ module, AssertMailerTransportTraitTest - which are actually general tests for sending emails, that AFAICS would (almost) work without the rest of this patch.

In #13 I said

Find a way to store the transport DSN on a email.

Maybe this part isn't needed in Core - we can just do it in Contrib.

Anyway we can discuss anything if you want, otherwise I'll wait for a NR until I look again.

I raised #3395814: Trait for testing Symfony Emails and #3395815: Page to send a verification email

Wow, that was very confusing. You committed a considerable refactoring of the service architecture between me reviewing the MR and checking it out to have a look at it a few minutes later and then somehow what I was looking at was completely different :)

I think that latest commit at least partially addresses the concerns from @AdamPS in that it simplifies creating the transport to our own TransportManager and then that just "materializes" the default transport into the mailer.transportS service. Which is confusingly named, why the plural?

Current thoughts while going through this a bit.

* Agree that this is a lot code and specifically test coverage and test implementations of things. I'm not sure yet what we can feasibly split off, but if we postpone the test trait to #3395814: Trait for testing Symfony Emails then we could definitely also move the test coverage for it.
* The Originator event subscriber seems useful but what exactly makes specifically that necessary to be added here?
* I'm a bit unsure about the single transport thing. We did agree that core only needs a single transport, but for me, that was about the configuration aspect of the default transport. Having that baked into the service architecture as the mailer.transports service seems like it's going to make it harder for contrib to expand on that? Do we really need to do that, especially at this stage, also in combination with the next point.
* As it stands, with the public service, this pretty much gives us a working mailer system using symfony mailer. Looking at \Symfony\Component\Mailer\Mailer, if you don't set up the message bus stuff, it's just a very thin wrapper around the transport. So, what's stopping modules from using this service once it's committed now and then we risk breaking them? Anything we add in core.services.yml by default becomes a public and stable API unless noted otherwise. While there's discussion as to just how many layers we'll want to have on top of this in core, I think we all agree that we'll need something? Do we need to mark these services as internal/private/experimental?

Combining the last two points, I think one option is to not have that mailer.transports service and force direct transport calls to at least go through the transport manager which also gives us a place to document the API stability and when and when not you should use that?

Considering the naming of the services, I think there is still an option to have most of this in an experimental mailer module, we could then move the services without even renaming them to core at some point. The interfaces that you type on are trickier of course. But I'm not sure if I feel comfortable adding all this in its current state into core.services.yml.

> I'm a bit unsure about the single transport thing.

I was pretty confused about this too, however I think Symfony forces it upon us. I guess the reason for the plural Transports (which I don't especially like either, however it comes from the Symfony class name) is that this isn't actually necessarily a single transport. Instead it will find/create the required transport and use that to send. We have to do it this way because creating the Symfony Mailer service requires injection of a class implementing TransportInterface.

Currently DSM does something different as a workaround - it creates a Symfony Mailer object for each email sent with the required single transport. However that breaks DI and prevents use of Symfony Messenger for example.

@znerol I'd like to say a big thank you for your hard work here. I can see that you are giving your time because you want to help Contrib and Custom developers.

My role is one of the major developers in this area, and so I'm grateful. Given I'm representing your intended audience, your "customers", then I hope that you will listen to some ideas about what we might actually want😃. I haven't re-reviewed the code yet, however I have some concerns from reading the IS and recent comments. Sorry @znerol this comes out as quite a long list of things that I believe need to change. In many cases I feel I'm repeating what I've already said once or twice before, but I didn't see any response so they're still relevant.

Put the necessary services in place such that contrib and custom code can start using the mail delivery part of Symfony Mailer safely in test and in production code by simply retrieving / referencing a configured transport service from / in the container.

I'm don't agree with this statement from the IS. I think that's much too big a goal for one issue. I approve of the bullet list of things you suggest to be added to the discussion of #1803948: [META] Adopt the symfony mailer component. I don't agree with them all (for example you say "share modules providing third-party transports", but actually you don't even need a Drupal module for the 3rd-party transports already known to Symfony). We don't have to do them all in this issue, we don't even necessarily have to do them all in Core. I suggest instead this issue should do what the title says - add some services for transports to the DIC.

We are still at a very early beginning stage of Symfony Mailer in Core. We can't fully predict the needs for the end goal of the new APIs and Mailer Service. I vote that everything we add here is marked as @internal and experimental. Contrib and Custom are welcome to use it, but they need to be ready for change. We are not necessarily creating something ready for production use.

Contrib code is already using Symfony Mailer😃 on 20k sites. This issue is currently in danger of breaking that - it could conflict with service names and tags already used, and it could "undo" security critical measures already taken. Therefore let's take small simple steps that we can verify the consequences of, and do each step fully without leaving important bits out. The transport factory collection definitely feels like it belongs in Core, but we need to be careful about removing features and security compared to Contrib. At very least we should ensure there is a way for Contrib to get back the features already in use with a reasonable migration path.

Regarding the tests: I think it is important that we do not leak email during test execution. In order to ensure that this is working correctly, we have to cover that requirement with tests.

I agree that is important. However I suggest it's not part of this issue as here we don't need any tests that send emails. I suggest you could add your requirements and test code to #3395814: Trait for testing Symfony Emails. I created a detailed list of ideas and @Berdir has responded with some more good ideas. Your ideas are good too. Clearly this is a significant area for discussion and deserves a separate issue.

Still needs work because I'd like to cover FailedMessageEvent and SentMessageEvent with some tests

I see there is some discussion again about events. We had previously decided to disconnect event subscriber because we don't yet know what events/hooks/callbacks we want for Symfony Mailer in Core. This whole area is another significant discussion, and needs a separate issue. I would say it's about [PP-5]😃 as it's hard to even imagine the right events until we actually have the classes that they would live in. The current consensus is towards decorating the Symfony Email class rather than extending it, in which case these events would likely be entirely unsuitable. Sure, early adopters may listen to the Events that Symfony already provides, but it's not necessarily going to be the same when Drupal officially integrates Symfony Mailer.

using part of Symfony Mailer safely in test and in production code

I agree safety is important, so I vote we should include the fix for #3384844: [PP-1] Add security checking for Symfony Mailer transports in this issue. Otherwise, we are encouraging Contrib to expose security bugs.

> I was pretty confused about this too, however I think Symfony forces it upon us. I guess the reason for the plural Transports (which I don't especially like either, however it comes from the Symfony class name) is that this isn't actually necessarily a single transport. Instead it will find/create the required transport and use that to send. We have to do it this way because creating the Symfony Mailer service requires injection of a class implementing TransportInterface.

The default implementation, yes, but we're not actually required to use that. With a transport injected into the mailer, it's literally a one-line wrapper around transport send, it does absolutely nothing else. It only gets a little bit more complicated when using a bus.
And the latest patch doesn't use Transports anymore, it is set as a factory from TransportManager::getTransport(). I think the S is just a leftover from that refactoring.

We can easily have our own implementation that implements the Symfony Mailer Interface and inject the TransportManager service and get the transport from that. And then we can later add a feature that gets a transport DSN from Email object or an event or whatever and pass that to TransportManager::getTransport(). Or contrib can do that in its own implementation of the Mailer. It already supports that.

As a third opinion on this discussion, I'm somewhere in the middle. I fully agree that this is too much in a single issue, it's one thing to write the code, but reviewing and approving all of this is _not_ something I feel comfortable with.

The challenge is adding an incomplete/partial API to core.services.yaml that we don't want modules to use yet. Because I really think that it is too early for that and if modules start using it now, it *will* break.

> Regarding the tests: I think it is important that we do not leak email during test execution. In order to ensure that this is working correctly, we have to cover that requirement with tests.

With this I agree, we need *some* tests for sending mails and we want to make sure that we don't leak mails in tests and verify that. But what I meant specifically is that there is the new AssertMailerTransportTrait, and a AssertMailerTransportTraitTest for it ( didn't fully grok the rest of the tests so I can't really comment on them). It's a single line of code, one that we could easily inline into those early tests and then use the test framework issue to flesh out a proper test trait and commit to that as an API.

> We are still at a very early beginning stage of Symfony Mailer in Core. We can't fully predict the needs for the end goal of the new APIs and Mailer Service. I vote that everything we add here is marked as @internal and experimental. Contrib and Custom are welcome to use it, but they need to be ready for change. We are not necessarily creating something ready for production use.

+1.

> Contrib code is already using Symfony Mailer😃 on 20k sites. This issue is currently in danger of breaking that - it could conflict with service names and tags already used, and it could "undo" security critical measures already taken.

Didn't check that closely, but IMHO, if Symfony Mailer used service names and tags that are _not_ prefixed with symfony_mailer then that's on that module. All services are prefixed with mailer and the tag is mailer.transport_factory. Unless I'm missing something, the transport discovery should then run completely parallel to symfony_mailer module and not interfere with each other? And symfony_mailer simply won't switch to the core services until they are fully ready.

As a site owner I want to send emails via the SMTP protocol without installing contrib modules.

Given this product requirement, SMTP requires configuration. With the current implementation as far as I can see this will require the site owner to enter a DSN directly? Should site owners be expected to configure mail this way, or should we be providing transports via plugins with configuration forms? SMTP configuration often includes a password; this will be displayed in admin UI if the DSN is configurable directly.

I took another quick look at the code, and many of my concerns are fixed thank you.

We can easily have our own implementation that implements the Symfony Mailer Interface and inject the TransportManager service and get the transport from that. And then we can later add a feature that gets a transport DSN from Email object or an event or whatever and pass that to TransportManager::getTransport(). Or contrib can do that in its own implementation of the Mailer. It already supports that.

OK, you've lost me there. The part I'm stuck with is this. Symfony\Component\Mailer\Mailer is a final class, and it requires TransportInterface as a constructor parameter. So I agree we can override and inject anything we like, but still that code will only get the Symfony Email class in $message. I don't see how to get back to our Drupal Email class, which decorates the symfony one, rather than extending it.

Didn't check that closely, but IMHO, if Symfony Mailer used service names and tags that are _not_ prefixed with symfony_mailer then that's on that module. All services are prefixed with mailer and the tag is mailer.transport_factory. Unless I'm missing something, the transport discovery should then run completely parallel to symfony_mailer module and not interfere with each other? And symfony_mailer simply won't switch to the core services until they are fully ready.

DSM used the tag mailer.transport_factory because this is a standard indicated by Symfony mailer library for discovering transports. DSM includes a fix for #3384844: [PP-1] Add security checking for Symfony Mailer transports but Core doesn't, so this issue as it stands could weaken security of live sites (I don't know which implementation would end up taking precedence - maybe it could even be random). Please can we get the security team to decide if the linked security issue needs fixing, and if so fix it in Core also?

There is an opportunity to reduce the amount of changes almost by 50% if the OriginatorSubscriber is extracted to a separate issue.

+1
Sorry I don't really understand autowire😃. Is this going to run automatically on all sites? If so, it could break any site already using symfony mailer library.

Given this product requirement, SMTP requires configuration. With the current implementation as far as I can see this will require the site owner to enter a DSN directly?

+1
Another problem with entering a DSN directly is there are complex escaping requirements, so it's bad UX. We have a GUI with proper forms in Contrib. I guess it will be a product management decision whether to add one to Core.

> OK, you've lost me there. The part I'm stuck with is this. Symfony\Component\Mailer\Mailer is a final class, and it requires TransportInterface as a constructor parameter. So I agree we can override and inject anything we like, but still that code will only get the Symfony Email class in $message. I don't see how to get back to our Drupal Email class, which decorates the symfony one, rather than extending it.

The Email class and how we'd use it and accessing it in (transport) events is a different concern.

I was only talking about the single-transport service/injection. Yes, the default implementation is final, but all code should work against the interface, and we could and expect will provide our own implementation of SymfonyMailer that could instead of a single Transport, work with our TransportManager. Or not even use that interface, we'll see. But it seems to be that your workaround of a one-off mailer doesn't seem necessary when all it does is $this->transport->send($message, $envelope);

I do agree that if we have our own wrapper or alternative implementations of the Email then the default transport events are a problem.

On the UI: Yes, if we say that users can use SMTP without any contrib module, then we do need a UI. As we also said that core only supports a single transport (configuration) at a time, it could be fairly basic, similar to the current swiftmailer UI where you just pick one of the fixed core-provided options and if SMTP, you get the 5-6 default settings that swiftmailer also exposes (host, ip, encryption, username, password). Anything fancier, you use a contrib module. I'm also still very much open to leaving the UI entirely to contrib (if you are not comfortable setting the mailer DSN directly in settings.php or something). Having an API that standardizes HTML, attachments, headers, ... is still a _vast_ improvement to the status quo.

> Added SendmailCommandValidationTransportFactory
Great thanks. This is looking good to me now if we defer OriginatorSubscriber. Minor comments:

1) Does this also need a change to default.settings.php? Or maybe not as it's still experimental??

2) Should mailer.transports be renamed to mailer.transport?

> But it seems to be that your workaround of a one-off mailer doesn't seem necessary
Agree - when I wrote that code I had a poor understanding of how things worked.

@znerol The patches on this issue have influence from DSM, but the code here is much more beautiful and elegantly crafted into Symfony😃.

+1 from me for overall approach.
I made some detailed comments in the MR.

Given this module is experimental, I think we are not encouraging Contrib module developers to change their code yet. So perhaps we should alter the parts of the IS quoted below?

As a developer I want to start adding message event subscriber for each hook_mail_alter implementation early on so as to be prepared when users start to switch to the new mail system.

in test and in production code

Apologies, my suggestion keeps altering as I consider it more deeply😃. I'm starting to see @Berdir's point I think. I have evaluated various options and I now believe the code in the Mailer service (both Core or Contrib) will likely end up like this:

$dsn = $email->getTransportDsn();
$transport = $transport_manager->fromString($dsn);
// Then either create a Symfony\Component\Mailer\Mailer using this transport, or copy the small amount of code from that class into our own.

This means that with Core alone, it's already possible to set the transport to anything using hooks/API. It also creates the perfect base-point for Core/Contrib to add a transport GUI, even with the option for selecting different transports for different emails.

This works with a TransportManager service with a single function fromString(string $dsn = NULL). If the DSN is null, I propose that the TransportManager call into a DefaultTransport service, which has a single function getDefaultDsn(). This allows for Contrib to override the DefaultTransport service, but without needing to change the TransportManager. (Or it could be protected function getDefaultDsn() on TransportManager.)

Rejected options
Inject into the Drupal Mailer an instance of Symfony\Component\Mailer\Mailer
or of Symfony\Component\Mailer\Transport\TransportInterface. The problem is that both of these force all emails to go through a single instance of TransportInterface. This interface carries the Symfony email class, however we will decorate it - so we have no easy way to get back to the Drupal Email class. There are some awkward ways: put the transport DSN in a header, or create a class that extends the Symfony email, with a variable that points to the Drupal email.

Commenting in the issue again as it's challenging between the overlapping review-threads.

> The purpose of this class is to be used in the container to help create the mailer.transport service. Modules which like to customize the creation of the mailer.transport service are expected to swap out both service definitions (i.e., mailer.transport_factory and mailer.transport). I think that is actually the crucial thing to understand.

I understand that this would be the approach, but I don't agree that this is the correct thing to do. Maybe it is in the Symfony world, I have very little experience there working on anything but super-trivial projects. But it is IMHO not something that will work well in the Drupal world where different modules are are combined and are expected to work together.

You suggest that a module that wants to provide a more flexible and dynamic selection would implement its own factory and its own mailer.transport service, which if I understand you correctly, would then also need to implement its own fake-transport that then actually transports the mail to the chosen transport. But with its own discovery tag, that will the not be compatible with the google transport module, that just provides that transport with the core tag. So a site that wants to sometimes send mails via google API with that module can't do that.

In my opinion, Drupal core should offer a neutral transport collection and factory/selector API. All it does is give you a transport, if registered, for the requested DSN string or object. As suggested, similar to existing patterns that we have in core, that *could* in factory be a FactoryFactory pattern that implements TransportFactoryInterface.

And the decision for the chosen transport would then happen on the layer above that, for example the default core logic could be in a mailer service, and a module that wants to change that could either replace *that* or implement an event and set a header or something, like the Symfony Transport class supports.

I am finding this tricky because we seem to be going round in circles😃.

> I started a sandbox with examples to better show how the API is supposed to be used in contrib / custom code.
As I said (many times!!) IMHO this issue is not creating an API. It is adding things to the DIC. So these examples are not actually code that we'd actually expect or recommend people to write.

#3380041: Create a new email interface that decorates symfony would create an API which should be used in forms. The current mailer_form_example is bypassing the whole Drupal mail system, including it's templates, theming, email policy, hooks/callbacks. This would create significant problems for site builders.

I haven't yet raised an issue yet to discuss events. However I believe we would create new events for two reasons. Firstly we are likely going to decorate the email class, so we'd like events that pass the decorated class. Secondly we will likely have multiple phases in our email building pipeline that aren't possible to access using the symfony events. This means that the mailer_event_subscriber_example has several problems. First it will alter the inner (decorated) Symfony Email object - this will affect the email sent, but it won't alter the Drupal Email object. So any code that correctly uses the Drupal Email object (for example to write a log) wouldn't see the correct cc address. For this reason we might even aim to disable the symfony events. Secondly, this code acts on all mails sent, without any possibility to see the Drupal context such as module/key.

Again it's tricky for me to know what to do.

• Several times I've described code that I believe would work.
• Several times the MR contains code that I believe doesn't meet the requirement, and I have explained the reasons as clearly as I can.
• I've never seen any reply to either part about why it would be wrong, we just keep apparently going round in circles.

> I think that is actually the crucial thing to understand.
You seem to believe that people don't understand your idea. Actually maybe we do understand - the discussion is about if it's the right solution😃.

> I understand that this would be the approach, but I don't agree that this is the correct thing to do.
+1 to pretty much everything that Berdir says. The first question is the requirement. I see two parts:

1. Collecting and creating transports. This is neutral, it contains no preferences, policy or configuration. There should be no need to override it.
2. Transport policy/configuration. Core provides is a simplistic default based on a single configured DSN. Contrib will override or create a completely different service.

The second question is design/implementation details of 1). Berdir suggests a factory factory which is fine in theory but actually I think Symfony doesn't really allow it: parseDsn(), which handles failover/roundrobin is private. The logic could vary between Symfony versions so it seems unwise to duplicate it. Therefore I suggest fromString(string $dsn) which is much easier to implement in the service, and also it's the easier for the service-user as it requires only one step (rather than one step to create the factory and one step use it to create the transport).

Edit: sorry this was a cross-post with the previous comment.

> I have an example in the working
Please can you clarify where is the example?

> Note that the service tag is on the transprot factories (not the transport). There is no limit regarding the number of services which can collect these factories or their ids (see TaggedHandlersPass). I also hope that one fine day Drupal will support !tagged_iterator. Then we can simply drop addTransportFactory() and instead inject the finished iterator as constructor argument.

I completely agree with this statement. However it seems unhelpful to force all other code to duplicate the collection logic. What is the disadvantage of the alternative that Berdir and I suggest? We can implement the collection code in Core as a service that anyone can re-use.

> Regarding the API: It is crucial in my point of view that there is a well known transport service in the container. This is not at least in order to support the messenger component. That one uses an event subscriber (on the messenger side) to dispatch an event from the queue directly to a transport S\C\Mailer\Messenger\MessageHandler. That message handler needs a transport to be injected.

OK great now we are starting a productive discussion of the reasons. I feel the reason that this issue is surprisingly difficult is that the Symfony classes don't really fit what we want in Drupal. What you describe is one possible solution, and I will outline 2 other possibilities below. However before I do that please can I ask my favourite question😃 - do we need to solve that problem as part of this issue?? Once we have a candidate design for the Mailer and Email classes then it would be easier to give the correct answer. I feel that what we need here is to add the transports to the DIC. Do you think it would work if this issue just creates a TransportFactory service? We could leave out all code relating to configuration for now, and perhaps get something useful here that is possible to commit??

Anyway here are my 3 ideas:
1) As @znerol says, define a single well-known transport service that implements Symfony\Component\Mailer\Transport\TransportInterface. As I mentioned in #44, there are some difficulties. We are quite likely to decorate Symfony Email in a Drupal Email. This interface gives no easy way to get back to the Drupal Email. I can see some possible ways around that, but they are a bit ugly. a) We could extend the Symfony email, add a variable that points to a Drupal email, then decorate the extended class. b) We could put the transport DSN inside a header.

2) Ignore the Symfony classes that are too restrictive and don't meet our needs. Instead we create our own versions.

3) Use the Symfony classes, but not as services. For example we can create a Symfony\Component\Mailer\Mailer on a throwaway basis per Email with the correct transport (this is what DSM does).

> I prefer to work very focused on single, concrete steps one at a time.

I completely agree with that. Perhaps the difficulty is that we don't have a clear definition of what the current step is??

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

Yes that works better from my point of view thanks.

1) My main outstanding comment still not addressed.

You have specified service: mailer.transport as the main entry point. The interface is Symfony\Component\Mailer\Transport\TransportInterface which reference the Symfony Email class. This creates a problem for code (already existing in Contrib) that wishes a) to support multiple transports and b) uses a new Drupal email class that decorates the Symfony class. The code needs information from the Drupal class but only has access to the Symfony class.

2) Smaller comment. For "Out of scope" point 1, let's work out how Contrib can fill the gap. Currently DSM has code that calls Transport::getDefaultFactories() which is neat, because it allows the 3rd-party transports to work automatically if present, without errors if absent, and without code specific to any transport . The alternative of defining each in services.yml would perhaps need a module per transport. Once this issue lands, I expect DSM would override the mailer.transport_factory service.

• The service is defined as returning the final class \Symfony\Component\Mailer\Transport. This restricts the options for how to override it. It seems better to have an interface.
• Currently TransportFactoryCollection is final, which seems to be an unnecessary obstacle - Contrib would need to duplicate the code.

> From the point in time a message enters the transport subsystem via the send() method, it must be subclass of RawMessage.
I agree. However the current patch forces all emails to enter via a single "virtual"/"meta" transport. This transport then has to multiplex between the actual different underlying transports, and it has to do so based on the limited information in RawMessage. This seems like a clumsy and unnecessarily restricted approach compared to what DSM does as I will describe below.

> My recommendation is to keep it simple.
Yes but actually IMHO your recommendation is more complex and has more limitations than my suggestion.

> I'd like to keep the discussion focused on the entry point, thus not going to respond to the rest of #79 at this time.
IMHO the fact that you repeatedly ignore comments that you believe are not relevant is making this issue much more difficult. From my point of view, all your following comments and patches don't really make sense because they ignore points that are very much relevant from my perspective.

From my perspective point 2 is relevant to the entry point, because I have a different idea of the correct entry point. I feel that my suggestion makes sense in relation to the issue title - I am proposing a service that gives access to the transports, without any restrictions trying to force the caller to use them in a particular way.

1. The entry point is a service GlobalTransportFactory[Interface] with function fromString(string $dsn = NULL)
2. The implementation collects transport factories and creates an instance of symfony Transport class stored in a protected variable. Also I propose that all created transports are stored in a protected array variable keyed by DSN to allow re-use.
3. In case of NULL DSN, the GlobalTransportFactory calls the ConfiguredTransportInterface service which is like ConfiguredTransportFactoryInterface except it returns a string DSN instead of a transport.

In the @Mail plug-in we can write very simple code:

$dsn = $message['transport_dsn'] ?? NULL;
$transport = $this->transportFactory->fromString($dsn);
$transport->send($email);

In the Mailer library we can write very simple code:

$dsn = $email->getTransportDsn();
$transport = $this->transportFactory->fromString($dsn);
$transport->send($email);

Plus we don't need to create a meta/virtual transport at all. So all round it's very simple.

Both cases can eventually be enhanced to support a bus by creating a throw-away instance of the Symfony library Mailer class with the selected transport (or by copying the code of that class).

How does that sound??

This is based on the successful implementation in Contrib of DSM, and the code already works without needing anything from Core. If Core creates services that would actually make the DSM code more complex then potentially we'd keep the code we already have😃.

> The current service structure and entry point respect the requirements of the Symfony messenger component. That one requires a TransportInterface in the container.

Any chance you could provide a reference to the relevant code please?

Thanks for the links.

I suggest that we "ignore" the question of messenger in this issue. We would need to solve it as part of #3380041: Create a new email interface that decorates symfony, and there are likely a range of options. We can adapt Drupal Mailer to fit Symfony, but less ideal for Drupal. Or we can make Drupal Mailer ideal for Drupal and then some adaption is needed for Symfony Messenger (MessageHandler is apparently a very simple class, so maybe there are options to ignore/replace it??). It seems difficult to know until we start to try the different options, creating the classes, services etc.

In any case the "entry point" for the Mailer service will presumably be something like Mailer::mail(). The mailer.transport service seems like a mostly internal thing that would only be used by the Mailer and Contrib modules that extend it, and only for the purpose of fitting with Symfony.

This issue (as I've said many times before😃) is apparently about adding transports to the DIC. Everything I've described in #81 seems to fit that idea very well. If we want to add a mailer.transport service later then there's nothing preventing that. The proposal of #81 would already be a significant step forwards
- We can adjust existing Contrib modules to use the new services.
- We can create a Contrib module that overrides the new services, providing a UI to edit transports (with full support for failover, 3rd-party, etc.options, etc.), taking code from DSM.
- We can use this same module with Core, DSM and DSM-L.

How does that sound???

It's tough to keep up here. So far, it's mostly just been @znerol, @AdamPS and me being active here, with @znerol and @AdamPS having pretty different expectations of what this should do and how it should work. I'm being torn between them, seeing both advantages. I think we need some more feedback from the community at this point, specifically also framework maintainers.

@znerol did a great amount of research on how the transports API is expected to be used. That's not how I would design it, but it's also clear to me that there are advantages, for example around interoperability with other components like the messenger. This is nicely outlined in the issue summary.

I think one aspect that's not covered yet in the issue summary where there's still a disagreement is whether or not we should trigger the built-in events. Like @AdamPS, I have some concerns as well, due to things like translations, BC with the old mail API, context (theme, language, ...) and so on and given that it's still not clear whether or not and how we'll wrap Email objects to cover our use cases.

But thanks to the hybrid core+experimental module approach that I quite like, I'm OK with moving forward with this approach and see if we can build the systems that we need or decide we need in core on top of this. I'm not quite sure *how* we approach that given that this is now in an experimental module that we can't yet depend on, but we can figure that out in follow-up issues.

Adding a variety of tidbits/opinions from discussions with @znerol on Slack from the past few weeks.

I'd like to see the transports class working: id `mailer.transport` class: Symfony\Component\Mailer\Transport\Transport.

Core should support creating transports from multiple DSN's into transports.

Mailer requires the concept of a default transport. By convention, this is the _first_ defined transport. Core could do something else, of course. I'd try to not invent new concepts whatever we do.

The default transport is autowired to Symfony\Component\Mailer\Transport\TransportInterface. I think its reasonable for core to always fall back to this transport.

Contrib/custom code should always have the option to send to an alternate transport. Either set in the intial Email object or added as email headers via X-Transport.

There should be some kind of factory in the container that produces Mailer objects, such that its arguments are autowired properly. The method I used for contrib Symfony mailer was a non shared service.

Actually I don't recommend it for core, its just contrib was already contructing anew each time.

Thanks @znerol for putting so much work and energy into this issue. Unfortunately I have much less spare time and energy at this moment, and I find it tough even to keep up with all the developments here.

> The approach outlined in #81 assumes that transports are stateless.
Actually I don't think so. In my second numbered point (sorry it was brief and perhaps hard to understand) I suggested re-using transports for emails that specify the same transport DSN. This allows transports to have state and is good idea for performance (it cuts the cost of repeatedly creating the transport). It's true that currently DSM creates a transport per email however we can easily fix that.

NB if we autowire based on the Symfony Transports class (rather than using an Interface), we apparently prevent reuse because the fromString function creates a new transport each time.

In summary, it seems that

1. symfony presents a clear idea for how the mailer and transports should work
2. it does so in a way that makes it difficult for Drupal to be different: there are lots of final classes, private functions/variable and classes without interfaces
3. even where Symfony does create an interface, there are very good reasons why Drupal might want a different interface

Basically I agree, that it is pretty easy to introduce the Symfony interfaces and classes into Drupal by copying the Symfony Framework Bundle. I can see that this has some advantages, however you end up with something very different from DSM Contrib module. In Contrib, we have deeply integrated many Drupal mechanisms into the Mailer. To do that, we found it necessary to ignore many of the Symfony interfaces.

I believe that the most recent PR is likely to be fundamentally incompatible with DSM Contrib. It would potentially lead in a direction when Contrib used hooks to delete all the Core services. It could make migration from Contrib to Core difficult.

> with @znerol and @AdamPS having pretty different expectations of what this should do and how it should work.

Not necessarily😃. I am open to adopting some of the ideas of @znerol, it's just that I'm not convinced that it should be part of this issue. My point is that as soon as we put in place an entry point that it has huge consequences on the entire mail system design. The wrong interface can make it very difficult to integrate some Drupal key concepts.

IMHO the right approach is to create the entry point as part of #3380041: Create a new email interface that decorates symfony. As part of deciding that interface we should have at minimum

• A working prototype implementation
• A feature comparison with Contrib DSM including a design that shows it would be possible for Contrib could add back the missing features (or if not, clear approval from the framework manager that these features don't matter)
• A working prototype migration path from Contrib DSM for the 23k sites currently using it

I feel that one potential problem at the moment is that the design for Core Mailer is perhaps being done without a deep understanding of what Contrib Mailer can do, the design chosen and the reasons why. I tried quite a lot of different things, and hit various problems, gradually leading to the current design. The discussion here includes some suggestions that I already tried, and found problems with.

I feel that Symfony is a toolbox. We can choose which tools to use, and don't have to use them all.

Clearly we do want to use the transports, and the email class as that's the whole purpose of this META issue. We don't have to expose them however - they could just be an implementation detail.

In terms of the rest, in Contrib DSM we didn't use much else

• We didn't expose any Symfony interfaces or the classes because they weren't quite what we wanted. It was easier to create new interfaces.
• We didn't use the Symfony events. Firstly they used the email class which we didn't use, secondly we had a different pipeline so needed new events (actually we used hooks, but it doesn't make much difference)
• We didn't use the Mailer service which anyway is just a few lines of code (we create one internally, but that's only because I didn't realise you can send directly to the transport)
• We didn't use the Messenger service. Currently we don't have an equivalent, maybe we should use it. But it seems to rely on other parts that we don't want - maybe we can adapt or decorate it??

Thanks for a clear explanation. I'm giving my time here for free in the hope that it helps the Drupal community. I'm starting to think that you perceive that in fact I'm an obstacle, in which case I will immediately stop wasting my time and avoid commenting further.

It is important to understand that this MR targets the mail delivery part of the upcoming mailer implementation.

Yes you said this several times before and please trust that I do understand😃.

However AFAICS my (frequently repeated😃) response to that still has not been answered. In summary: creating a mail delivery part with that specific interface significantly restricts options for the remaining parts of the system. Off the top of my head, the two biggest points are:

1) It assumes that we would stick with the symfony Email class, optionally extending it. AFAICS it's not a good interface if we wanted to decorate the Email class (with a protected inner variable containing the symfony class.). In DSM Contrib we decorated because some parts of the Symfony class were restrictive. This was discussed but unfortunately I can't remember where, perhaps in the META issue with @catch and @Berdir seeing some benefit in decorating. However that discussion didn't have the full context of this issue, and I can see that there are good reasons for extending. However it would potentially be wise to investigate the potential downsides before making a firm decision.

2) In the existing MailManager there is a switch of render context, and the actual mail building happens in the callback to avoid polluting the render context. In DSM Contrib we also have an interface based on a callback, extending the idea to switch also language (matching Commerce), theme (matching mailsystem) and user (matching simplenews). AFAICS, the symfony interfaces don't really support the callback approach because the caller builds an Email class and then sends it.

AFAIC the current patch is leading to a future that looks like this. I regret that I was not better able to predict the direction that Core would choose😃.
A] Core provides basic function (leaving Contrib to provide advanced cases as usual)
B] Existing DSM Contrib provides advanced cases, but by removing the Core implementation which is fundamentally incompatible. This gradually becomes deprecated and then unsupported.
C] New DSM Contrib (which BTW I'm not volunteering to maintain😃) provides advanced cases on top of Core - potentially excluding some features in B that are not really possible with the restrictions that Core has introduced into interfaces. This module could also potentially include migration for sites currently using B.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily 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.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily 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.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily 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.

Imagine this will need a change record or a few.

I popped into to see how this issue is progressing. I'm definitely not going to get sucked back into an argument. I'll just point out a few things and leave you to decide whether to pay any attention to them.

I find the scope of this issue quite confusing. The title is "Add symfony mailer transports to Dependency Injection Container" which suggests the lower parts of the email stack that shouldn't be used directly. In that case, it's basically fine.

• Forcing all emails through a single instance of TransportInterface is a bit awkward however it can work. It will require a de-multiplexing transport that checks for a special header that specifies the transport DSN. (Symfony already has one, but it assumes a fixed named set of transports and it doesn't allow dynamic setting of DSN e.g. in a hook.)
• I have some comments that would help reduce duplication in contrib. If they aren't accepted then contrib can simply ignore the core transport factory service and create a new one.

Then later on I read "As a developer I want my service to reference a mailer service in order to submit an e-mail message". That suggests that this issue creates a service and API (exposed directly from symfony) intended for use in contrib code that sends mail. In that case, I have some serious concerns as the interface seems totally unsuitable. AFAICS we would then lose some key things compared with the existing MailManagerInterface:

• module+key to help hooks identify mails (we could combine them into a single string)
• encoding key information into 'params' and then building the email from these (allows hooks to alter the params, or code can replace the building code entirely)
• building the email in a callback function to switch render context so avoiding pollution (and we can add switching of language and user)

Also we need to consider migration from the old mail system to the new. I expect that we will need a period of time when both old and new mails systems are supported. I feel that sites likely want to choose when to switch mail system independent of developers changing which API they use. This can be achieved by means of mapping code. However if modules are encouraged to write directly to the symfony services then it's less clear if that could work.

I propose that:

1. We should mark the mailer.mailer and mailer.transport services are @internal, and likely rename the first one.
2. Then in a later issue, core can create a new mailer service that contrib would actually use. This service sits above what's currently called mailer.mailer. Most of the bullets currently in Problem/Motivation would need to be moved to this later issue.
3. Finally contib code can create a further enhanced mailer service that sits above the core one. Currently contrib Drupal Symfony Mailer has many extra features that are popular with sites:

• deep integration into Drupal mechanisms, including theme/template/render, CSS libraries, configuration, plug-ins.
• generic "mailer policy UI" that exposes GUI access to many parts of the Email interface.

Specifically, the "later issue" is #3380041: Create a new email interface that decorates symfony, and it gives abundant reasons that hopefully make it clear that we need a wrapper service above the symfony mailer one.

I've thought some more about multiple transports. I agree that core won't support the required transport configuration, however I do feel that we should offer the API to allow contrib to do so. Hence there should be a function on the email interface to set a transport DSN (which comes in the later issue), and this issue should have the required transport code to support it. I believe the required code is in fact simple, and if we don't do it then the core transport factory/config services would be mostly unused - because almost everyone would install the config module so that they can configure their default transport and it would replace the core services.

So in summary here is my suggestion:

1) Change the issue title to "Create the symfony mailer delivery layer". It has become much more powerful than just integrating transports.

2) Update the IS to remove all the existing "As a developer" bullets which instead go in the later issue. There are 2 things we can do:

• Developers of modules that already have written similar code to this issue (likely only DSM and DSM-L) can switch to use the core code.
• Developers can create a module with a UI to specify mailer transports.

3) Rename the mailer.mailer service to mailer.delivery and mark it as non-public.

4) Mark the mailer.transport service as non-public. Implement it with a new class named DemuxTransport, FanOutTransport or something similar which contains the following simple code.

• Check for a special header X-Transport-Dsn and then remove it, else use empty string (meaning use default transport).
• Call the transport manager to create a matching transport and store it for reuse in case of a future request with an identical DSN.
• Send the email using the new or existing transport.

5) Rename ConfiguredTransportFactoryInterface to TransportFactoryInterface with interface as follows.

• function createTransport(string $dsn = '');
• function setDefaultTransport(string $dsn);

6) Please do call Transport::getDefaultFactories() in the TransportFactoryAdapter. I hear and understand your IS "Note: Symfony mailer provides many transports...". However this simple addition will not cause any of the problems you describe as it simply makes use of something that Symfony already offers - and any issues raised relating to the supported list can be pointed upstream. This change gives automatic support for 14 3rd-party transports (in the version I happen to be using today) and that list will automatically increase. Having done that we can remove the 3 transport factory services, keeping only sendmail that we alter.

If we do those things then IMHO it would create a really useful transport delivery layer for both core and 2*contrib. I would then happily give my vote for RTBC.

Thanks for the reply, and for all your hard work on this issue.

1)

I gather, that you are very concerned about the lower level part (i.e., the mail delivery) to be exposed in the container

Well I am responding to what you have written😃.

• The IS states that developers can write code in 7 specific ways. For me this implies that code would use the API from this issue directly.
• The example code seems to use the API from this issue directly.
• The example code seems to have the problems that I describe in #114.

If you replace all the above with a statement that this is a lower-level interface that would not normally be used directly then I agree that the problem would be solved.

2) I'm not sure you answered my point about whether mailer.mailer is definitely the right name for the delivery layer. Do you have a proposal for name of the layer above that is the actual mailer that people should use?

3)

But for the beginning, I prefer to follow closely what symfony mailer offers out of the box.

Well my doubts are mostly about TransportFactoryAdapter and ConfiguredTransportFactoryInterface which I guess aren't from symfony mailer, but our own adaption?? I feel that if we instead adapt in a slightly different way, then the result could be much more use to contrib. However it doesn't matter that much as presumably I could just override the mailer.transport service to a different class and ignore the other two services??

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily 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.

This comment is about policy and communication, which I feel are key for this complex issue.

1) Policy

I feel that there is a key policy decision here: is it OK for module code to chose to write directly to the delivery layer if it prefers??

The current mail API requires a particular structure with parameters and a callback, which gives key benefits for customisation and avoiding certain bugs. The current rendering code for generating HTML pages has key patterns including MarkupInterface, render array, themes, and access control. In both cases, we seem to have a happy consensus the developers don't try to bypass them.

The contrib DSM code effectively integrates these two on top using a new mailer the sites on top of the delivery layer. It provides various benefits including security. (If someone offers to fund me $500 for each security bug relating to email I find in a current stable contrib module I reckon I'd find quite a few😃.) If modules write directly to the delivery API then DSM can't really work for these modules - e.g. already the HTML is translated and rendered. So I am in favour that we also build a consensus here not to bypass this layer.

I found this comment in #116 very helpful and it could usefully be added to the IS:

=======

There are two activities when it comes to send transactional mail from Drupal:

1. Building mails
2. Delivering mails

This issue is about the second activity. It is clear that the mail building part will need another round of exploration and experimentation to get to something simple and useful. But it is out of scope for this issue.

=========

To say that something is experimental means that it's the current preferred option however that might change. That's quite different from something that is contrary to what we actually want.

I feel that the current scope statement bullets 1,2,3,7 are actually part of building mails (#7 altering the building process). Therefore I would propose to put them as out of scope, as this issue doesn't yet provide the correct Drupal API for these cases. People could use it like that - but it's effectively the same as if they already use Symfony Mailer directly without needing this issue. Neither case is the Drupal preferred API. I agree you have proved that the delivery layer works for these cases and that's great.

On the other hand, bullets 4,5,6 are the proposed correct Drupal API as far as we know - but of course it's experimental.

====

2) Sandbox

It's great that you provided the Sandbox, and you link to it in the IS:

There is a Sandbox with examples demonstrating how custom / contrib is supposed to use this API.

Developers are likely to start copying parts of your code, so let's try to make this code as helpful as we can. Specifically I would be grateful if you agreed to add @todo markers in each place where module code uses the delivery service, and it should later be changed to use the building service.

I don't so much agree with the sentiment from #121

The example code validates the approach taken here. It isn't supposed to be the reference for the resulting product.

Secondly, my technical comments.

1) I'm not sure you answered point 6 from #115. Yes you have emphasised this is out of scope, however I feel I am raising a valid question about the reasoning given for that decision. I feel that Symfony has provided a useful solution for a genuine requirement. I don't understand your reluctance to use it.

Please do call Transport::getDefaultFactories() in the TransportFactoryAdapter. I hear and understand your IS "Note: Symfony mailer provides many transports...". However this simple addition will not cause any of the problems you describe as it simply makes use of something that Symfony already offers - and any issues raised relating to the supported list can be pointed upstream. This change gives automatic support for 14 3rd-party transports (in the version I happen to be using today) and that list will automatically increase. Having done that we can remove the 3 transport factory services, keeping only sendmail that we alter.

2)

There is some precedent in core for services without a service name.

Yes good idea. Perhaps we could even do it for all the services which Symfony seems to do already. The interface serves very well as the service name, and I don't see any reason for another identifier. Even for classes that don't auto-wire, $container->get(MyInterface::class) works well.

3)

See the comment on the GitLab MR. In summary, contrib and custom code wishing to customize the way transport(s) are configured is not supposed to reuse TransportFactoryAdapter. Instead such modules are supposed to supply their own implementation of ConfiguredTransportFactoryInterface.

I continued the discussion in the MR. Personally I prefer a simpler approach. I would be happy to see any changes here, but it's fine if not. The code in this MR doesn't cause me a problem as I am free to ignore it.

Great thanks @znerol those changes really helped.

I have created a new 2.x version of DSM contrib module that uses the service structure from this issue. So the current code is definitely usable. However Symfony mailer offers many different integration options, and I believe we could make some better choices for Drupal.

1) Transport.php has the code we need, however I would prefer to decorate it rather than expose it directly. We can create a class named something like UnifiedTransportFactory with a corresponding interface. The advantages are: a clearer name (it's a transport factory, not a transport); an interface file with comments; simplification of the service structure by removing one service; flexibility to override this service (Transport is a final class so it cannot be overridden).

2) Symfony mailer allows for the implementation of Symfony\Component\Mailer\Transport\TransportInterface to be either a single transport or a multi-transport. I feel that for Drupal we should always use a multi-transport - even if there is only one configured transport. Advantages: we only have one setup to test rather than two; it makes Core ready for multiple transports (similar to how MailManager was ready for multiple mail plugins) even though we don't provide a way to configure it; it simplifies the service structure removing one service and with a clearly named class that indicates the purpose; we can allow the X-Transport header to contain a DSN, so hook code can modify the transport.

I feel it would be great if some of the other 32 people following this issue could offer their views.

Removed the @logger.channel.mail argument in the abstract transport service definition in order to avoid...

Thank you, that was causing many weird errors for me too. Maybe it would help to put a comment in the service file to explain??

Regarding Transport::getDefaultFactories() , I don't see it as weird if the transport services automatically appear when the corresponding transport factory class is installed. This seems like exactly the desired behaviour.

I have two questions please.

1) It should be possible to automatically create a service for each of the default factories. Would this be any more attractive to you?

2) If we don't do this, can you describe what you would do instead? A contrib module could create all the service definitions in a services.yml file, but then they would exist even if the corresponding transport wasn't available. We could make one module for each transport, but that seems pretty inefficient.

I'm a bit late to the party but went looking for an issue like this after a recently being reminded of the poor DX of the current API.

I've tried to go through all 138 comments and it looks to me like they have all been addressed.

The new API in this issue feels like the right level of abstraction to me, and a core API plus experimental module seems like the right approach.

Looks like there is thorough test coverage too, and the IS is up to date.

We still need to get a framework manager to review, but otherwise I would RTBC it.

Thanks @znerol.

>> The Symfony mailer Transport facade is now gone.

Good, I'm glad that we are no longer using a 3rd-party final static class as an interface. The new code is simpler, nice work.

It does mean that this issue provides even less function as a support for Contrib - the "combined transport factory" with a fromString() function has now gone. This isn't necessarily a negative as a simple Core can be the right decision. DSM Contrib contains many features that sites already use and that this issue now specifically excludes. Therefore I believe I shall have to ignore almost all the code in this issue and create a separate implementation. This isn't necessarily anything we need to consider a problem as it is just a different implementation of the same interfaces. I am hopeful that we can still find some agreement in #3380041: Create a new email interface that decorates symfony.

>> Core should just stick with one transport,

For clarification I am in agreement with this consensus that Core only support one transport (in terms of configuration). My suggestion was that we could nevertheless provide a transport layer implementation that allows multiple transports to exist. This would for example allow a hook to override the configured transport and instead use the NULL transport. I am aware that likely you still disagree, however I feel it's useful to clarify that my suggestion isn't necessarily against the agreed consensus.

I have two specific comments on the updated code.

1) There is now a function TransportServiceFactoryTrait::fromDsnObject(). However I believe the DSN object is only capable of representing a "simple" transport without high-availability or load balancing. For this reason, in my code I have used the fromString() function instead. Perhaps this trait should do the same?

2) The comment for TransportServiceFactoryInterface says this:

Contrib and custom code may choose to replace or decorate the transport service factory in order to provide a mailer transport instance which requires more complex setup.

In DSM contrib, I propose to override the definition for Symfony\Component\Mailer\Transport\TransportInterface directly as the implementation does not need a factory. Do you agree that this as a reasonable customisation option? If so, please could you alter the comment to include it as one of the options?

1) Re #142

Hence, core does not have any need for a reimplementation of the fromString() (and consequently the parseDSN()) method. Its maintenance cost should be left with Symfony developers.

Well reimplementation would be avoided with my suggestion to decorate😃. And if we are only implementing what Core needs then it's potentially not really worth creating a trait hoping that Contrib can reuse😃. Anyway, sure whatever you want, these details are not so important.

2) I added the work-in-progress Contrib code as a second API example to the IS. This code uses the interfaces/services from here. As already discussed, it hardly uses the classes in the current MR because of their limitations (which have been chosen consciously to simplify Core). I have raised #3500885: Make symfony mail delivery layer more useful to contrib with my suggestions to change that, linked in the IS as a possible follow on task. The code in this issue is experimental and we can change it later so we don't need to delay this issue.

===

I agree with #139: let's make this RTBC. The community has now made a detailed review, and znerol has made his answers clear. I have been an active reviewer, so I hope that I am justified in setting RTBC. The next step is the framework manager review.

Is there anybody who would be willing to help out with the Contrib code that uses the interface/services from this issue? Even just 1 hour to discuss the design would be useful.

I raised #3501138: Add support for 3rd-party symfony mailer transports as another follow-on issue.

I added more info to #3500885: Make symfony mail delivery layer more useful to contrib. Initial investigation suggests that it would not increase code complexity or size. There are clear precedents in Core, including the current/old Mail System. See the IS for details.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily 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.

We're going to need a change record here here that we should keep updated as each follow-up issue lands. Going to leave at RTBC to get committer and framework manager reviews

Forgot to say - great issue summary - could be used to form the basis of the CR.

Left a couple of questions on the MR. Some may warrant comments in the code
Removing the tag - the main concern from an FM POV is the number of transports added that may be redundant.

@znerol please add to CR about discovery of the factories as it will be a point of confusion when more then one transport will be declared

Asked for a couple of comments in the code related to the questions I had - thanks

Looking at the process for adding a new experimental module do we have an existing roadmap for the path to stable for the module? i.e. items #8 and #9 on that list? I think we need those before this can go in.

I've also asked release managers if this is eligible for addition during beta

about #9 - it's the next in a parent meta (step 2) #3380041: Create a new email interface that decorates symfony - which is not yet started

but for #8 it needs follow-up to discus factory approach but for 11.2 I bet it enough to start with single transport

Discussed this with @larowlan and @quietone. This MR adds a new Mailer module that is alpha stability. As per the experimental modules guidelines, alpha stability modules may be removed from releases, and in this case the module is not really useful on its own at the present time. Therefore, instead of committing this to 11.x now for 11.2 (only to have to remove it again), we agreed that the best course of action is to commit this to 11.x shortly after 11.2.x has been branched, with the aim to meet beta stability requirements over the next 6 months and include this as experimental in 11.3.0.

Thank you @znerol for your patience and continuing work in this space.

Adding issue credit.

Committed and pushed e298fb7bb23 to 11.x. Thanks!

Therefore, instead of committing this to 11.x now for 11.2 (only to have to remove it again), we agreed that the best course of action is to commit this to 11.x shortly after 11.2.x has been branched, with the aim to meet beta stability requirements over the next 6 months and include this as experimental in 11.3.0.

For completeness sake can someone please clarify if this went into 11.2 or if it's delayed until 11.3?

For anyone who missed it (like I did :facepalm:), in summary:

• Not added to 11.2.
• Added as experimental to 11.3.x with the intention of making it beta / stable prior to 11.3.0's release.

Also, given the importance of this new functionality it might be worth tagging for the release notes / highlights. Thank you.

Yep let's tag it for 11.3.0 release highlights.

Added Mailer to Experimental modules and themes in Drupal core

wow great, that's super important for Drupal. Thanks to everyone involved! :-)

As this is experimental it won't actually ship with 11.3, so removing the highlights tag.