Add new email adjuster to inline CSS attached to tokens

Attached is a patch against 1.x-dev.

Doh, wanted to set this to Needs review, of course.

The last submitted patch, 2: 3372281-02-tests-only.patch, failed testing. View results →

Great thanks for the issue and patch. It's an interesting case. As far as I can see, the patch works like this:

1. The site admin using the GUI has responsibility for choosing whether or not to add token CSS
2. They have a single choice yes/no which applies to all of the tokens.

I'm wondering if these are always going to be the right choices. Maybe the choice is not so much site-dependent - for each CSS library either it is suitable for email or not?? Maybe a developer or CSS expert is the best one to decide, so the choice should be made in code rather than config (there could be a hook)?? Maybe we need to be able to choose per library??

Do you have an example where it would be bad to add the CSS library to a mail?

Maybe we need to be able to choose per library??

It would probably be best if you were able to choose this per library, but then it also gets complicated. With the current implementation, you can just add the processor and if the CSS fits, which you can simply test, you're done. If not, you can choose to not use the processor and instead add to your own mail css library or just keep the plain output of the token without the library. I think in at least 80% of the cases, this yes/no choice per mailer policy should be enough. Everything else would either require more complicated configuration that might involve reading code (so I guess we don't want to do that) or you have some kind of hook or event that you need to re-write for every use case. If Symfony Mailer gained more traction or was merged into core, then I could imagine that this might be an additional key in the library definition or something like that. But for now, I don't think implementing a hook for that would make much sense. Then I could also straight-away implement a hook to inline that additional library for that type of mail policy.

Do you have an example where it would be bad to add the CSS library to a mail?

Not really, no. I just know that often you still need some hacks to support certain mail clients, especially if the markup or css rules get more complex. I think w/r/t email we're somehow still stuck in the 90s, so to say. In my opinion, developing an email template for a newsletter is still a lot like web development used to be before Mozilla Firebird/fox became a thing and the browser wars ended. My assumption was that CSS attached to tokens is most likely optimized for display in modern browsers and might therefore not necessarily be suitable for inclusion by default, so you probably wanted some kind of choice as a site builder.

>> Do you have an example where it would be bad to add the CSS library to a mail?
> Not really, no

I'm concerned this patch is moderately complex and it's speculative if it's the right fix. We have one example where the CSS is good (the issue you linked to) and zero examples where it's bad. Maybe instead we should always add the token CSS; maybe a hook to control it; maybe a GUI field to blacklist libraries; maybe as you say a key in the library definition, which could be added in for existing libraries using an alter.

Secondly I am concerned that this adjuster isn't well-suited to GUI admins. The other adjusters in this module so far are directly linked to parts of the email and underlying email interface. The admins might look down the list of adjusters and are confused by this one (what is this Token CSS?) or when actually they would benefit from the token CSS, they don't understand the situation enough in order to enable it.

Here's my proposal. The base module should contain enough to allow people to write the adjusters or hooks. The rest we postpone for now - or if you are willing to start the "extra" module (which I feel could be very minimally maintained yet still useful) then it could go there.

In TokenProcessorTrait we can put token libraries from the body (probably no point for the subject) somewhere for other code to find them, and optionally call addLibrary(). However I'm not sure where we put them. setParam() is intended for the parameters chosen by the code that sends the email (maybe it would even be removed in 📌 Improvement to email sending interface Needs work ). setVariable() is intended for twig template variables. Neither one is currently allowed after rendering. I guess we could add set()/get() of arbitrary properties like on FormStateInterface which also has get/set of values.

Maybe instead we should always add the token CSS

That would probably work for me. If you'd be willing to take it, I could write a patch for that. I'd just have to double check on a few systems first, maybe there actually is an example for a "bad" token library hidden somewhere in one of them...

if you are willing to start the "extra" module (which I feel could be very minimally maintained yet still useful) then it could go there.

As I said in the other issue, I'll seriously think about that, however I guess that would still require some API changes here, because we'd still need some way to access either the bubbleable metadata or the token libraries.

In TokenProcessorTrait we can put token libraries from the body

I agree that we don't need this for the subject. Body should be enough.

I was also unsure about how to best pass on the data, that's why I finally took the approach of adding the libraries directly in TokenTrait, because I didn't want to change the scope of the other methods and adding additional get/set methods to the interface just for this seemed like being a little bit too much. There is also still the issue of token replacement in mail addresses (that's eventually going to be another issue/patch ;). For now I have added that to AddressAdjusterBase, but ideally I'd need a way to access the token data (and token options) there to make this work nicely, which I can't... I didn't yet figure out how to best implement that as well.

Maybe having the possibility to add arbitrary metadata to the email object is a good idea. If we can't just always add the token libraries for some reason, then I'd suggest we look into that further.

There is also still the issue of token replacement in mail addresses

See also #3312071: Replace tokens in Plain text alternative → .

Maybe having the possibility to add arbitrary metadata to the email object is a good idea.

I've thought some more, and I suggest we use setParam() - we can remove the restriction that it is only valid during initialisation.

I guess that would still require some API changes here, because we'd still need some way to access either the bubbleable metadata or the token libraries.

That's exactly what I was thinking and I propose we do it in this issue. We can do $email->setParam('token_libraries', XXX). Then it's possible for custom code to do any of the "maybe" solutions I described, e.g. a hook that always adds the CSS or an adjuster that selectively adds it.

>> Maybe instead we should always add the token CSS
> That would probably work for me. If you'd be willing to take it, I could write a patch for that.
I mean that it might be the right solution - however we don't have much data on how well the token CSS works in email. In the worst case this could break email formatting on people's sites.

In the long run I think it's the right direction. On my sites we attach the entire theme CSS onto emails with just a few modifications (for example for .clearfix and floating images). We could even have code in ad adjuster to map arbitrary CSS and make it work better for email.