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.

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.

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".

Yes, of course.

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".

This API has nothing to do with the new mail system, it's explicitly an abstraction from the actual mail API, so that we can switch it out underneath.

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. It's just a welcome side effect that this allows to switch out the code with something that uses the upcoming new mail API, within the experimental mailer module. Because having experimental code in non-experimental modules is tricky. And once that API is no longer experimental, it will move back into user module and replace the current default implementation.

And eventually, maybe, we can then decide to deprecate this API again, if we think that working directly with the new mail API is sufficient and covers all aspects, such as enabling/disabling certain mails. Or we maybe we won't, because you could even use this abstracted service to send out SMS/app notifications instead of/additionally to e-mails. But that's several steps in the future and not relevant for now.

I went through a gitlab code search ( #3051266: Searching for code across all of Gitlab → ) in order find out how _user_mail_notify is used in contrib. In order to exclude core clones and distros, I used the following query:

_user_mail_notify -path:core/modules/user -path:modules/contrib

That returns 228 results. I went through half of them and manually assessed how the function is being used. There are two bigger groups and a couple of interesting special cases.

Modules dealing with user registration

• https://www.drupal.org/project/better_register →
• https://www.drupal.org/project/bulk_user_registration →
• https://www.drupal.org/project/business_core →
• https://www.drupal.org/project/cas →
• https://www.drupal.org/project/civicrm_entity →
• https://www.drupal.org/project/cognito →
• https://www.drupal.org/project/commerce →
• https://www.drupal.org/project/commerce_guest_registration →
• https://www.drupal.org/project/created_account_register_message →
• https://www.drupal.org/project/crowd →
• https://www.drupal.org/project/entrasync →
• https://www.drupal.org/project/firebase_authentication →
• https://www.drupal.org/project/iyo →
• https://www.drupal.org/project/jsonapi_user_resources →
• https://www.drupal.org/project/minimal_register →
• https://www.drupal.org/project/nodehive_core →
• https://www.drupal.org/project/openlucius →
• https://www.drupal.org/project/opigno_learning_path →
• https://www.drupal.org/project/resend_register_mail →
• https://www.drupal.org/project/resend_user_login_link_user_operation →
• https://www.drupal.org/project/social_auth_hid →
• https://www.drupal.org/project/user_csv_import →
• https://www.drupal.org/project/webauthn →
• https://www.drupal.org/project/webform_user_registration →

Modules dealing with password resets

• https://www.drupal.org/project/admin_password_reset →
• https://www.drupal.org/project/diba_security_policy →
• https://www.drupal.org/project/eep →
• https://www.drupal.org/project/login_emailusername →
• https://www.drupal.org/project/login_flow →
• https://www.drupal.org/project/mass_password_change →
• https://www.drupal.org/project/mass_pwreset →
• https://www.drupal.org/project/openstory →
• https://www.drupal.org/project/pass_reset_checkbox →
• https://www.drupal.org/project/passwordless →
• https://www.drupal.org/project/patreon →
• https://www.drupal.org/project/rest_password →
• https://www.drupal.org/project/rest_register_verify_email →
• https://www.drupal.org/project/secure_password_reset →
• https://www.drupal.org/project/social →

General purpose / all ops

• https://www.drupal.org/project/if_then_else →
• https://www.drupal.org/project/mail_debugger →
• https://www.drupal.org/project/rules →
• https://www.drupal.org/project/user_account_emails →
• https://www.drupal.org/project/user_api →

Special cases

GSIS (GOV.GR) Login →

They are actually in the first group (Modules dealing with user registration) but they do just call $account->activate(); to trigger the registration email.

Own implementation

The following modules ship their own version of _user_pass_notify.

• https://www.drupal.org/project/group_notifications →
• https://www.drupal.org/project/magic_code →
• https://www.drupal.org/project/user_registrationpassword →

Custom op

The Forgot username → module actually did implement a custom op (sight!).

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.

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.

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.

The individual functions are fine. They provide a good place for docs and we get rid of the $op strings at the call site.

I updated the issue summary and the change record. This is ready from my side.

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 didn't have time to read the comments properly and review the update, just wanted to add one note for now.

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.

_user_mail_notify() is an internal, underscore-prefixed function. We don't *have* to do anything, we are just nice because a lot of contrib would break. Specifically, we are not forced to provide a 1:1 replacement of anything, we decide how our future API's look like and what we want to support. We can also deprecate an implicitly internal function with an explicitly internal service. It doesn't mean that it can't be called, it just means that we might change it.

What we did in some OOP hook conversion issues is that we deprecated some functions, _underscore prefixed and regular ones, without replacement, because the actual replacements are protected and can't be called. We could do something similar. We could deprecate _user_mail_notify() but *not* call the new handler, just trigger the deprecation message. Any call to the old function continues to work as-is but doesn't go through the new API. It would mean they wouldn't use the new mail API once replaced, but that's perfectly fine IMHO, especially as long as the whole thing is still experimental.

Any call to the old function continues to work as-is but doesn't go through the new API. It would mean they wouldn't use the new mail API once replaced, but that's perfectly fine IMHO, especially as long as the whole thing is still experimental.

Exactly. This is the responsibility of NotificationHandler::sendCompat(). A replacement service implemented by the mailer module either could override it (and throw an exception to make it explicit that _user_mail_notify() isn't supported by the experimental mailer). Or it could leave it alone and then a mail would be created and sent by the legacy mail system.

Filed ✨ Provide a public PHP interface for user registration and deprecated _user_mail_notify() register ops Active and ✨ Provide a public PHP interface to initiate a password reset and deprecate _user_mail_notify() password op Active as follow-ups.

> Exactly. This is the responsibility of NotificationHandler::sendCompat()

Yes, my suggestion is to specifically not move that to the new service at all. Having it there means we'll need to remove sendCompat() in the future. That method currently doesn't explain that you're not supposed to call it, it explicitly "supports" an arbitrary $op and does _not_ trigger a deprecation message. forgot_username could call that directly and then we'll just break that later.

We want to deprecate _user_mail_notify() fully IMHO, we want to deprecate/remove every single .module file function we have left in core. They all need to go.

The current state to me is very confusing, with the inconsistent deprecation and what you're supposed do as a module doing that. I'd also clarify for modules like forgot_username what they are supposed to do (implement their own hook_mail())

As far as I'm concerned, we can fully deprecate _user_mail_notify() with @internal or without on NotificationHandler. IMHO, we don't need an interface to do so. An interface means we support alternative implementations, @internal means you're not supposed to call it. But happy to let a core maintainer decide on that.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.