Add a way to capture mails sent through the mailer transport service during tests

Wasn't sure how to test this one.

I installed the new test modules and clicked "Send mail"
Then I checked the db directly and see the mailer_capture entry in key_value table.

Code wise LGTM

This looks good.

It would be interesting to check if this still works in the case that a Contrib module has overridden the service for TransportInterface. It seems like it would, which is good news.

Which categories of testing does this work for (i.e. Unit, Kernel, Functional)? All 3 categories are likely to be useful.

There is a similar issue #3395814: Trait for testing Symfony Emails, and good news is that the proposed solutions are similar. The other issue goes an extra step, proposing a full collection of assert functions for efficiently checking emails, for example: assertBodyContains, assertSubject, assertTo. The functions operate on the email at the front of the queue, and the function readMail() removes an email from the queue and then testing begins on the next one. This reduces the amount of test code substantially compared with each module having to navigate the email array directly. So the other issue can remain as a follow-up - or if Core doesn't want it, then it can remain in Contrib. See MailerTestTrait in DSM+.

I tried to integrate this patch into DSM+. I found that MailerCaptureTrait::getMails() currently returns SentMessage rather than Email as indicated by the comment. I fixed it as below, removing the reference to AbstractTransport.

class CaptureTransport implements TransportInterface {

  /**
   * {@inheritdoc}
   */
  public function send(RawMessage $message, ?Envelope $envelope = null): ?SentMessage {
    $capturedMails = \Drupal::keyValue('mailer_capture')->get('mails', []);
    $capturedMails[] = $message;
    \Drupal::keyValue('mailer_capture')->set('mails', $capturedMails);
    return NULL;
  }

In the tests for this patch we should add some asserts on fields on the email, which would prevent errors like this from being introduced.

EDIT: fixed return value

Follow up from #9.

• It works if a Contrib module has overridden the service for TransportInterface.
• It works for Kernel and Functional tests

I like it - it has some clear improvements over the current DSM+. I have made an issue to change DSM+ to match this #3533041: Alter mailer testing to match Core proposal. I'll wait until this is committed.

Also DSM+ has some clear improvements over this. We can use #3395814: Trait for testing Symfony Emails if we wish to add them to Core.

I made some detailed comments in the MR.

Below is a comparison of the code we could have in MailerCaptureTest before and after the trait added in #3395814: Trait for testing Symfony Emails. The character count reduces from 559 to 264 = 53% reduction, and it is more readable IMO.

The trait itself is 300 lines, so not a big maintenance burden. Given that, I hope we might add it to core. If so, then it would potentially make sense to adjust this issue to have module mailer_test containing MailerTestTrait.

    // Before
    $capturedEmails = $this->getMails();
    $this->assertCount(0, $capturedEmails, 'The captured emails queue is empty.');

    $mailer->send($email);

    $capturedEmails = $this->getMails();
    $this->assertCount(1, $capturedEmails, 'One email was captured.');
    $this->assertEquals('admin@example.com', $capturedEmails[0]->getFrom());
    $this->assertEquals('State machine energy a production like service.', $capturedEmails[0]->getSubject());
    $this->assertStringContainsString('environmental along agree let', $capturedEmails[0]->getHtmlBody());

    // After
    $this->assertNoMail();

    $mailer->send($email);

