Selecting default mail theme / enforcing mail theme

@attisan, I've also wanted this functionality and I created a new email adjustment.

Now, the user can select the theme that wants to render the email from each mailer policy.

Great thanks, yes this is a very good idea. I added some comments.

@attisan, I agree that we should prevent switching active themes but I think that this approach is more user friendly.

@AdamPS, I've made the changes that you suggest and added the theme switch back functionality. Also, I can confirm that the DefaultsEmailAdjuster occurs between the switch and switch back.

Great thanks.

I've been thinking some more at the strategy level. It feels like the theme for an email is a really important concept and maybe we need to handle it more explicitly. We could add functions to UnrenderedEmailInterface: setTheme(), getTheme(). ThemeEmailAdjuster would just call setTheme() and DefaultsEmailAdjuster would call getTheme(), which should return the active theme if none has been set. Then we move the code to change themes into the Mailer class much like the existing code for language, creating a changeActiveTheme() method.

Advantages:

• Easy to understand.
• Allows hook code to change themes easily, without copying/conflicting with the adjuster.
• Removes the subtle relationship of timing between DefaultsEmailAdjuster and ThemeEmailAdjuster.
• Ensures the Mailer is fully in control of rendering, which will probably be important when we try to optimise caching and bulk sending of emails.

We need to take care with the timing of changing theme. Some proxy EmailBuilder implementations such as SimplenewsNewsletterEmailBuilder cheat and render the body during the preRender() stage. This suggests we should change theme at the same time as changing language, before preRender(), which feels right. However it creates a problem because ThemeEmailAdjuster is then too late to set the language. I wonder if we should add a preBuild() method to EmailProcessorInterface which is called along with the pre_build hook. It feels reasonable and makes things symmetrical with 3 hooks and 3 corresponding processor functions.

What does anyone else think?

It is a good idea to add a preBuild() method to EmailProcessorInterface.

In that way, the subtle relationship of timing between DefaultsEmailAdjuster and ThemeEmailAdjuster will be removed and the email adjusters will be more flexible for future features.

OK I will add preBuild() and let you know. Or maybe better to call it init()?

I think preBuild() is better.

https://git.drupalcode.org/project/symfony_mailer/-/blob/1.x/src/Mailer....

OK, done, see #3253847: Improvements to Email interface. So I think the remaining steps here are:

1. Update for 3253847: replace UnrenderedEmailInterface/RenderedEmailInterface with EmailInterface
2. Add to symfony_mailer.schema.yml for new adjuster (NB they are in alphabetic order)
3. New functions on EmailInterface setTheme(string $theme_name) and getTheme() (put just before the library ones?)
4. New function Mailer::changeTheme(string $theme_name) called at same times as changeActiveLanguage()

We need to think carefully about the default behaviour of getTheme(). Using the active theme is buggy because in various hard-to-predict circumstances the admin theme is active. I wonder if we should make the default behaviour of getTheme() this: use the active theme, unless it's the admin theme in which case use the default theme. Although this seems a little complex, the advantage is that it ought to cover the requirement of a high percentage of sites. Then we can remove the special case entirely from ThemeEmailAdjuster and it just lists the actual themes (which obviously includes the current default theme). What do you think?

I think that the default behavior of getTheme() should use the active theme only.

In that way, we prevent switching active themes by default.

OK I agree that Email.php / Mailer.php should default to leaving the theme unchanged.

However I believe the scenario I am discussing is still relevant and should be handled somehow, most likely in an adjuster. The problem is that the admin theme can be active in unexpected scenarios. For example, the developers test that the commerce order receipt is working correctly from within checkout. However then the sales team uses "resend order receipt email" from the orders view within the admin theme and the email loses theming. Similarly the admin theme will be active for simplenews test emails and some but not all user emails.

Most likely the admin theme has no mail CSS and this could be true of other themes too. Before this patch, there is code in DefaultsEmailAdjuster that detects this case and falls back to the default theme instead.

Perhaps there could be a checkbox setting on ThemeEmailAdjuster called something like "fallback": if blank, the theme is changed unconditionally, if ticked, the theme is changed only if the active theme has no mail CSS.

What do you think? It would be nice to cover this case, however I'm willing to defer it to a separate issue if you really aren't interested.

I understand your concern. There is no need to create a separate issue.

We can add an extra option (Active with fallback?) on ThemeEmailAdjuster that will act as you described.

What do you think?

OK that would be great. Thank you very much for your support of this module.

Thanks for joining the discussion @attisan

looking for the combo of easy preset (default) and all possibilities at hand in the config.

Certainly I would agree with that.

