Clarify and simplify message settings

Thanks for fucusing the reasoning and pointing out what this ultimately is: A D7 zombie.

Let's give it a honorable thanks and put it to rest.

Great stab at this! Code looks quite straightforward.

+++ b/src/Plugin/Mail/SwiftMailer.php
@@ -607,13 +532,13 @@ class SwiftMailer implements MailInterface, ContainerFactoryPluginInterface {
+      if (!$is_markup && ($format == SWIFTMAILER_FORMAT_HTML)) {

You hardly ever want to use == in PHP. Use ===.

+++ b/src/Plugin/Mail/SwiftMailer.php
@@ -269,10 +265,14 @@ class SwiftMailer implements MailInterface, ContainerFactoryPluginInterface {
+            $existing_format = preg_match('/.*\;/U', $header_value, $matches) ? trim(substr($matches[0], 0, -1)) : $header_value;

What the heck does this? If it need be that complicated, let's add a code comment. Or is it simply rtrim($header_value, ';')?

First, more or less random thoughts:

Not sure if we should treat this and #3125041: Clarify and simplify message settings as separate issues, as usually you'd use them together. Disable respect format and configure a text format so that all plain mails are converted to basic HTML.

A bit hard to keep it separate when reviewing/discussing. As @AdamPS said in #3125042: [META] New & improved format conversions, this was a valid option to send out simple but decently looking e-mails that did not require custom code or extra modules and a lot of configuration/overrides. I'm pretty sure that there are plenty of sites out there where the user.module e-mail configuration contains huge chunks of HTML, because it works in combination with those settings. And that's not really a security issue for those e-mails because configuring them requires administer users permission anyway. But yes, it might be for others if they put unvalidated user input directly in the mail body.

I'm not sure about the exact attack vectors, since e-mail HTML has to be treated as unsafe anyway as everyone can just send you whatever they want directly anyway. But I suppose you could at least trick site admins into doing things as they might trust the e-mails they receive from Drupal and what they show as links and so on.

If we'd keep the other setting then this would indeed be fine to remove, since the other would create markup objects of everything anyway then. If we want to remove that too it's a bit more complicated IMHO.

I agree that these features and options are tricky, but the problem is that the mail API is pretty weird and only saw very limited changes in D8, so it's a bit too simplified to say it's just a D7 leftover and is fine to remove everything. There are a few cases where core actually does pass through markup objects, for example contact_mail() #2666160: contact_mail() casts rendered markup to string, but most e-mails don't and swiftmailer doesn't actually define the API, so it has to deal with what it gets (or rely on other modules and custom code to do so, but that does seem like a bit of a UX regression?)

I still feel like it would be acceptable in terms of security to make the format setting visible, remove this and have very clear descriptions and instructions in the UI that say that you either need to make sure that the only e-mails that are sent contain only trusted content (which I think is the case for most sites) and that misconfiguring it is not a security issue. Having it in the UI actually gives us a place to explain it, as exposed to people using config import/export or drush to change it. Core isn't different, nothing stops you from granting anonymous users access to the Full HTML format, the only "protection" is the help text that says "Improper text format configuration is a security risk. Learn more on the Filter module help page." on /admin/config/content/formats.

Yeah, I missed that this would make the setting always FALSE. But wouldn't that mean that it would be impossible to *not* send some mails als plain text and some as HTML? I'm not sure if that should really be the goal here, I'm sure some prefer to only send plain text HTML mails, also because sending out good looking HTML mails takes time.

Per your own suggestion, I would propose that we first have real-life functional tests at least for contact and user e-mails, to see what exactly is being sent with common setting combinations for format, respect format, and filter format (plain text | basic html)? And it might also be a good idea to include an integration test with simplenews, which has per-newsletter settings to send or not send HTML mails, removing the respect format setting would break those configuration settings?

> But wouldn't that mean that it would be impossible to *not* send some mails als plain text and some as HTML?

Hmm, good point. Re-thinking about this.

Nice!

I did not thoroughly review it yet, but the configuration sounds more intuitive really. Will look at this later again.
And yes, this should not block other patches for too long.

We have a task to try it out in our project, but didn't get to it yet. Hectic times :) I'll try to have a quick look at least over the weekend.

When that patch was uploaded, i thoroughly reviewed it.
I thought a can test it soon-ish and give feedback then, but then also got hectic times.

So let's at least give feedback on the code: I like it code-wise as a big win.
So when someone did a manual test, let's get it in and build on it.

I've been testing this and 2.x in general a bit over the last days and it seems to work well.