🇬🇧United Kingdom @adamps

This is the last project blocking my D11 upgrade😃. I hope we might get a new release soon please as many sites don't want to put a dev release onto a live site. I understand, folks are busy, but maybe it only takes 5 minutes.

It's a good theme, but eventually folks might drift away...

Thanks to @znerol #51 I realised that DSM+ doesn't need to use plugins for Mailer/MailHandler services. I still propose to use attributes on the service class for the metadata, which can be discovered by means of a service tag. I raised 📌 Mailer classes don't need to be plugins Active . I agree that Yaml files could also work to define metadata.

Commerce created an improved mail system and AFAICS it has the following benefits: it allows a mail to be defined in a single file; it implements language switching; it uses twig templates to define the email (as does simplenews). Easy Email provides a UI for configuring email body. DSM+ supports all of the mentioned features and emails use twig either from config or a template.

We can follow these successful examples. We can combine the MailHandler and MailBuilder into a single class. The building part by default lives in a function build(EmailInterface $email), or the class can define multiple build functions. It is similar to a Form, which can have multiple submit functions. We no longer need the _controller in the Yaml file because the MailHandler is self-contained and specifies its own build function. Code that wishes to replace the implementation can override the entire MailHandler service. This avoids many of the problems of #53 which come from replacing half of the implementation. Code that wishes to alter the email registers their own build() function which receives the identical information to the original build. The build() function would normally not set an HTML body render array (although it could using setBody()) because mostly it seems better to define the HTML using twig config/template rather than as a series of calls to t() in code.

I admit, my proposal resembles DSM+😃. However it is proven to work.

adamps → created an issue.

We have requirements [D2 CAN_REPLACE] and [D3 CAN_ALTER]. I would be interested to understand how they are met by 📌 Introduce email yaml plugin and an email building pattern known from controllers Active .

1. I would like to override ContactMailBuilder and trust that its public methods won't change (except in ways that are BC).
2. I would like to be able to access or change any aspect of the email, including Drupal specific concepts not present on the Symfony class. I might have previously altered or added to the email parameters or wish to do so in this code. So I would like the build functions to have just one parameter, which is the Drupal EmailInterface and then I can get everything from that, including parameters, and key. (The following comments give more detail about specific parameters.)
3. I would like to use the current langcode (I had the opportunity to participate in language negotiation, which was followed by environment switching) rather than having a $langcode parameter.
4. I would like to be free to build the email however I choose, which doesn't necessarily use a filter format, so don't wish to receive a $format parameter. I understand that I could ignore it, however I feel that the presence of the parameter on the interface gives the calling code a false expectation that it will have an effect.
5. I would like to generate my own body and subject however I choose, including generating it from a Twig string in config or using a Twig template. I don't wish to replicate the code that sets up the variables instead I would like to access them via the Drupal EmailInterface, and trust that the set of available variables won't change (except in ways that are BC). I would also like metadata that describes the available variables which I can show in the UI.
6. I would like to override User mails too and I would like a consistent pattern compared with other modules, in other words to find a mail builder. I will assume for now that the UserNotificationHandler->build() could be moved to a mail builder.
7. I would like to build the mail how I choose. I don't wish to receive parameters $confirm_url and $token as they seem to correspond to an implementation detail. I don't wish to give the calling code the idea that it can choose these values (AFAICS doing that would break even with the Core implementation). I would like to choose which emails contain the confirmation URL.
8. I would like to customise the replyTo address, including the possibility to have none. Therefore I would like the code that sets replyTo to be in the mail builer which I can then override rather than in the calling code. Similarly for the address/langcode of the admin email, the recipients of an update email and the decision whether a particular user mail is enabled. I would like to create a unified UI that can configure any value on any email, including the decision to enable/disable it.

#51 is very useful thanks. I expect that if someone wrote a similar response to #51, and "Drupal adaptation layer" it would also be useful.

@johnny5th Good point. We can check if the route exists, or just catch the RouteNotFoundException.

The routing.yml file is flexible and it can map to many "things" - not only to a controller but also a form, an entity list, and entity form, an entity view, and maybe more. In the case of emails, I feel that we only have "thing" and so we don't need the flexibility. There is another pattern which is more common/familiar and simpler: plug-in plus attribute. We could make an @EmailBuilder plug-in and put the metadata (currently in the yaml file) on the attribute.

The current controller design only covers the email body. To build an email we also need to cover the subject, recipient and perhaps more. These must also be set within the switched environment because they are language dependent, and can affect render context. I feel we should let go of the idea of returning a render array (even though it does give a pleasing similarity with the routing file) and instead the controller should act on the Drupal EmailInterface object, typically in the build phase, but could also be init or post-render. This gives the entire implementation in a single class which is easily swappable.

Great, we now have several sources of ideas: 📌 Introduce email yaml plugin and an email building pattern known from controllers Active ; DSM+; Easy Mailer (although AFAICS it mostly acts at higher layers than what we are concerned with here); comments on this issue and on others.

I feel that the key to moving forward is to take some decisions as a group, specifically related to the requirements and design. I added a new section to the IS "Key decisions and resulting issues" with 3 draft proposals. They are related, and could alternatively be seen as one proposal, to create layer 3, the "Drupal adaptation layer".

In my view this is the most important part of the new mail system. It's the absolute minimum that we add on top of Symfony to get a useful working Drupal integration. It exposes an API still quite close to the original Symfony without a load of "baggage".

I fixed the tests - I added some asserts that fail before the fix is applied. The fix is now ready for commit.

Unfortunately the tests are failing on the 4.x branch, see 📌 Fix tests Active , so we need to wait for that to be fixed.

There is one easy fix: add static to public function simplenewsNewsletterNameTokenDataProvider() in 2 places.

The other failures relate to the simplenews_issue field being missing, which is strange. It should come from /config/optional/field.field.node.simplenews_issue.simplenews_issue.yml.

adamps → created an issue.

Regarding $langcode, my comment is purely about where we put the code, and I have no intention to change the behaviour.

On MailManagerInterface, the call site code is forced to set $to and $langcode, and optionally can set $reply. All other email data is set in hook_mail(). This causes two problems:

1. Hook code cannot easily alter the language - it would need to duplicate the logic in user_mail()
2. There is a split in the code for building the email, and anyone reading the code has to look in two places.

In DSM+ the email builder plug-in builds the entire email. It uses the init() function to set any values that affect context switching and the negotiated language is still available. It uses the build() function for everything else.

I believe my proposal helps to fix issues like 🐛 Password reset invalid mail notify language Needs work . We can centralise the automatic choice of language for sending to a user inside the mailer service, based on extending setTo() to accept a user object. We can even generalise this further by introducing a EmailableInterface which classes such as User and Subscriber can implement (it has functions to get the email, display name, language and user context). This avoids requiring every module that sends email to a user to duplicate the same code (or to use different code and give inconsistent results). So we could decide that the correct code is something like this: IF "the detection method is based on URL" AND ("current user matches the target user" OR "current user is anonymous") then we pick the current language, else we would pick the user language.

