Allow custom TransportFactory

I tried to add a new TransportFactory and Transport class to handle a new DSN like foo://

This function is entirely with the Symfony Mailer library so I suggest you ask there.

I don't really get this one yet😃.

1. - $transport_dsn = $email->getTransportDsn();
That can't be right surely? We need to use the provided DSN.

2 SymfonyMailerServiceProvider.php duplicates a lot of code with a long list of service providers. We don't want to maintain that code in this module.

I can't commit this unless it can be done without breaking things.

1. $email->getTransportDsn() ; must be used.

2.

it is normally generated by composing when symfony/mailer is installed on a Symfony project.

Yes but this is not a symfony project it's a Drupal module. It seems very strange that symfony mailer library expects some other code to register services for it. Who says this is the right thing to do? Why doesn't it do so itself?

There is a list of factories in Symfony\Component\Mailer\Transport::getDefaultFactories. We shouldn't duplicate that.

Thanks for the explanation.

1. It's a good suggestion to make a service for Symfony\Component\Mailer\Mailer. However currently this class only accepts a transport at create time - there is no way to change this later. This module allows each email to have a different transport. Therefore this isn't going to work, and we need to stick to creating a instance of the Mailer object for each message sent.

You could raise a feature request for the library to add a way to specify a transport DSN per send request. This could be a new function or an extra parameter on the existing method.

The Symfony\Component\Mailer\Transport\Transports has the same problem - it requires an array of all possible transports at create time. This module cannot provide such a list, so we can't use this class as a service.

However Symfony\Component\Mailer\Transport doesn't have this problem. It can be created with a list of factories. So this class could work as a service, and we can call the fromDsnObject() function. However this is messy, it requires registration of the service in code to add the list of factories as an argument.

2. I stick to my belief that the details of all the transports don't belong in this module. Transports will get added, so it's chasing a moving target. Instead we need to call Transport::getDefaultFactories().

----

So putting everything together, I suggest the answer is like this.

1) Add code to the constructor of Mailer
- Copy the code currently in SymfonyMailerCompilerPass with comment "Retrieve all service transport factory tagged with mailer.transport_factory".
- Join this array with the result of calling Transport::getDefaultFactories().
- Create a Transport object using this combined array and store it in a protected class variable.

2) Instead of calling Transport::fromDsn() call $this->transport->fromDsnObject().

No other changes needed.

There are no new services because each of the library 3 classes that could be a service are unsuitable, as they require information at create time that isn't easily available. This could change if symfony library code changes.

Thanks, that looks good.

I think it would be nice to keep the same registration system as describe in the Symfony Mailer documentation:

I agree.

I have some detailed comments. Please set the patch to needs review, check the tests pass and fix all coding standards.

1.

+  /**
+   * @var \Symfony\Component\Mailer\Transport
+   */
+  protected $transport;
+
+  /**
+   * @var \Drupal\symfony_mailer\TransportFactoryManager
+   */
+  protected $transportFactoryManager;
+

Comments need a description of the variable in addition to the @var.

2.

protected function getTransport() {

Do we actually need this function? Why not set $this->transport = in the constructor?

3. Let's put the call to Transport::getDefaultFactories() into SymfonyMailerCompilerPass. Instead of $transports = []; do $transports = Transport::getDefaultFactories(). Then the $factories constructor parameter contains all the factories rather than just some of them, and we only call getDefaultFactories() once.

4.

$container->register('symfony_mailer.transport_factory_manager', TransportFactoryManager::class);

Can we put this into symfony_mailer.services.yml as it's the more common place for people to look? Then we can have a comment to explain that an argument will be added.

5.

+class TransportFactoryManager {

Please implement an interface.

6.

+ private $factories;

We always use protected please. Also this needs a comment with a var statement.

7.

+   * @param TransportFactoryInterface[] $factories
+   */
+  public function __construct(iterable $factories)

Inconsistent types - I suggest TransportFactoryInterface[] is better, it is a stricter type. Also needs a description of the @param.

8.

+  /**
+   * Return all existing factories.
+   *
+   * @return \Generator
+   */

I prefer @return TransportFactoryInterface[], also the needs a description beneath the @return.

Sorry I made a mistake in #12.

Symfony\Component\Mailer\Transport doesn't have this problem. It can be created with a list of factories. So this class could work as a service, and we can call the fromDsnObject() function. However this is messy, it requires registration of the service in code to add the list of factories as an argument.

The "messy" code (SymfonyMailerCompilerPass) is necessary anyway to call $container->findTaggedServiceIds(). So I propose we should create a service for Transport instead of creating TransportFactoryManager. Apologies for the extra work.

Great thanks. I have updated the MR with two changes:

1. For performance, Mailer should create the Transport object once during the constructor, not once per mail sent.
2. I discovered that in Drupal the service_collector tag corresponds to tagged_iterator in Symfony. Use this instead of creating a compiler pass.

Please can you review and check it works in your system?

Great thanks