Introduce email yaml plugin and an email building pattern known from controllers

I had a first go to understand, and found it not so easy😃. I feel that for a module to send an email should not be so complicated. It seems much simpler in DSM+. Could you help me compare them? Either we could make this simpler, or list clear reasons why it is more complex.

1) These lines seem to happen a lot. I feel it should be just one line to send a mail. The underlying layer can handle the separate build/render/send.

try {
        $email = $this->emailRenderer->render($definition, ['items' => $items], $langcode);
        $this->mailer->send($email->to($recipient));
        $sent++;
      }
      catch (TransportExceptionInterface | PluginException $e) {
        Error::logException($this->logger, $e);
    }

2) I'm confused by inconsistencies between the modules, they seem far from following a pattern. Some have a builder some not. I feel that one class is enough and it can have separate functions for the parts. All modules can have the same 2 functions.

1) These lines seem to happen a lot. I feel it should be just one line to send a mail. The underlying layer can handle the separate build/render/send.

True. The call site needs more attention at some point. I'd like to keep the question open for a while to see what kind of abstraction best fits these cases. It very much depends also on whether or not we want to support passing a closure to $emailManager->getEmailDefinition().

2) I'm confused by inconsistencies between the modules, they seem far from following a pattern. Some have a builder some not. I feel that one class is enough and it can have separate functions for the parts. All modules can have the same 2 functions.

I tried to present many different approaches in 3539651-core-emails. E.g., the confusingly named ActionMailBuilder class is an action plugin and a mail builder at the same time (demoing the closure approach). The UpdateMailBuilder shows that it is still possible to create text-only emails (question is whether we want to support that or not). UserNotificationHandler shows the closure approach with first class callable syntax, ContactMailHandler shows how it could look like if the "controller" is set via yml file.

If we want to continue with this approach for a bit longer, we should probably decide which pattern is to become the standard one.

I general, I like the idea of declaring emails with a plugin like this. The YAML syntax is very straightforward and consistent with other plugins, and for more dynamic use cases, developers can use a deriver class. Having the emails formally declared opens up use cases that are difficult to deal with now. In Easy Email, I had to make my own plugin for emails and then declare the core emails myself in order to make them overridable. I think discoverability will be nice for other use cases like ECA, as well.

A couple of specific changes I propose we consider for the plugin definition.

1. Option for template instead of controller: I like the _controller option. It makes rendering an email very similar to rendering a route. But I think many emails could probably be rendered using only a template that would use the provided params as variables. In such cases, the controller is a boilerplate creation of a render array specifying the template. I propose that we allow developers to skip that boilerplate by adding a key to the plugin specification to specify a template instead of a controller. So the developer can choose one or the other. And if they choose neither, we can default to a template that is derived from the email's machine name. The implementation for this could be that we always call a controller, but there's a default controller that we write that renders a template from the email specification if no other controller is specified.

2. Typed data for parameters: In Easy Email, my specification includes data types for the parameters. This allows me to match up the parameters with fields in the email entities, such as entity references for users or Commerce orders. I don't think we should require developers to provide the types for their parameters, but it would be nice to support it if we are able to do so.

3. Consider whether we really should support closures: It's a neat idea, but I'm struggling to figure out the use case where it really helps. Yes, it can be nice to have the definition of the rendering function in the same place as the call site, but it seems like if we encourage this type of behavior, we make it harder for people to override the rendering of emails. In the case where we make everyone who declares an email provide a controller or use a default one that renders a template, that specification will be alterable by any developer who wants to provide their own controller instead. How would that work with a closure? The closure is being created at the same time as the call to send the email is being made, so it seems like it you won't be able to alter it ahead of time. You might be able to catch it before it's rendered, but that's an email by email alter rather than just altering the email specification for all emails of this type. So, I think we should think this one through a little more. Personally, I think a cleaner specification with clearer options for overriding is more important, but I'm open to changing my mind if someone can provide some concrete use cases where the closure is superior.

Regarding the rest of the code in the MR and related commits:

I feel that for a module to send an email should not be so complicated.

I agree with this sentiment. I think sending an email should be roughly as simple as it is now, which only requires calling a single method with module, key, params, etc. The way it's currently implemented here requires the developer to know more about how the email system works than they probably should have to know. I think if we don't support closures, this should be possible.

Next, I think that your code demonstrates the need for the Drupal adaptation layer that has been a controversial topic in our previous discussions. Here are a few key snippets:

<?php
$params += $definition->getDefaults();
    $context = [
      'id' => $definition->id(),
      'module' => $definition->getModule(),
      'key' => $definition->getKey(),
      'langcode' => $langcode,
    ];
    $this->moduleHandler->alter('email_params', $params, $context);
?>

<?php
$context = [
          'id' => $definition->id(),
          'module' => $definition->getModule(),
          'key' => $definition->getKey(),
          'langcode' => $langcode,
          'email' => $email,
        ] + ($build['#props'] ?? []);
        unset($build['#props']);
        $this->moduleHandler->alter('email_build', $build, $context);
?>

<?php
$context['metadata'] = BubbleableMetadata::createFromRenderArray($build);
        unset($context['email']);
        $this->moduleHandler->alter('email_post_render', $email, $context);