I feel that in most cases the email building should not be context dependent. For example, if the admin resends the commerce order receipt, then it should be the same as the original. If a test newsletter is sent then it should be identical to the actual email that will eventually be sent in cron. If a module triggers generating of a user mail (currently by calling _user_mail_notify()) then most likely they wish to use the same language as would normally be used.

Please see #7, especially

There is no point in having a debate as we can't change it. Any change would alter the emails of all the existing sites in ways that could be undesirable.

And should relatively mean any existing HTML emails remain in-tact.

Not true. Your suggestion would add paragraph or break tags wherever this is a line break. Whitespace is not supposed to have meaning in HTML, and this change would break the layout of any email that happens to contain a line break.

The processed_text rendering that occurs is already doing _filter_autop but it also strips all HTML and recreates links (it falls back to the fallback format that is usually plain text). So for MarkupInterface would be bad as it will strip HTML (though arguably does any Legacy email support it?).

Yes but the processed text rendering only occurs for plain text emails (not instance of MarkupInterface), which is the correct behaviour.

I propose to add #3542264

Makes sense to me

The contrib module is currently not compatible with D11 and also has only 34 sites that report using it.

When the mailer ceases to be experimental, do we still need a Core module? If so, then I feel we should keep the name "mailer". If this is only a temporary module, then one option is to give it a name that indicates the temporary status.

Thanks. It's a fairly unusual case because symfony_mailer_update_20001() enabled the mailer_policy module on all sites. However it's a valid bug. Presumably the same problem exists with symfony_mailer_update_20004().

The solution should be easy - just add

if (\Drupal::service('module_handler')->moduleExists('mailer_policy'))

The MR appears to have no commits.

Is this still an issue?

Thanks for checking. I no longer use this module, so please feel free to close the issue.

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 ✨ Framework for testing Symfony Emails Active . 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 yaml plugin is a really interesting idea that I would like to discuss and I believe it could work well. I find it difficult to discuss in the context of 📌 Introduce email yaml plugin and an email building pattern known from controllers Active because that contains a lot of details also about 10 or 20 other ideas, some of which I find good, some bad. As I said before, I propose we will make progress by taking decisions one-by-one.

I feel that the Mailer service from ✨ Create a new mailer service to wrap symfony mailer Active is a good match with the yaml plugin. The Mailer interface could be written as

function send($id, $params)

And the call-site code then becomes attractively simple

class UserNotificationHandler {
  public function sendRegisterAdminCreated(UserInterface $user): bool {
    return $this->mailer->send('user.register_admin_created', ['user' => $user]);
  }
}

All the work is now in the "controller" part, the mail builder, or whatever we call it. It's easier to understand in one place, and it's easier to swap the implementation (compared with the old mail system, where part of the code is in _user_mail_notify(), part is in user_mail() and neither can be swapped!).

We could even add parameters to the yaml file

    parameters:
      user:
        type: entity:user

although perhaps even better we could automatically deduce the type because the name is an entity type provided by the same module.

AFAICS 📌 Introduce email yaml plugin and an email building pattern known from controllers Active covers layers 3 to 5.

If we implement layer 3 (Drupal Adaptation) it would likely take in EmailRenderer. It would simplify the rest of the code quite a bit. We could remove all reference to $langcode because the language would already be switched. We could integrate concepts like token replacement into the underlying service.

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.

@zengenuity and I had a useful discussion of the design. We tended to agree at the high level, then had different terminology and ideas at the detailed level. I adjusted the IS proposed resolution slightly, including to reverse the order of presentation (sorry!) because this will match the order we work in - continuing our journey up the layers, each one dependent on the ones below. The next layer is ...

3) The "Drupal adaptation layer". We already have detailed issues with a list of points for discussion in the proposed resolution. Following @berdir suggestion I feel we should combine them into one (which it was originally!). ✨ Create new mailer service based on symfony Active , ✨ Create a new mailer service to wrap symfony mailer Active , ✨ Events/callbacks for new mailer service Active .

4) Then I propose that the final layer for Beta would be the building layer.

We can leave the others....

5) In the previous meeting and in a previous prototype I recall that we had the idea that for the Beta we could stick with the existing config layer. So Core generates plain text mails, and Contrib can override for HTML. I have kept that idea as a proposal in the IS.

6) @znerol already prototyped how we can avoid changing the user layer for beta, by overriding the old mail system services. I added that to the IS, it seems clearly good.

Before we move on to work on the individual layers, please can people comment on how they feel about the overall picture of the layers, and what we would do in our first beta version??

It's not really part of the work on the new mail system at all, it fits more into the work to deprecate and move all functions in .module files to services/classes.

Thanks that is a helpful way to describe it.

This is ready from my side.

OK yes it makes sense. I am convinced, and I believe the others on the thread were already convinced a while ago.

I added some new requirements based on recent comments and some of my own ideas, all marked DRAFT. I propose that one existing requirement be deleted, marked DELETE.

Please comment to say whether you agree, or alternatively we could do it at our next meeting.

I think we cannot deprecate _user_mail_notify as a whole without providing proper APIs to cover most of these use cases first. That said, I think the best way forward would be to deprecate custom ops here in a first step.

I feel that we have an easy solution. In this issue we could create NotificationHandler with exactly the same interface as _user_mail_notify(). The only difference is that it is a service with an API rather than an internal function. The purpose of the change is to prepare for mail system migration. It becomes just the same as contact module and others.

Then later in the new mail system we switch to the new preferred interface of one function per op, we change the return code, and anything else that we want. This follows the basic principle that we avoid change to the old mail system except in rare cases that we really have to.

Great thanks @znerol.

In the new mail system, Contrib would like to be able to trigger generation of emails.

I realise that the same applies to the old mail system. That's why we have lots of calls to _user_mail_notify() even though it's technically internal.

We are deprecating _user_mail_notify(). What do we expect people to use instead? The only answer I can see is NotificationHandler. Even if we mark it internal then developers will call it as they have no choice. So I suggest that it should not be @internal and should have an interface. This would match the other existing mail handlers.

When the new mail system becomes stable then we would deprecate NotificationHandler. The developers will have to change their code again, but I don't see an easy alternative.

EDIT NB My comment was a cross-post with the previous one.

Thanks for the clarified IS. I now understand much better what we are doing here, sorry for the prior confusion. I have made two clarifying statements below. Provided that you agree with these, then +1 from me.

The new service is marked @internal and intentionally has no interface. It is discouraged to use the service outside of core. The only purpose is to simplify transition to a new mail system.