I would argue that using the admin theme is never the desired standard

We should consider the case that the admin theme is also a valid page theme. I cannot imagine any reason to add mail CSS to a theme and then assume it would not be used to send emails. I feel that whoever writes code in this way has only themselves to blame.

having the default page theme applied

Applying the default [page] theme is good if a site has only one page theme. If a site has 2 pages theme, then it seems a perfectly reasonable configuration to send emails in the active page theme rather than forcing the default one.

The current proposed preset/default it: if the active theme has mail CSS, then use it; otherwise use the default theme.

As far as I can see, this works correctly for the widest possible set of scenarios where it is reasonably possible to predict the desired behaviour:

• Sending emails when the active theme is an admin theme (with no mail CSS): will use the default.
• Sites with multiple page themes, where both of them have mail CSS: will use the active.
• Sites with multiple page themes, where only one of them has mail CSS: will use the one with mail CSS.
• Sites where the admin theme is also a page theme

Does that make sense?

@balis_m I made some more commits so please do a git pull.

@balis_m. I would like to create a new alpha very soon. I can wait 1-2 days if you would be able to finish this one off. If you have other priorities then I'll make the release now and this can go in the next one.

@AdamPS I've made all the changes that we discussed.

Also, there was a bug on preBuild() call that I fixed.

https://git.drupalcode.org/issue/symfony_mailer-3254125/-/blob/3254125-s...

Sending emails when the active theme is an admin theme (with no mail CSS): will use the default.

This will work when the Active with fallback option is selected on ThemeEmailAdjuster.

Sites with multiple page themes, where both of them have mail CSS: will use the active.

This will work by default (without the ThemeEmailAdjuster).

Sites with multiple page themes, where only one of them has mail CSS: will use the one with mail CSS.

This will work when the Default or any active theme option is selected on ThemeEmailAdjuster.

Sites where the admin theme is also a page theme

This will work by default (without the ThemeEmailAdjuster).

Great many thanks, new code looks good. I made a few minor comments.

Sites with multiple page themes, where only one of them has mail CSS: will use the one with mail CSS.

This would also work when the Active with fallback option is selected on ThemeEmailAdjuster.

Therefore the Active with fallback option works for all the main cases I outlined, which is great. So I feel we could usefully add that option to the default root policy ('_'). What do you think?

I'm not sure that the Active with fallback option should be active by default, because, in that way, we switching themes by default.

I realise that I perhaps missed a part of the comment from @attisan. I had imagined a theme could only have email CSS if the site owner added it. However if this module becomes popular, eventually contrib themes might start to ship with email CSS. In this case, a site owner might install a theme without realising that is contains email CSS that is actually not the desired email CSS. It still seems a strange thing to do for an admin theme, but I guess it could happen.

So we might get the scenario that "the set of desired themes for email" is not equal to "the set of themes that contain email CSS". If there is a single desired theme, then this adjuster works well. If there are multiple desired themes, and also some non-desired themes that nevertheless contain email CSS then it's beyond the scope of what this adjuster allows. However that seems like a pretty subtle complex case, so I think it's OK for to leave it out for now - a site can write their own adjuster/hook of course.

Thanks see answers in the comments.

I'm not sure that the Active with fallback option should be active by default, because, in that way, we switching themes by default.

True, but it's a much softer default - it is explicitly visible in the policy configuration and can be deleted.

I agree with @attisan when he said:

you are usually looking for the combo of easy preset (default) and all possibilities at hand in the config.

.
Without this default policy, then simplenews test mails, commerce resend receipts, etc will likely be wrong, and people can easily miss this case in testing. I feel that more people will be thankful (without evening knowing) that we have this code in than will be upset by it.

I guess we could add one more safeguard: the fallback only occurs if the active theme has no mail CSS and the default theme has some?

I've added a comment here https://git.drupalcode.org/project/symfony_mailer/-/merge_requests/10#no.... We could make a check on DefaultsEmailAdjuster and add the default theme library. But, I don't think that is a good approach because the getTheme() can be used in various ways (ie. hooks, EmailAdjusters) and it's not a good idea to have duplicate code.

The rest of the changes have been made and the Active with fallback option added to the default root policy ('_').

OK I would like to get this checked in, so I hope you don't mind I'm going to change it myself to add the underscores.

I just added the underscores on ThemeEmailAdjuster. It was a really good idea.

Of course, you can make any change you want.

Ah you did it, thanks. I also tried to do it, and I find these new MR in git confusing and I have no idea where my changes went to! Hopefully I didn't create any problem.

@balis_m Many thanks for working on this one.