Extract _user_mail_notify() into a user MailHandler

_user_mail_notify() is technically an underscore prefixed function and not covered by BC. But there are dozens of calls to it, so we should deprecate it properly.

A bit unsure if we should do a straight conversion or actually rethink things. There are various issues open about improving docs, arguments and changing how it works, such as ✨ Use an enum for user email notification types Needs work , as a mailHandler, we should also split it into dedicated methods and make the $op stuff at least an internal thing. Deprecation should be easier than changing params on the existing function. But it will make this more complicated.

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.

Sorry cross-post removed a related issue - now fixed

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.

📌 Improve documentation of _user_mail_notify Postponed regrettably ended up in a bike-shed discussion on whether it is necessary to update coding standards before committing the docs improvement. The MR in ✨ Use an enum for user email notification types Needs work tries to change every single occurrence of an $op like string regardless of the context. By implementing the proposal from #2 (individual methods on a MailHandlerInterface), it is possible to circumvent both of these pitfalls.

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

#3463923-11: Improve documentation of _user_mail_notify → points out an interesting angle:

I must say I don't particularly like this, the enum would be much nicer. Regardless, I think it's possible (even if ill-advised) to alter or extend the available options, so this might be needlessly restrictive.

In Drupal 7 it was indeed very easy to extend the list of possible operations via a contrib or custom code. Simply by adding a pair of variable_set() calls in an .install file ensuring that user_mail_OP_subject and user_mail_OP_body contain some meaningful text. I haven't tried it, but it still might be possible to do something like this with a smart combination of form alters and config schema hacks.

This practice should therefore be deprecated properly.

Interesting observations in 🐛 _user_mail_notify return error when email sending is aborted Active regarding the _user_mail_notify() return value.

Issue 🐛 Use email verification when changing user email addresses Needs work seeks to add two new operations. It would be good to land this and the other issue in the same release.

CR is good but it needs details which $op is transformed into which method in new interface

Custom $op still can be implemented by decoration for the service

Othen then question about need for sendDeprecated() in public service looks RTBC

Added some quick thoughts.

My main concern was that someone who used to use custom OPs would now have that option taken away from them. But giving it some more thought, nothing stops them from calling the mail service with said custom OP themself. So I left a few nits left and right, but nothing too major.

The new service copies the old code in ways that seem to make sense, but also do not go any further. One could argue now would be a good time to revisit some of the comments, but if you want to limit scope to "just move the code" then I could get onboard with that too.

Moshe seemed okay with the approach taken here too (via Slack) and so am I. So as far as subsystem maintainers are concerned, you are 2 for 2. Thanks for working on this.

> My main concern was that someone who used to use custom OPs would now have that option taken away from them. But giving it some more thought, nothing stops them from calling the mail service with said custom OP themself.

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?

This is technically an internal function that you weren't even supposed to use, it's just a very common thing to do and convenient to use, and the two other issues that this references would have updated the docs to specify the allowed values and the other introduced an enum. both specifically disallowing the notion of a "custom $op".

Same with the underlying config. you can not add more things because you can't provide schema for it.

> The new service copies the old code in ways that seem to make sense, but also do not go any further. One could argue now would be a good time to revisit some of the comments, but if you want to limit scope to "just move the code" then I could get onboard with that too.

The idea is that we isolate the code in a way that allows us to provide an alternative implementation using the new mail API, first in an experimental module, then later moved back to user (at least that's my recommendation).

I removed the BC for custom $op parameters. Instead _user_mail_notify now maps the $op to the correct method.

One thing to note is that this also changes the return value. Before, FALSE was returned on error and NULL when mail sending was suppressed (either by config or by a hook). The new methods return FALSE in both cases.

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.

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?

No, and people shouldn't have. I'm trying to predict possible fallout from the changes. Like how we sometimes go the extra mile even though our BC policy says we are free to just change constructor arguments on some services: It's being nice. But I doubt we need to do that here.

The idea is that we isolate the code in a way that allows us to provide an alternative implementation using the new mail API, first in an experimental module, then later moved back to user (at least that's my recommendation).

Yeah, I got that and I like the idea. My suggestion was more that, when copying the old code into the (perhaps temporary) service, we might want to see if the comments are still up to standards. But if we want to keep the scope narrow, I'd be fine with that too. As you indicated: This code might not live very long.

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]);
  }

}

I restored the BC for _user_email_notify() and also the return type behavior. Dispatching op to method call in _user_mail_notify() is actually more work than keeping BC.

I also found a way to remove the interface. It is possible to mark methods for setter injection with the #[Required]. That way, a subclass doesn't need receive and pass on a MailManager via its constructor. It only needs to declare the services which it really needs.

I tried to update the issue summary in order to be more precise about the intention here. Regarding #20, this issue is about a different abstraction layer. It has no influence on the design of a future mail building layer.

Regarding the discussion whether this should be enums or separate methods: 🐛 Use email verification when changing user email addresses Needs work would definitely profit from separate methods. The new email address could be passed in via an additional argument. The current MR has to tunnel it via the param array. This could be solved much better with a method with dedicated signature: https://git.drupalcode.org/project/drupal/-/merge_requests/5773/diffs#di...

Updated the deprecation message and the change record. _user_mail_notify() is replaced by an internal service. It follows, that there is no replacement API, this must be reflected in the CR.