    $this->readMail();
    $this->assertFrom('admin@example.com');
    $this->assertSubject('State machine energy a production like service.');
    $this->assertBodyContains('environmental along agree let');

Thanks that looks good. How do you feel about #13?

Before this gets committed I feel we should discuss #13 please.

In preparation for #3395814: Trait for testing Symfony Emails it would likely make sense to adjust this issue to have module mailer_test containing MailerTestTrait. Otherwise the follow on issue creates extra work for us and the core committers by renaming everything.

The module name is currently mailer_capture. This is good for the current use case.

In #3395814: Trait for testing Symfony Emails I propose to add some extra functions that I believe will be very useful, see also #13 explaining the benefits. These functions would naturally live in the same module, probably even the same trait. Therefore I propose that we change the module name now to mailer_test, and the trait to MailerTestTrait. The module/trait are responsible for assisting testing of emails, which includes both capturing the email and asserting the values. Developers will almost always want both parts I believe, so we don't need a module for each.

How do you feel about that??

OK that makes sense.

Is it OK with you if I add #3395814: Trait for testing Symfony Emails into the roadmap??

I'm not convinced that providing a test API in a test module is the right call here and replacing the transport completely, so essentially I challenge #3. We already customize the mailer_dsn in test setup and expect this to work. I think it will be confusing for example if you want to have your own collector for something or test your own transport (and maybe mock it's internal API or something) and then that won't work. See comments.

I think it's strange that we then use a trait from a test module as an API. Because this is an API.

Is there any harm in just switching to collecting by default?

+1

There are two main classes in this issue: the capture transport, and the API for accessing captured emails. We can put these in the natural location as @Berdir suggests - not inside a test module or anything.

Then we just need a mechanism for enabling the capture transport. It's much less important where this is located because the exact mechanism is internal. It could be a test module, however I feel there is likely a better option. My impression is that the experimental mail plugin is mostly just a proof of concept / placeholder, and maybe we could even delete it when the new mailer lands. Even if we don't, then could we keep testing of the experimental module separate (a minority case) from normal testing, e.g. with a separate base class hence provide a separate dsn?

Thanks for the updates (not yet checked). Personally I prefer the existing name of AssertMailTrait or similar. It would then be natural to add extra functions with specific asserts in #3395814: Trait for testing Symfony Emails.

Looks good to me, thanks @znerol. I will leave in NR until @berdir checks.

You can ignore my previous remark as we can change the name as part of #3395814: Trait for testing Symfony Emails (I suggest MailerTestTrait).

I see the problem with the config for the old API vs. the new API and the whole thing being an opt-in through the experimental module.

Looking through the existing mailer test, we have existing test coverage about not sending mails in \Drupal\Tests\mailer\Functional\TransportServiceFactoryTest, and also a kernel test and both are essentially based on the system.mail:mailer_dsn config, so setting that *does* work. But if I understood correctly, the problem is that setting that explicitly to a "capture" scheme instead of null would not work for \Drupal\Core\Mail\Plugin\Mail\SymfonyMailer. We could work around that with some hardcoded logic that sets capture back to null and say we don't support that there. Or we actually do add support for that in there ourself. It is just another transport.

That said, I can also live with the module that you have to enable for now, we can re-evaluate that when we make the new mailer API non-experimental and switch existing mail sending and their tests over.

What we do need is a change record that describes how to do that I think, even if this is experimental. Needs work for that.

Setting this back to RTBC. Leaving it to a committer to decide on whether or not we want to have this opt-in switch with a test module or a different approach, see #27 and previous discussions.

Our code is experimental, so we are allowed to change things after commit. Even so I feel we should do our best to get things right first time to minimise confusion/disruption for those evaluating/testing the experimental module. Is there are reason why we can't do that here? We are implementing the test API which will become widely used in module test code.

1) The mailer capture transport should always be enabled when running tests using the experimental mailer (same as in the old mail system). We don't need to ask developers to enable a test module to make it work.

I hear the discussion about possible technical difficulties, however surely there must be a way as we can change any code that we like. There are some suggestions in #27

We could work around that with some hardcoded logic that sets capture back to null and say we don't support that there. Or we actually do add support for that in there ourself. It is just another transport.

2) Change the trait name now so we don't have to rename it in #3395814: Trait for testing Symfony Emails. Our goal is one trait that covers both capturing and asserting - tests will always need both and they don't need to use two separate traits. (Perhaps a unit test only wants asserting, if we ever write one - fine it can just ignore the capturing part.) I propose MailerTestTrait, which matches DSM+ and is similar to the name AssertMailTrait in the old mailsystem. If you prefer a different name that's OK by me if it covers both parts: capture and assert.