In the new mail system, Contrib would like to be able to trigger generation of emails. I believe Easy Email might include a UI for this feature generally. Commerce has it specifically as "resend receipt".

_user_mail_notify has responsibilities which shouldn't be substituted. E.g., it has code which checks whether a notification is enabled and it logs an error if an account has no email configured. A contrib/custom module would need to replicate and maintain these responsibilities over time. This isn't desirable.

In the new mail system, Contrib would like to be able to substitute any part of the email code, and we have requirements relating to this. I wrote in more detail about this as a comment on the MR.

Without experimentation its impossible to know what works and what breaks, though.

We have one 4 year experiment including over 40k sites that is known to work😃.

In 📌 Introduce email yaml plugin and an email building pattern known from controllers Active :

The rendering itself happens inside an EmailRenderer service which has the following responsibilities:

That sounds rather like ✨ Create a new mailer service to wrap symfony mailer Active :

The Email building pipeline proceeds in the following sequence:

Also I see you are introducing template suggestions, email plugin manager, some metadata on the plugin definition, and various other things quite similar to DSM+. You have some really interesting new ideas that I like. So that is quite promising. On the other hand, I can see many ways in which DSM+ is fairly obviously better, and the MR code would fail to meet needs that have been clearly expressed by multiple developers in Contrib issues.

Many of my comments remain unchanged from before. I find these huge MR too big to process in any sensible way. There seems to be a danger in "reinventing the wheel". We should also consider simplifying the transition process for the existing sites, so try to avoid replacing an interface with something mostly equivalent, when perhaps we could leave it the same and it be equally good.

I feel that likely you experiment with these big prototypes to clarify your own thinking, and to explore radical new options. Which is fine, but eventually we need to find our way to agreement on issues we could commit.

Here are some steps that I would see as useful:

• Start discussion on the "Drupal adaption layer" to create a Drupal Mailer service and EmailInterface. These map very clearly to many of the agreed requirements, and I feel are essential. It would significantly simplify all the MR you create, because currently you have a lot of "boiler plate" code that could be handled by this layer.
• Work on details in specific issues with a smaller scope. This would help me even if @berdir is right and we have to bring it back together to get a commit. Obviously this requires some agreement on a design, which I am keen to discuss.
• Provide a clear explanation for the choices made, especially when it is re-inventing very similar proven Contrib code. If you could reference a DSM+ class/concept/etc and specify 3 improvements that you would like to make then I can likely easily agree. I would then point out 5 things you apparently made worse, you might agree in 4 cases, it was just an accident, we discuss the 5th case, and very quickly we are done.
• Reuse or reference existing issues and ideas from other people rather than duplication.

OK great. I'm not the right person to review the complexity of the implementation, so let's try and get another reviewer for that.

This does seem to meet the needs of the mailer project. I have put in an example in the IS that shows it fits in the context of ✨ Create a new mailer service to wrap symfony mailer Active . Of course this is based on just one possible idea of solving that issue and the details would change, but the principle seems good anyway. Equally it would fit in MailManager (not that I propose changing that now!) or DSM+.

My interpretation of this part about the correct line-endings is about ensuring that the email is valid. It is saying that the calling code doesn't have to worry about the details in the RFCs regarding the rules for line-endings. For an HTML email, then the body is MIME-encoded. Still, Drupal (or in fact Symfony Mailer Library) will put line endings according to the rules, but they won't have any effect on the message.

I agree that this isn't the only way to interpret it. Anyway the comment in hook_mail was likely written before HTML email was even invented. There is no point in having a debate as we can't change it. Any change would alter the emails of all the existing sites in ways that could be undesirable. I feel happy with the current version which matches behaviour elsewhere in Drupal e.g. hook_help().

The only solution I can see is for you to add in a space or line break in your code.

PS If you Mail System, then you still need to use something else as a plugin to send the emails. Mail System just allows you to configure which thing to use.

We have an amazing opportunity to rewrite the entire Drupal mail ecosystem without constraints from the past. I can see so many possibilities😃. We could automatically generate the entire Email Configuration UI from simple metadata on the mailer service/plugin classes.
We could make the email building code extremely simple and clear by including a few common patterns into the Core framework (and avoid bugs where people forget those things).

We can make the migration process simple for both the site owner and developer with a carefully designed migration framework. Given that we don't have much idea yet what the migration process will be, how do we know that this issue is going to be helpful (please see #4)?

DSM+ is an example of many such things. Sure maybe we won't to do any of it in Core, but it would be great if we could at least take time to have a look at them and decide. This issue ends up constraining things that we might later want to change. We are in danger of eliminating possibilities without most people even knowing what those possibilities are.

Below is the user email code from DSM+. It replaces _user_mail_notify(), _user_mail() and most of AccountSettingsForm. See all the code that's just gone away:

• language is automatically switched based on the 'to' address (which accepts AccountInterface)
• token replacement is automatic from the token_types metadata (we only have to set the special token options)
• subject/body config is stored so it can be automatically applied (the config entity id specifies the applicable mail tag, the fields specify the different parts of the email)
• same for the 'to' address config for the admin message, defaulting to the site address
• the config UI can be automatically built (including appropriate token help) based on the required_config metadata, we just need a single line in AccountSettingsForm to trigger its placement

All we have left is this below, the simple essentials. Of course it doesn't need to be exactly like that - there are many possible variations and improvements that follow the same ideas.

declare(strict_types=1);

namespace Drupal\symfony_mailer\Plugin\Mailer;

use Drupal\Core\StringTranslation\TranslatableMarkup;
use Drupal\symfony_mailer\Attribute\Mailer;
use Drupal\symfony_mailer\EmailInterface;
use Drupal\symfony_mailer\Processor\MailerPluginBase;
use Drupal\user\UserInterface;

/**
 * Defines the Mailer plug-in for user module.
 *
 * Replaces _user_mail_notify().
 */
#[Mailer(
  id: "user",
  sub_defs: [
    "cancel_confirm" => new TranslatableMarkup("Account cancellation confirmation"),
    "password_reset" => new TranslatableMarkup("Password recovery"),
    "register_admin_created" => new TranslatableMarkup("Account created by administrator"),
    "register_no_approval_required" => new TranslatableMarkup("Registration confirmation (No approval required)"),
    "register_pending_approval" => new TranslatableMarkup("Registration confirmation (Pending approval)"),
    "register_pending_approval_admin" => new TranslatableMarkup("Admin (user awaiting approval)"),
    "status_activated" => new TranslatableMarkup("Account activation"),
    "status_blocked" => new TranslatableMarkup("Account blocked"),
    "status_canceled" => new TranslatableMarkup("Account cancelled"),
  ],
  required_config: ["email_subject", "email_body"],
  token_types: ["user"],
)]
class UserMailer extends MailerPluginBase implements UserMailerInterface {

