backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Don't hard-code the line wrapping in backdrop_mail() and _backdrop_wrap_mail_line() #6237

Open alanmels opened 1 year ago

alanmels commented 1 year ago

Description of the need

As reported on https://backdrop.zulipchat.com/#narrow/stream/218635-Backdrop/topic/How.20to.20bypass.20backdrop_wrap_mail.28.29/near/394140550 I tracked the lines are getting wrapped by backdrop_wrap_mail() following really outdated RFC standards and that breaks the format of messages read on modern mail clients.

Proposed solution

@albanycomputers (https://backdrop.zulipchat.com/#narrow/stream/218635-Backdrop/topic/How.20to.20bypass.20backdrop_wrap_mail.28.29/near/394143904):

The RFC Standard for emails are "old" to say the least and other techniques for manipulating emails have been developed, i.e. modern email clients and HTML encoding... etc..

So, I would agree, remove the hard coding and either leave it at that or add an option to wrap at line 80.

Hope this helps.

Alternatives that have been considered

I could not find any way of bypassing this wrapping at about 78 characters when I really need one of the projects to be sending continuous lines, letting the receiving client program do the correct wrapping.


Useful resources

klonos commented 1 year ago

I agree that we shouldn't be hard-coding this. Since this one seems like a nice issue for me to get back on things, I'll grab it and see if I can come up with an elegant solution.

Since we don't have any existing configuration form about emails in core (other than admin/config/people/emails, which is for another, very specific thing), it is perhaps not a good idea to introduce one. I was thinking about a config entry in system.core.json that people can then override via their settings.php file.

alanmels commented 1 year ago

Would be nice, @klonos, as this problem seems to be persisting even after switching to HTML. Maybe _backdrop_wrap_mail_line() should be gone altogether?

bugfolder commented 1 year ago

Since we don't have any existing configuration form about emails in core (other than admin/config/people/emails, which is for another, very specific thing), it is perhaps not a good idea to introduce one...

Actually, I think that is the perfect place for this setting. Put all the existing settings in a single expanded fieldset details element, then add a collapsed "Advanced" box at the bottom that contains this (and any other similar settings).

klonos commented 1 year ago

Actually, I think that is the perfect place for this setting. ...

This email functionality we're looking to change will also affect other things in core (like the sending of security and module update notifications), as well as contrib modules that send out emails. Putting it in a place that only handles emails related to account creation/activation and password reset doesn't seem right to me 🤔 ...it feels like something that would belong under admin/config/system instead.

klonos commented 1 year ago

...looking at the respective code in D11, the code within the wrapMailLine() function has not changed much, but there is an addition at the top of the function to account for mail headers, that shouldn't be split even if they exceed 77 characters in length:

  protected static function wrapMailLine(&$line, $key, $values) {
    $line_is_mime_header = FALSE;
    $mime_headers = [
      'Content-Type',
      'Content-Transfer-Encoding',
      'Content-Disposition',
      'Content-Description',
    ];

    // Do not break MIME headers which could be longer than 77 characters.
    foreach ($mime_headers as $header) {
      if (str_starts_with($line, $header . ': ')) {
        $line_is_mime_header = TRUE;
        break;
      }
    }
    if (!$line_is_mime_header) {
      // Use soft-breaks only for purely quoted or unindented text.
      $line = wordwrap($line, 77 - $values['length'], $values['soft'] ? " \n" : "\n");
    }
    // Break really long words at the maximum width allowed.
    $line = wordwrap($line, 996 - $values['length'], $values['soft'] ? " \n" : "\n", TRUE);
  }

I think that we should include that bit in our fix here.

klonos commented 1 year ago

...this is also relevant: https://www.drupal.org/project/drupal/issues/364670

alanmels commented 1 year ago

@klonos, do you think this will be fixed soon? Or should we completely bypass Backdrop mail functions for the time being as the format of the sent messages look terrible in the receiving mail client?

Screenshot 2023-10-15 at 7 46 32 PM

klonos commented 1 year ago

@alanmels I cannot predict when this may get into core, but since it is a new feature, it will be in one of the minor releases (15th of Ja/May/Sept each year).

argiepiano commented 1 year ago

@alanmels have you considered sending HTML emails instead? You can use Mime Mail for that.

Additionally, if you really want to use plain text emails, perhaps you'll have more luck (and faster features) by adding a feature like the one you mention here, to the contrib Mail System, for example, or even Mime Mail. I haven't looked into this, really... so I don't know if there is a way to overstep the hard wrapping done in core.

I don't know enough about email clients, but isn't there a risk that, when you receive plain text email only, some clients will stop wrapping lines automatically, and rely on the hard-coded line wrapping included in the text?

alanmels commented 1 year ago

@klonos, got you! Thanks.

@argiepiano, we have a specific case when Backdrop needs to send messages to a system that accepts text formats only. So it's not for every mail client, but about our specific use-case. When using HTML format instead, the receiving platform takes in the text version anyway, and the problem in this case is that the links are getting listed in the bottom of the messages, so using MImemail is also not ideal for our use case.

Anyway, nothing urgent, this can wait, I was just wondering about timeframes.