Bugs with format conversion

#3 thanks @Berdir

You need to allow *some* HTML, for example links.

Sure - I agree that it's good to call a text formatter and it can contain the filter "Convert URLs into links" which I think plain_text does by default.

My objections are related to the details of how we do it.

plain text isn't the only safe format, you can't inject javascript into e.g. basic_html.

I agree that a default D8 clean install of a certain profile creates a text format called basic_html that is safe. In other situations is might be completely different. My point is that Drupal states that we need to check access before using a text format but we aren't.

#4

So no issue for swiftmailer. (Or am i missing something?)

Oops I didn't explain well then. Unfortunately yes I have not enabled you to understand my point so I have tried to improve the IS. My point is that in SwiftMailer::massageMessageBody() this module is making its own call to a formatter and I believe it's not doing it correctly.

The standard case of which is using the formatter text datatype, which handles all permission and access issues.

Agree

If people want to have formatted user mails, provide an implementation for this (i'd recommend easy_email).

Yes fine.

#3 basic_html might be safe but it would likely be a pretty bad idea. It would mean that the contact form would effectively become HTML but the person typing a message wouldn't realise that for example they needed to type & instead of &.

#8 Sure we definitely don't want to break sites.

For example, convert URL's doesn't help if you want to do something like click here, it only converts raw URL's to links.

Well if you have an actual link then your content is already HTML so it doesn't need to be converted. Provided that your $body implements MarkupInterface then it will work fine. If not then the HTML will get escaped. This aspect seems out-of-scope for this issue because it worked fine before and after #2831959: HTML in Drupal core emails and I'm not proposing to alter it.

I have improved the IS again.

Here is the commit from #2831959: HTML in Drupal core emails. It adds some code relating to the scenario that the target format is HTML and the content contains no tags.

On the other hand #8 as far as I understand is discussing the case if you put HTML links in the user register email so the content will contain tags. #2831959 didn't change that case.

However I agree that this is a desirable scenario for to cover. If $body implements MarkupInterface the HTML will remain in (however I suspect that's not true for user register mails). If not the HTML will be converted to text which is pretty strange if the target format is HTML.

So I think what you are asking for is a good idea, it's currently broken and this issue can help fix it.

@geek-merlin very funny and very true.

We are starting a new 2.x branch and it's the ideal time to remove some legacy dead wood from the module. There is currently some black magic with little explanation in the UI or code. There will likely be some resistance but in the end hopefully people will agree that the overall system is clearer, more reliable and easier to use.

People might say to us "who are these crazy new guys to force users of this module in a certain direction". Well firstly we are offering to maintain it and no one else was. Secondly to those people, please comment on this issue now - we are very open to listen. Or if you don't see this issue in time, you can raise issues later and we can make it better.

I don't think we necessarily have to be entirely against conversions. For example, it could be OK for this module to have a setting "convert all mails to HTML". In this case we would take mails that are plain text plus are labelled as plain text and then convert them to mails that are HTML plus labelled as HTML.

Added related issue #2893606: Generating HTML emails where people are getting caught out by exactly this code.

Updated the IS to clarify what I think we are trying to do here.

OK, I have checked in more detail. Almost all of the problems come from when "Respect provided e-mail format" is set to FALSE. So I have made this issue specifically about that and will create a separate one for the other problem.

Given this is an optional setting then the situation is more easily managed. We can add a warning about the problems and if people choose to continue using it they accept the downsides.

See updated IS.

Here is a patch that hopefully finds a balance between back-compatibility and correctness. It feels that we must warn about the downsides and ensure that there is a way for sites to avoid them.

OK, finally now I think I get it. Come on everyone please tell me if I am on track. I have spent quite a few hours studying carefully but I wasn't involved in all the history so please correct me if needs be.

Especially @berdir thank you for the earlier comments and if you are still following please tell me what you think now.

I will post again in the issues that I propose to back out to give people to chance to object.

Thanks both for some feedback. It has been interesting trying to figure this one out.

Regarding security yes you are right and I have mostly stopped worrying it. It's not mentioned in the IS any more.

Regarding "non MarkupInterface strings with html" I believe the current code will get HTML-escaped them (in the plain_text format) so they are already not working.

Regarding using "Basic HTML" yes I thought that too for a while and other people on other issues have suggested it. But I now think that's a wrong choice. The chosen format will be called with plain text that needs to be encoded and it's only called if there are no tags. Hence my point that we need to document things:-).

Testing for markup using strip_tags() is unreliable and it can corrupt messages. Any plain-text message that contains $lt; followed by a letter will be counted as markup and therefore corrupted. Any markup without a tag won't count, even if it contains & so any entities will be double-encoded. There's not really any way a site can prevent the corruption as it's built into swiftmailer. To me this doesn't seem good enough and that's what I propose we need to fix.

Starting 2.x seems like a chance to take a new course and get things right. Ideally any corrective processing to be at the application level - i.e. specific to the module/key. Module A always needs us to apply the default text format, so we apply it specifically there. However I'm open to persuasion on that count. In the IS search for "pragmatic reasons" where I have already mentioned that as a valid compromise.

Regarding "non MarkupInterface strings with html" I believe the current code will get HTML-escaped them (in the plain_text format) so they are already not working.

Sorry not in the plain_text format. It looks like they will be passed to MailFormatHelper::htmlToText($body) in dev or to Html::escape() in the beta. Either way they won't stay as markup.

There will be many normal cases where this module is passed "non MarkupInterface strings containing legitimate plain text that happens to contain a < . Surely we have to run HTML escaping in this case?

OK I have a plan. Let's fix the clear bugs first. Then we can consider the more subjective, non-BC changes later. I updated the IS for that.

@Berdir sure we can give plenty of time for review - in the meantime there are other issues to fix.

It took me a while to figure things out, but hopefully now I have and no more changing things. There is no need to read all my past comments. The IS is up-to-date and hopefully quite clear and concise. The proposed new code is hopefully simple and logical.

I have now split the "secondary problems" into two separate issues - #3125041: Clarify and simplify message settings and #3125042: [META] New & improved format conversions. It is those that are potentially controversial and we should not rush in to. We need to give everyone a chance to understand and to comment.

PLEASE FOLLOW AND COMMENT ON THOSE ISSUES

With those parts removed this issue is a simple bug fix. I have reduced the priority to normal. There are some cases where conversions are done incorrectly - mostly HTML messages that look like plain text and vice versa. This issue is fully BC and it you look at should be entirely uncontroversial: look at the changes to tests (patch coming in next comment) and hopefully it's entirely obvious that the new version is better than the old.

OK, it would be great if we could review then commit this one as a priority please. Once we have done that I can make another alpha release that I believe is good enough to use on live sites and I will start testing with it. (It's an alpha release not an RC because we haven't finished making non-BC changes yet).

As per previous comment this issue is now fully BC and just fixes some bugs.

Please ignore the interdiff - it got broken because of another large commit in between. The new patch just adds the test changes.

Great thanks.