?>

These are alter hooks where you're passing around multiple objects. The specification for what is in the params and context changes a bit per call, but it's clear that passing around the params alone or the Symfony email object alone is not enough. If I'm developer, I have to understand which different types of objects I'm going to get in each of these alters, and I have to know what is expected to be in context at each point, as it changes. The Drupal adaptation layer would provide a single object that we could pass around that would encapsulate the context, params, and potentially the final Symfony email. It would simplify the API that developers have to interact with at each point.

Moreover, I think we should consider using the adaptation layer object in place of the initial params array. For example, most email call site code in the current API looks something like this:

<?php
$params = [
  'user' => $some_account,
  'order' => $commerce_order,
  // ...
];
$this->mailManager->mail('commerce_order', 'order_receipt', $to, $langcode, $params, $actually_send);
?>

The call site code in your commits is a bit different, but much of the internal code is passing around these same variables. With the adaptation layer, I think we could get to something like this:

<?php
$email = $this->mailManager->new('commerce_order.order_receipt')
  ->setTo($to)
  ->addParam('user', $some_account)
  ->addParam('order', $commerce_order)
  ->setLangcode($langcode)
  ->setSend($actually_send);
$this->mailManager->mail($email);
?>

I think the latter is easier to read and more discoverable, as the context variables will have specific methods that can be autocompleted in IDEs, found by AI code agents, etc. And now we have one object we can pass around internally, though alter hooks or event subscribers, until we finally get to the point of actually sending the email, where we convert it into a Symfony Email. (We potentially could also have a Symfony Email embedded it that is being updated by the parent object all the time so there's no conversion process at the end. I don't have a strong opinion about that.)

Those are my initial thoughts on the MR and other commits. I think it's a good start on layers 5 and 6, as currently defined in the proposed resolution in 🌱 Requirements and strategy for Core Mailer Active , but I think it would benefit from us discussing layers 3 and 4 in more detail, which is where my comments above are focused. But overall, I'm feeling optimistic. After seeing this work and talking to @adamps last week, I feel like I'm starting to see the outlines of a workable solution. I'm looking forward to discussing this in more detail at our next meeting.

Thanks for the review.

Since this has come up multiple times now, I guess I'm going to iterate on the call site now. There are a couple of well known design patterns which might help in this situation. In my eyes, the current MailManagerInterface is a facade. It provides a convenience method (mail()) which abstracts away the whole complexity of mail plugins, formatting and logging.

Looking through core, I can find another well known design pattern which simplify access to complex subsystems. E.g., EntityStorageInterface::getQuery() returns an instance of a class implementing the builder pattern. The call site of entity queries resembles the example snippets by @zengenuity and @adamps. I think the builder pattern could be useful. E.g., a call site in the contact mail handler could look like this:

    $key_prefix = $message->isPersonal() ? 'user' : 'page';
    try {
      $email = $this->emailManager->getEmailBuilder()
        ->id('contact.' . $key_prefix . '_mail')
        ->param('message', $message)
        ->langcode($recipientLangcode)
        ->build() # returns a Symfony Email
      $this->mailer->send($email->to(...$recipients)->replyTo($message->getSenderMail()));
    }
    catch [...]

In cases where a message needs to be sent to multiple recipients with different languages, the builder can be reused (update module):

    $builder = $this->emailManager->getEmailBuilder()
      ->id('update.status_notify')
      ->params($items);

    foreach ($recipients as $recipient) {
      [...]
      try {
        $email = $builder->langcode($langcode)->build();
        $this->mailer->send($email->to($recipient));
        $sent++;
      }
      catch [...]

In a second step, addressing, sending and exception logging could be moved into a convenience method. It doesn't need to cover all cases (build() is still there) but it could simplify the most used ones:

Contact example:

    $key_prefix = $message->isPersonal() ? 'user' : 'page';
    $result = $this->emailManager->getEmailBuilder()
      ->id('contact.' . $key_prefix . '_mail')
      ->param('message', $message)
      ->langcode($recipientLangcode)
      ->send(to: $recipients, replyTo: $message->getSenderMail(), logger: $this->logger);

Update example:

    $builder = $this->emailManager->getEmailBuilder()
      ->id('update.status_notify')
      ->params($items);

    foreach ($recipients as $recipient) {
      [...]
      if ($builder->langcode($langcode)->send(to: $recipient)) {
        $sent++;
      }

Noted the other comments in #6. All of them are useful as well, thanks!

Looking at this code example:

<?php
$email = $this->emailManager->getEmailBuilder()
         ->id('contact.' . $key_prefix . '_mail')
         ->param('message', $message)
         ->langcode($recipientLangcode)
         ->build() # returns a Symfony Email

$this->mailer->send($email->to(...$recipients)->replyTo($message->getSenderMail()));
?>

This code suggests that mail ID, parameters, and langcode will not be available at the point the email is being sent. This will complicate the process of anyone attempting to alter the sending of the email, such as rerouting or logging it.

This code suggests that mail ID, parameters, and langcode will not be available at the point the email is being sent.

Good point. I think those should be added as tags / metadata to the Symfony Email object. This information is then also available on external mail service providers for filtering and routing.

Pushed the EmailBuilder approach.