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

Pushed code removed in 📌 [PP-1] Add symfony mailer transports to DIC Postponed . I think this could be much less complex if we'd just replace the mailer.transport service directly with the capturing mail transport. Then the mailer_dsn config wouldn't have any effect at all and we wouldn't need to override the override it in 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 ✨ Framework for testing Symfony Emails Active , 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);
  }

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.

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 📌 Alter mailer testing to match Core proposal Active . I'll wait until this is committed.

Also DSM+ has some clear improvements over this. We can use ✨ Framework for testing Symfony Emails Active 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 ✨ Framework for testing Symfony Emails Active . 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 for the review. Regarding #11, we should stick with AbstractTransport, otherwise transport events will not fire. I've added methods for both cases (getEmails() in order to assertions on the original Email and getCapturedMessages() for assertions on SentMessage).

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

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

In preparation for ✨ Framework for testing Symfony Emails Active 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.

Sorry, I do not get it. What needs to be renamed and when?

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

In ✨ Framework for testing Symfony Emails Active 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??

I think the additional assertions could be useful in unit tests too. For that reason it would be better to keep mail capturing and test assertions separate. The former only works in kernel and browser tests. The latter will work everywhere.

OK that makes sense.

Is it OK with you if I add ✨ Framework for testing Symfony Emails Active 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. in with a separate base class hence provide a separate dsn?

Moved MailerCaptureTrait to core, properly inject the keyvalue service into CaptureTransport.

Regarding how the transport is enabled, see the comment in the MR. Also keep in mind that the CaptureTransport requires (supports testing of) the experimental mailer module. That alone should be reason enough to not enable it by default.

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 ✨ Framework for testing Symfony Emails Active .

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 ✨ Framework for testing Symfony Emails Active (I suggest MailerTestTrait).

Update and rebase for 🐛 The keyvalue service cannot be autowired Active .

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.

Added a section to the existing CR [#3519253].

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.

Slightly extended the issue summary.

There was an issue link under Remaining tasks which doesn't belong there (a follow up doesn't need to be resolved before this issue can be committed). The follow-up ✨ Framework for testing Symfony Emails Active is linked from the related issue list.