Then in the IS "proposed resolution" we can remove the reference to the mailer_capture module and instead add a reference to the trait.

How do you feel about that??

The published change record https://www.drupal.org/node/3519253 refers to mailer_capture which is not available yet. That's very confusing.

The MR for this issue was identified as having a new Kernel/Functional test class that did not have the #[RunTestsInSeparateProcesses] attribute. A deprecation warning is now issued in the case of these omissions. I've rebased the MR added the attribute to prevent this from being committed as-is and accidentally breaking tests on HEAD. Because this is a minor change to test metadata and the tests are passing I am leaving the issue's status as RTBC.

Symfony\Component\Mailer\SentMessage is not quite friendly for asserting HTML emails. In functional tests you will likely fetch the original message. I wonder if it is better to capture the original message instead?

Here is the implementation I am using for acceptance tests (DTT).

final readonly class SentMessageSubscriber implements EventSubscriberInterface {

  /**
   * {@selfdoc}
   */
  public function __construct(private Outbox $outbox) {}

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents(): array {
    return [SentMessageEvent::class => 'onMessageSent'];
  }

  /**
   * {@selfdoc}
   */
  public function onMessageSent(SentMessageEvent $event): void {
    $original_message = $event->getMessage()->getOriginalMessage();
    if (!$original_message instanceof Email) {
      throw new \LogicException(
        \sprintf('Test outbox requires %s emails, but received %s.', Email::class, \get_debug_type($original_message)),
      );
    }
    $this->outbox->add($original_message);
  }

}

The outbox service also relies on keyValue storage and provide a few helper methods.

/**
 * {@selfdoc}
 *
 * @implements \IteratorAggregate<non-negative-int,
 *   \Symfony\Component\Mime\Email>
 */
final readonly class Outbox implements \IteratorAggregate, \Countable {

  /**
   * {@selfdoc}
   */
  private const string COLLECTION_KEY = 'mailer_outbox';
  private const string DATA_KEY = 'outbox';

  /**
   * {@selfdoc}
   */
  public function __construct(
    #[Autowire(service: 'keyvalue')]
    private KeyValueFactoryInterface $keyValueFactory,
  ) {}

  /**
   * {@selfdoc}
   *
   * @return \Symfony\Component\Mime\Email[]
   */
  public function getAll(): array {
    // @phpstan-ignore-next-line return.type
    return $this->getStorage()->get(self::DATA_KEY, []);
  }

  /**
   * {@inheritdoc}
   */
  public function getIterator(): \Traversable {
    return new \ArrayIterator($this->getAll());
  }

  /**
   * {@selfdoc}
   */
  public function getFirst(): Email {
    $all_emails = $this->getAll();
    return $all_emails[0] ?? throw new \LogicException('There are no emails available');
  }

  /**
   * {@selfdoc}
   */
  public function getLast(): Email {
    $all_emails = $this->getAll();
    return $all_emails[\count($this) - 1] ?? throw new \LogicException('There are no emails available');
  }

  /**
   * {@selfdoc}
   */
  public function add(Email $email): void {
    $this->getStorage()->set(self::DATA_KEY, [...$this->getAll(), $email]);
  }

  /**
   * {@selfdoc}
   */
  public function clear(): void {
    $this->getStorage()->delete(self::DATA_KEY);
  }

  /**
   * @return non-negative-int
   */
  public function count(): int {
    return \count($this->getAll());
  }

  /**
   * {@selfdoc}
   */
  private function getStorage(): KeyValueStoreInterface {
    return $this->keyValueFactory->get(self::COLLECTION_KEY);
  }

}

Thanks for continuing to work on this. Eligible only for 11.x as the experimental mailer has been removed from 11.3.x.

Committed and pushed 4b2b8604a46 to 11.x. Thanks!