  /**
   * {@inheritdoc}
   */
  public function notify(string $op, UserInterface $user): bool {
    if ($op == 'register_pending_approval') {
      $this->newEmail("{$op}_admin")->setEntityParam($user)->send();
    }

    return $this->newEmail($op)
      ->setEntityParam($user)
      ->setTo($user)
      ->send();
  }

  /**
   * {@inheritdoc}
   */
  public function init(EmailInterface $email): void {
    $email->setParam('token_options', ['callback' => 'user_mail_tokens', 'clear' => TRUE]);
  }

}

Thanks.

I found that using splide-wrapper class without a nav already breaks the base splide library. I didn't fully understand it, but I guess it might be from splide.load.js line 143 if (!$.hasClass(p, ID + '-wrapper')) {. Maybe that code is specifically looking for splide-wrapper so won't like splide-wrapper--nav. Anyway I'm sure you will understand it much better than I do.

Here's the template I ended up with. I skip the splide-wrapper class, but keep the others. I also added a class based on the optionset which is handy.

{#
/**
 * @file
 * Default theme implementation for a splide wrapper.
 *
 * Available variables:
 * - items: A list of items containing main and thumbnail of splide.html.twig
 *   which can be re-position using option Thumbnail position.
 * - attributes: HTML attributes to be applied to the list.
 * - settings: An array containing the given settings.
 *
 * @ingroup themeable
 */
#}
{%
  set classes = [
    settings.skin ? 'splide-wrapper--' ~ settings.skin|clean_class,
    settings.skin_nav ? 'splide-wrapper--' ~ settings.skin_nav|clean_class,
    settings.vertical ? 'is-wrapper-vertical',
    settings.vertical_nav ? 'is-wrapper-vertical--nav',
    'splide-wrapper--' ~ settings.optionset|clean_class,
    blazies.is.nav ? 'is-wrapper-nav',
    settings.navpos ? 'is-wrapper-nav--' ~ settings.navpos|clean_class,
    'over' in settings.navpos ? 'is-wrapper-nav--overlay',
    'over' in settings.navpos ? 'is-wrapper-nav--' ~ settings.navpos|replace({ 'over-' : '' })
  ]
%}
{%- if blazies.is.nav -%}
  {% set classes = classes|merge(['splide-wrapper']) %}
{%- endif -%}

<div{{ attributes.addClass(classes)|without('id') }}>
  {{- items -}}
</div>

In #4 I suggested to leave the code path for the old mailer almost entirely untouched, and create a new service only for the new mailer. If we make a new service in this issue, then it's not experimental. It might not be what we want for the new mailer. Because of this uncertainty I suggested postponing this issue for a few weeks at least.

I'm unsure where the notion of of custom $ops comes from exactly, do we have any data that supports that this is a thing people did?

The user_registrationpassword module "sort of" makes up some new $op values. However from a quick inspection it has separate config, _user_registrationpassword_mail_notify() and _user_registrationpassword_mail(). So it likely isn't actually an example of this "thing", but it gives a hint why people might try it.

The MR here turns the $op argument into an internal implementation detail. It isn't API anymore and therefore may remain a string.

Please see #6. $op is definitely an API as any code that alters emails depends on the values. Also configuration depends on the values and the code in AccountSettingsForm must match. True, it's already a string and I agree it isn't necessarily correct to fix that by a widespread replacement everywhere in the code. Still, I feel that creation of separate API functions that make the $op appear more internal isn't necessarily helpful. It makes the interface complex and creates dull boilerplate copy/paste implementations. Could we instead create an enum on the interface??

I removed the BC for custom $op parameters.

The comment for _user_mail_notify() clearly states the allowed values for $op, so I agree we don't need to support any others.

One thing to note is that this also changes the return value.

Of course as @berdir says, _user_mail_notify is technically an internal function. We have decided that sites treat it as an API (what else can they do?) so we will do the same. For an API, a return code change is a BC break, so I feel let's leave it unchanged. We can still have our preferred return code on the new interface. We could make sendToUser() a public @internal function for BC only, which simplifies _user_mail_notify(), removing the switch statement.

Thanks @danrod you are right.

I see it now: libraries/splidejs--splide/dist/css/splide.min.css (that's the path on my system anyway). My mistake was that I was looking for the file within the module not the library. I acknowledge that the explanation did include the word "library" - I just didn't get the meaning😃.

znerol → credited adamps → .

adamps → created an issue.

adamps → created an issue.

It looks like we could do something like this

if (function_exists('update_requirements')) {
  $requirements = update_requirements('runtime');
}
else {
  $requirements = \Drupal::moduleHandler()->invoke('update', 'runtime_requirements');
}

My sites are still on D10 so I can't test it.

All patches go into 2.x please, which is the default branch.

I believe this is "working as designed" and the behaviour is the same in swiftmailer and Drupal Symfony Mailer Lite.

If you pass in MarkupInterface, then it is treated as HTML. You can control line breaks using HTML tags such as br and p. Sure, Drupal can generate the correct line endings, but they don't have any affect on the layout of HTML.

I guess there must be a bug in your config but there's not enough info to say.

Sorry this issue queue isn't for questions like that, it's not a helpline. Try https://drupal.stackexchange.com/

"Import" converts the current site config to the new format, which is quite a different thing from a default. If you want the defaults then you should do "enable" (without importing), or "reset". All of that is part of the "mailer override" module, which is for optionally replacing the implementations of legacy mail implementations from other modules (so yes it's non-standard, it was tricky).

If imported like the User module then you lose all of your changes if you import them again.

True, and it gives a clear warning. How else could it be? I am not a magician😃.

===

If you are writing a new mailer, then ignore all the above. You can just provide a straightforward default in config/install directory. Then you could compare/reset it in the usual way with the config update module. This seems to be exactly inline with how config works in other parts of Drupal. Does that answer your question?

You could also hardcode a value into your code. It could still be overridden in the UI, but the UI wouldn't see the value from code. So maybe this what you tried that wasn't working?

The problem is that Core doesn't provide an API for the information that we need. So I had to call an internal function, which has now been removed. We can check the issue that removed it to see if it explains the replacement to use.

Good idea. I like ✨ Execution environment: Safely run code inside an environment with partially substituted system state / config Active . As you say, it decouples negotiation from switching. We can use it in the new mailer in ✨ Create a new mailer service to wrap symfony mailer Active or a similar issue. If we wish to back-apply it to the old mailer then we can re-open this issue.

We should close some old plan/meta issues or relate them if they're still relevant.

I feel we do currently have a problem with multiplying and semi-duplicating issues. I raised a bunch of issues a year or two ago. Some of them are a little out of date, but still they have many interesting ideas, and also some valuable discussions with @berdir and other core maintainers.

@znerol has since raised his own issues. They tend to operate in a different "space" without referencing the prior ones, often using different terminology. Some of them might now be outdated, others still contain important ideas or MR.

I feel that it would help now to merge into a single set of issues referenced from this IS that we all use moving forward. We can transfer information in from others and then close them as duplicates. Once the group has agreed that an issue is in scope, then we can open it up for coding and allow different MR to be compared side-by-side.

As per #29, @zengenuity and @adamps will aim to create a design and propose how it could map to issues, bearing in mind @berdir feedback. Then we can get review from the group, and if we manage to get agreement someone can do all the issue management.

I'm not sure how important discoverability and reusability of mail "templates" really is, it might in some cases even be undesired. We recently discussed that ECA explicitly doesn't support those special user password reset mails, to not allow to send them out in the wrong context, that could even be a security issue.

Interesting point. We definitely do need the discoverability as modules like DSM+ "Mailer Policy" need to know there is a password reset email so they can show a form to configure the subject and body. Removing the discoverability won't help security anyway: we'd end up instead with a form that allows the admin to type in the ID of the tag/key (so they type password_reset), and it would be equally insecure. Or the admin could apply the hijacking context setting globally to all user emails, or even to all emails. Or the admin could enable an email logging module and read the logs, or they could configure a transport that sends emails to their own private server, or enable a module that redirects all emails (normally used on a test site) to a different address.

During the meeting we added a new requirement C3 to cover this case, which I imagine would work a bit like this. The metadata for each email sending component can optionally include a setting to indicate a specific email category/tag is sensitive. The Email can have a corresponding isSensitive() function. We can create a Core permission "view sensitive emails". The functions that would cause a problem including setBcc(), setTo(), setCc(), getBody() can then all perform a check on the permission if the email is sensitive. I guess we need a setting for code to disable the check in case of trusted code that isn't related to a UI. We don't necessarily need all this for a beta however.

I'd avoid creating too many issues to discuss and implement every single bit. It's hard to get things into core piecemeal.

OK thanks for the advice. Still, I feel we need a way to take some decisions individually (and sure we could change our minds later), rather than evaluating the whole thing in a single MR.

I see that DSM has the concept of mail params on the object. That's still keys and not types that can be enforced, but I think that's an improvement over the single magic mixed type that this has. With mulitple params, we can still document them and their type.

Based on feedback from 1.x, now in v2.x we have a clear interface such as UserMailerInterface which has types for each function argument. These then get converted into the type-less array params, but we have at least ensured they are the right type in the first place. We also have a function setEntityParam(EntityInterface $entity) which automatically sets the param with a key matching the entity type, so it gives some level of type hinting. We could even add add code that prevents overwriting such a param with the wrong type.

In addition to the initial params set at the call site, we found that hook code sometimes just wants to record some data into params (perhaps to read again at a later stage in the email pipeline). Forms have setValue(), rendering allows adding anything to $variables. So probably we can't reasonably try to control everything that goes into params.

IMHO, the hardest thing of the whole process is experimental code/module and BC in all kinds of directions.

Yes you are right, we should have some requirements about that.

They'll still work, but sites will need to be aware to configure their mail transports in two places and so on.

That sounds scary to get working correctly, to test and support. Maybe we should allow only one active mail system on a site?? Even so it will be plenty hard enough.

I think we should have a single flag that switches all (supported) mails to the new API.

I was thinking the same thing. Let's keep it simple.

Don't think we want or can automatically transform hook_mail() implementations, there are too many custom things going on there with attachments, HTML and what not.

Actually it is definitely possible without too much complexity, we have it in DSM+. Not necessarily guaranteed to work 100%, but in practice we haven't really had new issues recently. Perhaps it doesn't need to be in Core - sites could choose to enable it from Contrib if they want to work that way.

So perhaps the requirements overall are something like this:

1. The experimental mailer reports which modules support which mail systems (some might support only one, some both).
2. The experimental mailer module provides a setting to choose which mail system is active: old or new. All modules use the active mail system. (We could skip this page at first, and use enabling the experimental mailer as the switch. However it's arguably a more realistic beta if we have the page, and it allows the user to see the level of module support before switching)
3. By default, emails are dropped if the module doesn't support the active mail system. However there is a mechanism to enable a Contrib module to perform conversion.

I feel that separate functions would likely just be a nuisance in this case as the implementations are all identical (except 'register_pending_approval' generates the extra mail, but that can be a simple if test like now). The values of $op aren't really internal because they form part of the email "ID" - module/key in old terms, tag or whatever we might call it. This will be referenced in the config/code of any module that is involved in building or altering user mails. The list of $op values, and their labels is needed for the form UI that controls such config.

Sorry cross-post removed a related issue - now fixed

Definitely +1 for the idea in general - I agree we should end up with a user mail handler service.

I'm not sure if we need this issue specifically for user module however. As @berdir pointed out in the roadmap, every module will need a migration from old mail handler to new. Likely during the transition, the new mail handler will call the old one whenever the site uses the old mail system; the old mail handler is marked deprecated then later removed at the same time as the old mail system. (Contrib could instead use a major version change, the old branch for the old mailer and the new branch for new mailer.)

The new mail handler service could often want a different interface from the old, so we might be better to create a new service name rather than reusing the old one. We might also wish to specifically identify these new services, perhaps with a tag (which would allow checking which modules support the new mailer - perhaps a neater alternative to @berdir idea of #[SupportsMailer] on hook_mail()). Perhaps they share a base interface. Perhaps they share a way of yielding metadata. In DSM+ we have UserMailer as both a service and a plugin, and I feel it works quite well - it's an option we can consider.

Although the old mail handler for most modules is a service, the old mail handler for user is _user_mail_notify(). However it doesn't necessarily make much difference, and we can use the same migration mechanism.

In other words, we don't necessarily need a migration/deprecation step separately in this issue when we are already going to have one anyway for introducing the new mail system. I feel that it might be worth waiting until we have a clearer idea of the overall mailer design, especially the migration part, before we work on this issue.

I feel that we should have a group decision process for agreeing which issues are actionable. At very least it could be like setting RTBC status - please don't put your own issue in, at least one other person should be involved😃. FWIW I would put this issue in the "Mail building Components" section in the roadmap.

Personally speaking, I definitely agree with the idea. We are currently having a little discussion in the issue about how to do it, then once we have agreement at that level it becomes actionable.

I copied the overall design in from the meeting notes and tidied it up slightly. Next step @zengenuity and @adamps to develop ready for review.

Older version saved for reference

Proposed resolution

Introduce a new plugin type and a config entity type which work in tandem. I.e., use plugins to realize a strategy pattern (substitutable implementations) and the config entity to select one of them for each case. The combination of plugin type and config entity type is a known pattern in Drupal core. Examples for this architecture are Action, Block and Editor.

A sensible name for the plugin type and config entity in the mailer realm could be EmailTemplate.

The parts should roughly fit together as follows:

• Each implementation (email template plugin) declares which type of email it is capable of rendering. The responsibility of an email template plugin is to turn input data of a specified type into an email data object.
• Each call site (email sending code) declares the type of email and its specific variation it wishes to dispatch.
• A config (email template entity) links the email type+variation combination to a specific template implementation.

Call site

The call site (email sending code) interacts with the email template config entity. Config entities are easy to deal with in both procedural and object oriented code. The render() call is forwarded to the plugin implementation.

Procedural example (error handling removed):

$account = User::load(1);
$template = EmailTemplate::load('user.password_reset');
$email = $template->render($account, $account->getPreferredLangcode());
$mailer->send($email->to($account->getEmail()));

EmailTemplate config entity

Config entities can track their dependencies. Config which is shipped in a config/optional directory is checked whenever a module is enabled or uninstalled. Optional config is installed as soon as all of its dependencies are met and removed whenever one of its dependencies breaks. It follows that the experimental mailer module can safely provide config for all mail sending core modules. Whenever the experimental mailer module is enabled, the subset of email template config entities having all dependencies met is installed. Whenever the experimental mailer module is disabled, all the email template config entities are removed and the system is config wise back to the point before enabling the mailer module.

Contrib and custom code can list the email template config entities. This represents an exhaustive list of all transactional mails potentially sent out from a site.

EmailTemplate plugin

Core may initially ship rudimentary mail template plugins mirroring the current behavior of core hook_mail implementations. Better implementations (e.g. based on entity view modes) can be added later in the development process.

Contrib mail sending modules (e.g., Commerce, ECA, Simplenews, Webform) can add their own mail template plugins and config entities for their own mail types.

Contrib mail building modules (e.g., DSM+, Easy Email) can ship additional mail template plugins for existing mail types and substitute the default implementations.

Coming out of discussions at our last meeting I have been contemplating how we can progress efficiently with group agreement.

I feel that we will move forward by making individual decisions. Each one is a specific choice in a single localised area from a defined set of options. First we make a choice regarding the existence of an EmailInterface, which if we agree, leads to an issue. Then within that issue we have separate choices about each of the fields that could be on it, leading to an actionable issue that can be coded (the coder can make any more detailed choices themselves). Probably we will make 100+ of these choices together as a group. It seems a lot, however many of them can be done in 2 minutes. I felt that @zengenuity mostly agrees (please correct me if I'm wrong).

Lorenz says that he likes to think in code. I feel it would work well to make prototypes within one of the issues that we agreed as a group. We could have an issue for creating the component mail building code, and we could agree that for each idea someone had they would create a MR for only user module - maybe just 50 or 100 LOC. The author could explain the key points and advantages in a comment.

However prototypes for the large parts of the system (500-1000 LOC) from my point of view are not good for making decisions because it is like making 50 choices at once, and it's too much to take in. Potentially they generate multiple issues = noise in the issue queues, sometimes duplicating other issues, and drawing the attention of outside reviewers to the prototype rather than the decisions. Personally speaking I wonder if this kind of prototyping might even be better on a personal gitlab clone or similar??

How does anyone else feel?

Coming out of discussions at our last meeting I have been contemplating how we can progress efficiently with group agreement.

I feel that we will move forward by making individual decisions. Each one is a specific choice in a single localised area from a defined set of options. First we make a choice regarding the existence of an EmailInterface, which if we agree, leads to an issue. Then within that issue we have separate choices about each of the fields that could be on it, leading to an actionable issue that can be coded (the coder can make any more detailed choices themselves). Probably we will make 100+ of these choices together as a group. It seems a lot, however many of them can be done in 2 minutes. I felt that @zengenuity mostly agrees (please correct me if I'm wrong).

Lorenz says that he likes to think in code. I feel it would work well to make prototypes within one of the issues that we agreed as a group. We could have an issue for creating the component mail building code, and we could agree that for each idea someone had they would create a MR for only user module - maybe just 50 or 100 LOC. The author could explain the key points and advantages in a comment.

However prototypes for the large parts of the system (500-1000 LOC) from my point of view are not good for making decisions because it is like making 50 choices at once, and it's too much to take in. Potentially they generate multiple issues = noise in the issue queues, sometimes duplicating other issues, and drawing the attention of outside reviewers to the prototype rather than the decisions. Personally speaking I wonder if this kind of prototyping might even be better on a personal gitlab clone or similar??

How does anyone else feel?

This is a subset of ✨ Create a new mailer service to wrap symfony mailer Active .

Interesting. You have made it very generic and extensible. The downside is complexity: 3-4 times the lines of code compared with the straightforward approach in Drupal\symfony_mailer\Mailer and 9 extra files (plus it needs tests). The call stack becomes a harder to follow with many nested anonymous functions (whereas in fact only the render switch requires that - the others all operate inline).

My feeling is that we don't want such complexity unless we can conceive of a clear use case for it immediately inside Core. I do feel attracted to the idea of having an environment manager separate from the Mailer, and I would simplify it to a fixed set of things that it can alter, i.e. the 4 we already know. If anyone in Contrib really wants to add a 5th param then they can extend the class, and we could write our class to make that easy. Then we only need 2 files, probably less than half the code, and the interface simplifies to something like this (the manager will swap a NULL for the appropriate default)

$this->environmentManager->executeInEnvironment($function, $langcode = NULL, $theme = NULL, $user = NULL)

OR perhaps we could keep $environmentParams if you prefer, with string keys ['langcode' => 'XXX'] and if they are missing it uses the default??

When switching language, I believe we also need to do

$string_translation->setDefaultLangcode($language->getId());
$string_translation->reset();

TODO: Allow third parties to alter the environment params. E.g. invoke hook or fire an event

FWIW I feel we don't want a separate event. We can use the same event as for altering any other parameter on the email. An extra event would be nuisance for code such as DSM+ Mailer Policy that supports altering any of the parameters generically.

I started developing DSM+ 4 years ago to meet very similar same user stories and requirements. Through the experience of 46k sites we fixed 313 issues. Various other Contrib modules and likely numerous sites have written code to integrate with the interfaces so if we can keep those interfaces it would simplify migration of these sites to Core. Recently the 2.x branch is a significant rewrite based on all the things we learnt in 1.x. I agree we shouldn't blindly copy it into Core, but also we shouldn't blindly ignore it. From my perspective, before prototyping a new system we should look at the existing one and see what we feel needs to change and why.

Certainly @znerol has some ingenious ideas which we can also include in our overall solution. Also I see some things that I tried 4 years ago and didn't seem to work out so well; I see some key points that I learnt through direct user feedback that he likely would also need to consider. This is entirely natural as DSM+ is a mature codebase proven in the field in comparison with a prototype developed over a few weeks (or maybe months) by one talented developer.

Thanks @berdir

I don't think LOC is the primary factor to consider for this. Also because the current examples are way more complicated than they need to be due to feature flags, the experimental module and BC. The actual implementation would be a lot less.

I accept all that and I certainly don't believe we should be trying to minimise lines of code in an artificial way. I mentioned LOC just as an illustration - I could equally well present a list of detailed specific points to demonstrate my point about complexity.

But overall I'd clearly recommend keeping the actual mail builder interface/API as plain and un-opinionated as possible and start with that.

I certainly agree with that and it's exactly what I was trying to do with #19.

I had a look at 📌 Introduce EmailTemplate config entity and plugin type Active and 📌 Use EmailTemplate config entities in core modules Active . There are a lot of really interesting and ingenious ideas.

My first impression is that it's much too complex especially in comparison with DSM+ yet fewer features (e.g. still only plain-text).

DSM+ v2.x has a class UserMailer which has 57 lines of code, which includes HTML support but excludes the actual configuration of subject and body.

In 📌 Use EmailTemplate config entities in core modules Active there are

• 9 config files of 15 lines each
• 300 lines of PHP split across 4 new files
• 80 lines of code in _user_mail_notify() which is code that presumably fails to meet the requirement [D3 CAN_ALTER].

So 10 times as many lines, and 20 times as many files.

IMHO, the EmailTemplate config entity isn't the right direction. [D1 METADATA] can simply and naturally be solved by putting the email sub-types into the annotation for the plug-in. This is a standard pattern that we see many times throughout Core and Contrib. Code can alter the plug-in definitions and implementations using an info_alter() hook.

AFAICS the EmailTemplate config entity contains two parts:

• Allow altering of plugin metadata info using config
• Allow replacing of plugin implementation using config

I don't yet see a clear requirement for this added complexity, which seems non-standard for Core. It's also less robust as email sending would break if the config is missing.

Personally speaking I still feel that the "Drupal adaption layer" (I'll abbreviate to DAL) is the place to work next because it is the foundation for everything that comes after. We are working up the stack layer by layer.

I'll try to describe it here. We already have the delivery layer which has interface Symfony\Component\Mailer\MailerInterface. There is a single function send(RawMessage $message, ?Envelope $envelope = null).

My idea is that the DAL provides an interface with a very similar feel.

• Drupal EmailInterface which enhances the Symfony Email.php with Drupal information
• Drupal service Mailer class implementing MailerInterface
• Single function send(EmailInterface $email)

It covers the following requirements:

• [D4 EMAIL_INFO] by creating EmailInterface
• [B3 LANGUAGE_CONTEXT], [B4 THEME_CONTEXT], [B5 USER_CONTEXT], [C2 SAFE_ACCESS] as the service does context switching
• [C1 SAFE_ATTACH] as the service provides an API, although this might come after the beta
• [D3 CAN_ALTER] as the service will fire events
• [D5 STRUCTURED_BODY] as the EmailInterface will have get/set methods for accessing a render array.

So it does a lot, yet it is fairly compact (1000 lines of code total?) and can be split into stages, each one a clearly scoped issue. I feel that it is less controversial than the layers that come above, and we have a chance to get agreement in a reasonable time.

We discussed this during a call and by agreement we updated the IS to remove the statement of scope from #16.

I have updated the IS to give each requirement a code and a short name so we can reference them in the comments. Obviously this will only work if we avoid changing them😃.

Thanks it looks good. This is an important issue, so please let's have tests for some of the cases at least - doesn't have to be all of them.

We can create a new test case function which creates a user without an email address. Then visit a few of the important pages and check the result.

For sure please let's include a test for "Your account has no email address". Then at least one other case, which would likely be a check that a certain page has the normal text rather than a crash, which was previously the case before the patch.

Back to "needs work" for the tests.

Great this is becoming clearer.

The current code is intended to work like this. Staff recording a declaration have 3 options:

1. Invalid or if you prefer "never valid"
2. Valid: all the parts are present and no confirmation is needed
3. Confirmation required, and not yet sent (= confirmation missing)

What I had expected is that when we send the confirmation (which could be triggered automatically immediately by email, or could be later manually by post), we do 3 things:

1. set "confirmation sent" date to the current day
2. set the "claimable" date to 30 days from today
3. set the validity to valid - because in the absence of any cancellation, then we have done everything that we are required to do

I believe that from the above information we can distinguish all the different states as required. We can tell the staff's assessment either that validity was reached by means of a confirmation vs inherently valid by checking if there is a "confirmation sent" date. Staff can indeed change their minds by editing validity and if required other fields (which in some cases we could automate, e.g. can only have a claimable date if also valid). We can tell when it's OK to make a claim. We didn't need any background cron job to change validity after a time period, we didn't need complicated logic in the isValid() function, and we didn't create caching problems by means of a computed field that is date dependent. The key information is present directly as fields which can be used in views: view all claimable, or view all confirmation required.

If we get a cancellation, then we have two options. Ordinarily we set the "cancelled on" date to today, or whatever future date was indicated in the evidence. In case of a retraction (cancellation within 30 days of confirmation letter, implicitly or explicitly stating the declaration was in error) then we set the "cancelled on" to match the start date. The declaration can therefore never be used to make a claim, however its entire history is still evident from the various fields, and we retain the possibility for staff to change their mind.

How does that all sound to you?

I already mentioned in #9 that I believe there won't be universal agreement on the configuration structure: Core will have a basic one and Config may need something more powerful.

The 3rd paragraph of the IS states a clear scope which excludes configuration, suggesting instead that the next step is the
"Central Mailer". That's not really a good name, probably it would better be "Drupal adaption layer" - continuing upward from transport and delivery layers that we already have. The main content is the issues in #10, which are still awaiting response. The plug-ins and configuration would sit above this.

Has anyone found a way to implement HTML emails correctly using Symfony Mailer (DMS+) for Drupal?

Yes very many people have.

Only a small number of people are having a problem. There isn't yet enough information here to reproduced the problem. Normally it works, there must be something unusual that stops it working for some sites.

/usr/sbin/sendmail -t should likely be the default instead of -bs

Please see #2. Seeing as symfony don't recommend -t I doubt we will add it here. Also note that the same code is now in the experimental Core mailer, and we would like both to remain consistent.

My second point is an attempt to respond more directly to what you have done here.

I'm mostly confused about the problem/motivation in the IS😃.
1) I believe that we are not currently updating the validity field when the confirmation period passes.
2) According to my understanding of the cancellation issue, neither type of cancellation has a validity state. It's simply a restriction of dates, which could include back-dating the cancellation so that it was cancelled on the day it started.
3) I agree with your first sentence, and I don't believe we plan to make such an assumption.

And I don't understand many of the changes in the MR. It's not clear to me how they relate to the points in the IS. The code is as it is for a reason, and those changes seem to break things. I'm not attracted to the idea that interface concepts are different from the underlying state - especially isValid(). I don't like the idea that invalid is different from a negation of valid, as I feel that's confusing. I don't really see a need for confirmed date in addition to a claimable date - already there are about 6 dates, and it's quite complex.

I guess I don't really get what you are trying to do here, sorry.

My first point is regarding issues and features. Where possible I feel that we should generate MR related to features with a defined spec in the IS.

We have 🌱 The problem of changes Active for cancellation which refers to 📌 Figure out contexts and donors Active for a very long detailed discussion. The current cancellation code in 1.x branch is not especially meaningful or correct, as it predates many important discussions. If you are not happy with the results of the previous discussion then I would suggest to continue it in the other issues although potentially we will never finish this project if we keep changing our mind😃. It seems that you are now using "retract" in a different way than you did in the IS of the other issue.

I feel that cancellation is an important, big, complex feature. I wouldn't want to mix it with anything else, I wouldn't want to do it in parts, I would do the other issue in entirety when we reach the appropriate point.

Then in the roadmap we have a bullet like this:

Written statement. There is a field for Staff to mark a written statement as sent on a specific date. There isn't any mechanism so send one which could be via email or generating text for a letter to print or save as PDF. There isn't any further audit information such as which template version was used (could be a config entity).

It would make sense to me to turn this into an issue which again I would class as a feature request. We could have a discussion on that issue about how to do it.

adamps → created an issue.

The proposed resolution covers only some of the acceptance criteria. Others parts are covered well in some existing issues:

• ✨ Create new mailer service based on symfony Active
• ✨ Create a new mailer service to wrap symfony mailer Active
• ✨ Events/callbacks for new mailer service Active

I like the idea of a plug-in and a config entity.

I suggest that the plug-in needs full control of the email building process - including to, langcode, etc. So it would be

$template->send($account);
// OR
$email = $template->render($account);
$mailer->send($email);
// OR
$template->render($account)->send();

The advantages are: all the code is in one place; there might be multiple call sites, and we don't want to duplicate logic to each; code that overrides the plug-in or alters the emails can override/alter all the parts - otherwise the to address and langcode are effectively locked; also the to address may depend on language (e.g. send to the site address and site name, the latter being translatable) so it should be evaluated after the language switch.

Other email template types would have different parameters. Contact messages would need $template->send(MessageInterface $message). Simplenews needs 2 params, the issue and the subscriber. This suggests that we need an interface that informs the calling code what parameters to pass - there was clear demand for this in DSM+, and on other issues linked from the META. The interface is identical for all user sub-types and the implementation likely is too, but the config of course is different.

The mechanism of EmailTemplate::render() cannot support this variation in the interface (it can accept arbitrary extra parameters, but it cannot support type information => IDE code completion). This suggests instead the possibility that the plug-in is also a service, which allows all the flexibility of auto-wiring, dependency injection, auto-configuration (to set the plug-in manager automatically to be the factory for the service) etc. We can put the burden of loading the config entity into the framework code rather than requiring each call site to do it - which is important as I believe there won't be universal agreement on the configuration structure: Core will have a basic one and Config may need something more powerful. So the call site code can look like this:

__construct(protected readonly UserMailerInterface $userMailer){}
$this->userMailer->send('password_reset', $account);

or

Drupal::service(UserMailerInterface::class)->send('password_reset', $account);

Note that the interface is the same as the existing _user_mail_notify(), which seems like a sign we are on the right track, and should help during the transition. Once the new mailer is no longer experimental, we could alter all call sites to call Drupal::service(UserMailerInterface::class)->send. Then the new mailer plug-in could check if the site has chosen to enable the new mailer service yet, and if not instead call _user_mail_notify().

I guess there are two possible options: have a base field, and hide it when it's not needed, or add a bundle field whenever it's needed. For the 2 web self-declare cases, the explanation text isn't in the evidence, so we would need the bundle field.

- the explanation could actually be on the confirmation email, so it could be missing initially

I had also considered that we could track the confirmation email wording or template version. I recall that the guidelines said something about "demonstrating that the email had been sent according to a particular template" being acceptable.

Maybe best to make this a seperate issue? I can do it.

Thanks I raised 📌 Fix FunctionalJavascript test Active .

adamps → created an issue.

I ended up deleting the last two items:

4) Fix deprecation notices in test results (although some come from Commerce tests?)

CheckoutTest has deprecations, but AFAICS they come from Commerce.

Remaining self deprecation notices (2)

2x: Renderer::renderPlain() is deprecated in drupal:10.3.0 and is removed from drupal:12.0.0. Instead, you should use ::renderInIsolation(). See https://www.drupal.org/node/3407994
    2x in CheckoutTest::testCollectDeclaration from Drupal\Tests\gift_aid_commerce\Functional

Other deprecation notices (12)

  12x: Using the 'timestamp' formatter plugin  without the 'tooltip' and 'time_diff' settings is deprecated in drupal:10.1.0 and is required in drupal:11.0.0. See https://www.drupal.org/node/2993639
    12x in CheckoutTest::testCollectDeclaration from Drupal\Tests\gift_aid_commerce\Functional

5) Move the menus one level down, e.g. put Gift Aid under Services? This is the normal level, and it means that the various admin pages for the module are all visible as tabs.

Things have changed since I wrote that originally. Each of the admin pages now has its own tabs, so it no longer applies.

This is presumably a beta-blocker, so I'll comment on it to bump it up the list.

This seems essential - it's a bit that got left out of ✨ Fix GiftAidOverviewController Active

Without this, anonymous declarations will fail to arrive on the user, so I've altered the issue settings.

I'm happy for you to say it's not a road-map priority. Even so, I feel we should have these clearer issue settings to help us or anyone else see the status in the future.

We've done most of these now, the rest likely aren't important.

Thanks. I will make the fixes in 📌 Tidy up Active

I kind of like having submodules for atomic features

Fine. Potentially it could use some documentation so people can find it and a test so that it has a chance of working when they do😃.

Now have a triple green on the test results😃. OK with you?

I have never installed the necessary for infrastructure for FunctionalJavascript tests. Probably we need just one test, so it's not really efficient for me to learn it. Would you be willing to do it yourself please?

In DeclarationFormTest::testOralDeclarationRecordedByStaff(), the return statement should be down to just before @todo Fix when added code for cancellation.. It just needs some ajax to add evidence, which might start something like

$this->assertSession()->waitForElementVisible('named', ['button', 'Add new Gift Aid evidence'])->press();

I'm keen to do some basic tidy up at this point for clarity and efficiency moving forward. I'm interested in the first 3 please. What do you think?

adamps → created an issue.

but it breaks the links.task

Exactly.

The tests now pass again, so let's get this in.