Introduce theme negotiator for mail and use it in mail manager

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

Can the deprecations be updated for 11.2

Not sure who to ping to review this one.

Deprecation message updated. Good candidates for reviewers are the maintainers of the mailsystem → module I guess.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

> Good candidates for reviewers are the maintainers of the mailsystem module I guess.

the maintainer of that module is following this issue, I'll try to review soon.

Fixed issue reported in #13.

Apologize for letting this one sit.

But does need a rebase again

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

Conflicts due to 📌 Use ::class instead of string literals in plugin managers Active resolved.

Does this require a CR you think?

I guess yes.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

Pipeline passed.

Some comments on the MR. I'm also wondering if the mail theme classes shouldn't be in the `Drupal\Core\Mail` namespace instead of `Drupal\Core\MailTheme`? Though I think everything mentioned is personal preference and not necessarily a blocker.

Thanks, I like all of the suggestions.

I'm also wondering if the mail theme classes shouldn't be in the `Drupal\Core\Mail` namespace instead of `Drupal\Core\MailTheme`?

My plan was to deprecated everything in Drupal\Core\Mail eventually. 📌 [PP-1] Add symfony mailer transports to DIC Postponed introduces Drupal\Core\Mailer namespace as a replacement.

Added a CR draft.

Great. Here are my comments.

1) If we apply this to the existing mailer then it seems not to meet the back-compatibility requirements of a minor release.

• Change in behaviour: introducing this code could change the theme used when rendering emails. This in turn affects which templates and CSS files are used.
• Change in UI: before it was the mailsystem one and now it is the Core one.
• Change in config model: before the mailsystem setting now the Core one (requires a change to installation profiles etc).

Secondly it potentially creates forward-compatibility problems because then these changes aren't experimental, and we might need to make changes as we develop the rest of the mailer.

If we add it only to the new mailer, and we have a global setting whether to use old mailer or new, then this solves the problem as sites have a choice when to adopt it. Sites using the old mailer anyway have an existing working solution using mailsystem and might even prefer that we leave them in peace until one big change when they switch mailsystem.

2)

The Drupal Symfony Mailer project implemented this functionality in a more flexible way. However, since it is reusing patterns unique to that module, the mechanism isn't easily portable to current core architecture.

Yes I understand your point. I feel that the key here is to ensure that we can re-implement the DSM flexibility on top of the mechanism proposed here.

The main question I have is regarding MailThemeNegotiator::applies(), which requires the choice of theme to depend only on MailTemplateID. This doesn't "unambiguously identifies the email to be sent" as the IS states but identifies a category (the email also varies with the params etc). I can imagine scenarios where we would require more flexibility: e.g. the site has multiple brands with different main themes. We might check the user who will receive the email, see which brand they registered with, and choose the matching theme.

I feel we want instead is the email object - and I am aware this doesn't exist yet, however we could create a placeholder class for the purpose of this issue that we fill out later.

3) This patch is making key design choices about the new mailer in advance of an overall design (AFAIK). I feel it could be really useful to try to establish a design in advance, but otherwise I guess we can discuss design questions here.

a) The concept of a MailTemplateId. In contrib DSM v2 we have a string value named "tag" - which follows after the concepts in the underlying symfony code. This choice was actually influenced by a comment from @znerol on another issue😃 . It's a "dotted-string" format, with the first part being the module, hence allowing a hierarchy such as contact.page.copy and simplenews.subscriber.validate. The two alternatives are basically compatible so it's not a big deal, but I feel it could really help the community if we could make Core and Contrib consistent as much as possible. If you agree with my comment 2 then anyway this patch doesn't need to contain MailTemplateId so we can defer the discussion.

b) I agree with the basic approach of executing the mail building/sending code in a new context with the correct theme. When we take the whole design into account, I found 4 contexts that we need to switch: render, theme, language and user. This led DSM to choose a general approach, with a Mailer class which does all 4 switches. We could make MailThemeManager::switchTheme() public, and then it works more like AccountSwitcher (perhaps we call it MailThemeSwitcher?). I agree it could work with 4 separate switches stacked inside one another, each with try/finally and creating a new closure, but it seems less clear.

The language switcher is also missing, see 📌 Allow to change the current language Needs work , which should handle translations, see #3029003: TranslationManager has method setDefaultLangcode not defined on any interface. → .

The main question I have is regarding MailThemeNegotiator::applies(), which requires the choice of theme to depend only on MailTemplateID. This doesn't "unambiguously identifies the email to be sent" as the IS states but identifies a category (the email also varies with the params etc). I can imagine scenarios where we would require more flexibility: e.g. the site has multiple brands with different main themes. We might check the user who will receive the email, see which brand they registered with, and choose the matching theme.

I think it's perfectly fine if applies() and the actual logic considers global context like current user/domain/configuration, you can just inject that in your service. I don't expect we'd do any caching or so of this logic, so we don't need to be aware of all the things that might influence the decision, we just run through this on every e-mail.

On BC, I think it's fine if it applies to the current mailer if core doesn't come with en enforced default. Either nothing for now, or a configurable setting which could be a 1:1 copy of what mailsystem does, with current/default/specific theme list.

The Drupal Symfony Mailer project implemented this functionality in a more flexible way. However, since it is reusing patterns unique to that module, the mechanism isn't easily portable to current core architecture.

I suggested how the patterns from DSM could be introduced in Core in ✨ Create new mailer service based on symfony Active . In addition to being more flexible, it feels simpler, just $email->setTheme() without adding 3 new services and a service tag. So far my suggestion doesn't seem popular, and I've got no problem with that, of course you will make your own way.

You can ignore my other points from #25. Contrib DSM can just stick with the approach that it already uses and that works.