714724 - correctly encode emails as quoted-printable

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement)

Description

[:glob ✱]

a lot of email encoding issues will be resolved if we encoded all utf8 emails as 8bit instead of quoted-printable.

we'd be able to revert the change on bug 696005, which caused bug 714488.

bug 714488 exposed an issue where you can't add an attachment with an extension if you set the encoding header before calling hooks, however you can't retrieve the email body from an extension if you set the encoding header after calling hooks.

Comment 1

[:glob ✱]

Attachment: patch v1

while 8bitmime isn't supported by every mta, http://en.wikipedia.org/wiki/8BITMIME#8BITMIME indicates that it should be ok do make this switch now.

Comment 2

[:glob ✱]

Attachment: patch v2

i forgot to revert bug 696005

Comment 3

[Max Kanat-Alexander]

Comment on attachment 587623 [details] [diff] [review]
patch v2

Review of attachment 587623 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, I think instead of doing utf8::encode, we want to actually pass things in as utf8-tagged strings. I'm pretty sure Email::Send and all those guys work okay with that, no? So we would want to be doing utf8::decode if !utf8::is_utf8.

Comment 4

[:glob ✱]

Attachment: patch v3

if the utf8 param is enabled, then the email body will already be a utf8 tagged string, so we don't need to do anything special.

Comment 5

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Mailer.pm
Committed revision 8093.

Comment 6

[:glob ✱]

after committing this i found a problem with Email::MIME, backing out:

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Mailer.pm
Committed revision 8094.

Email::Send is happy to deliver emails with a body which is a perl utf8 string, however Email::MIME assumes the body is an encoded string, and will also decode it when you call body_str(), which fails.  you can trigger this by calling $args->{email}->body_str() in a mailer_before_send hook.

it looks like we'll have to go with patch v2.

Comment 7

[:glob ✱]

Comment on attachment 587623 [details] [diff] [review]
patch v2

re-requesting review given patch3's failure.

Comment 8

[Max Kanat-Alexander]

Is this a bug in Email::MIME, or a bug with how we're using it? If it's a bug in Email::MIME, we should report it to rjbs.

Comment 9

[:glob ✱]

Attachment: patch v5

i think we're using it wrong.

the crux of the problem is we're building a message with utf8 strings, but this isn't reflected in the headers we're passing to Email::MIME.  we're fixing things up with the walk_parts block, but that's after the fact; if you try to manipulate the message in some ways it's already too late.

here's an alternative patch which removes the walk_parts kludge, and instead updates all template and code which generate email to include the appropriate headers before passing it to MessageToMTA.

Comment 10

[Frédéric Buclin]

Comment on attachment 593009 [details] [diff] [review]
patch v5

>=== modified file 'Bugzilla/Mailer.pm'

>@@ -152,24 +152,7 @@

patching file Bugzilla/Mailer.pm
patch: **** malformed patch at line 84: 

That's because the header must be +152,6.

Anyway, with your patch applied, the output is now correct when using the "Test" method for the mail_delivery_method parameter while it was garbage previously. But when using SMTP and smtp.gmail.com as server, the email is still mangled.

Comment 11

[:glob ✱]

(In reply to Frédéric Buclin from comment #10)
> Anyway, with your patch applied, the output is now correct when using the
> "Test" method for the mail_delivery_method parameter while it was garbage
> previously. But when using SMTP and smtp.gmail.com as server, the email is
> still mangled.

works for me; can you please provide a dump of your mangled email so i can try to reproduce.

Comment 12

[Frédéric Buclin]

The problem only occurs with text-only emails. Text+HTML emails are fine. Did you try that? I can forward you an email if you want to; I don't want to attach it here or in any other public place.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 593009 [details] [diff] [review]
patch v5

Review of attachment 593009 [details] [diff] [review]:
-----------------------------------------------------------------

::: template/en/default/whine/multipart-mime.txt.tmpl
@@ +46,5 @@
>  --[% boundary %]
> +[% IF Param('utf8') %]
> +Content-Type: [% part.type %]; charset="UTF-8"
> +Content-Transfer-Encoding: 8bit
> +[% ELSE %]

I really do think it's more appropriate to be adding these inside of Bugzilla::Mailer as opposed to adding them everywhere and expecting everybody who ever writes an email template to remember them.

Comment 14

[:glob ✱]

(In reply to Max Kanat-Alexander from comment #13)
> I really do think it's more appropriate to be adding these inside of
> Bugzilla::Mailer as opposed to adding them everywhere and expecting
> everybody who ever writes an email template to remember them.

i agree that it would be more convenient, but it would be significantly more work, and more fragile.

the content-type and content-transfer-encoding needs to be set correctly before parsing the message with Email::MIME.  with your suggestion we'd have to parse a mime payload to insert these headers without using Email::MIME.

Comment 15

[Max Kanat-Alexander]

(In reply to Byron Jones ‹:glob› from comment #14)
> the content-type and content-transfer-encoding needs to be set correctly
> before parsing the message with Email::MIME.  with your suggestion we'd have
> to parse a mime payload to insert these headers without using Email::MIME.

  Hmm. Are you sure there isn't a way to change the settings of Email::MIME (or use some different method of Email::MIME, or use some additional CPAN module) so that we could do this safely? We should be able to safely assume that all input will be 8bit.

Comment 16

[:glob ✱]

Attachment: patch v7

here's another approach to attack this problem.

this sub-classes Email:MIME to ensure the headers are correct before the body is set.

Comment 17

[Max Kanat-Alexander]

Comment on attachment 610496 [details] [diff] [review]
patch v7

Review of attachment 610496 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty awesome. :-)

::: Bugzilla/MIME.pm
@@ +15,5 @@
> +    my $class = shift;
> +    my $self = $class->SUPER::new(@_);
> +
> +    # Force Email::MIME to re-create all the parts.  Without this as_string()
> +    # doesn't return the updated headers for multi-part sub-parts.

Is this a bug? Should it be fixed upstream?

@@ +31,5 @@
> +
> +    if (Bugzilla->params->{'utf8'}) {
> +        $self->header_set('Content-Transfer-Encoding' => '8bit');
> +        $self->charset_set('UTF-8');
> +    }

Will this also affect attachments that might not be UTF-8 in the future? It's not so important now, but good to think about.

Comment 18

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #17)
> Will this also affect attachments that might not be UTF-8 in the future?
> It's not so important now, but good to think about.

What's the relationship between bugmails and attachments? Attachments are not attached to emails.

Comment 19

[:glob ✱]

(In reply to Max Kanat-Alexander from comment #17)
> Comment on attachment 610496 [details] [diff] [review]
> > +    # Force Email::MIME to re-create all the parts.  Without this as_string()
> > +    # doesn't return the updated headers for multi-part sub-parts.
> 
> Is this a bug? Should it be fixed upstream?

no, i don't think it's a bug.  we're kinda doing things behind email::mime's back.

> > +    if (Bugzilla->params->{'utf8'}) {
> > +        $self->header_set('Content-Transfer-Encoding' => '8bit');
> > +        $self->charset_set('UTF-8');
> > +    }
> 
> Will this also affect attachments that might not be UTF-8 in the future?
> It's not so important now, but good to think about.

good point -- we should only add the headers if they aren't already present.  will update the patch.

(In reply to Frédéric Buclin from comment #18)
> (In reply to Max Kanat-Alexander from comment #17)
> > Will this also affect attachments that might not be UTF-8 in the future?
> > It's not so important now, but good to think about.
> 
> What's the relationship between bugmails and attachments? Attachments are
> not attached to emails.

it's possible for extensions to send emails with attachments.

Comment 20

[:glob ✱]

Attachment: patch v8

Comment 21

[Frédéric Buclin]

Comment on attachment 613601 [details] [diff] [review]
patch v8

>=== added file 'Bugzilla/MIME.pm'

>+        my $ct = parse_content_type($self->content_type);
>+        my $charset = $ct->{attributes}{charset};
>+        if (!$charset || $charset eq 'us-ascii') {
>+            $self->charset_set('UTF-8');
>+        }

I expected this patch to remove this ugly hack about us-ascii. :(

Comment 22

[:glob ✱]

(In reply to Frédéric Buclin from comment #21)
> 
> I expected this patch to remove this ugly hack about us-ascii. :(

it isn't a hack; if you don't set a charset then Email::MIME sets it to us-ascii for text/plain and text/html content-types.

because we don't set a charset, that's what is returned for the majority of our parts.

Comment 23

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #22)
> it isn't a hack; if you don't set a charset then Email::MIME sets it to
> us-ascii for text/plain and text/html content-types.

I know where us-ascii comes from, and I'm not blaming you. I thought we could avoid the problem described in bug 723944 comment 8 with your patch.

Comment 24

[:glob ✱]

Comment on attachment 613601 [details] [diff] [review]
patch v8

found a problem -- triggers "Wide character in print at Bugzilla/Send/Sendmail.pm line 34."

Comment 25

[Frédéric Buclin]

glob: any progress on this bug?

Comment 26

[:glob ✱]

note to self: also need to fix whine.pl

Comment 27

[:glob ✱]

Attachment: patch v9

fixes the "Wide character in print" error, and updates whine to use the MIME modules instead of hand rolling multipart email.

Comment 28

[:glob ✱]

Comment on attachment 658519 [details] [diff] [review]
patch v9

due to the 1000 char maximum length of lines in email, we should be using binary, not 8bit (see rfc 1341).

we should always use this encoding, not just when utf8 is enabled.

Comment 29

[:glob ✱]

Attachment: patch v10

switch from 8bit to binary encoding.

Comment 30

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #29)
> switch from 8bit to binary encoding.

This means the source code of the email will become unreadable, right?

Comment 31

[Reed Loden [:reed]]

http://en.kioskea.net/contents/courrier-electronique/mime.php3 specifically calls out binary as "binary format; not recommended." Just saying... ;)

Comment 32

[Reed Loden [:reed]]

Also, for binary to work correctly, the SMTP server has to support it (RFC 3030), as per https://en.wikipedia.org/wiki/MIME#Content-Transfer-Encoding -- so, I worry that this may not be a good idea depending on the level of support in SMTP servers.

Make sure you read RFC 2045 as well.

https://social.technet.microsoft.com/Forums/en/winservergen/thread/cc97a407-ecf3-43e3-9389-d355f2ac1ebe

Comment 33

[:glob ✱]

(In reply to Frédéric Buclin from comment #30)
> This means the source code of the email will become unreadable, right?

nope, it's as readable as 8bit encoding.

(In reply to Reed Loden [:reed] from comment #31)
> http://en.kioskea.net/contents/courrier-electronique/mime.php3 specifically
> calls out binary as "binary format; not recommended." Just saying... ;)

:(

> Make sure you read RFC 2045 as well.

the tl;dr is "in 1996 not many systems supported binary transfer encoding"

> https://social.technet.microsoft.com/Forums/en/winservergen/thread/cc97a407-ecf3-43e3-9389-d355f2ac1ebe

the takeaway here is "iis6 on windows server 2003 doesn't support binary transfer encoding", which is a problem :(  (fwiw i didn't encounter any issues during testing, but that's sendmail and gmail).


the lack of support from the ms camp puts a halt on that change, although 8bit support isn't universal either.


what are the thoughts on staying on quoted-printable encoding, along with the other changes in the latest patch?  it'll simplify extensions which modify bugmail, and we don't risk running into issues with mta's which don't support 8bit transfer encoding.

Comment 34

[Frédéric Buclin]

Comment on attachment 660736 [details] [diff] [review]
patch v10

r- due to the lack of support for binary, see previous comments.

Comment 35

[Frédéric Buclin]

We are now late in the 4.4 dev cycle, and I don't want to take the risk to break bugmails on this branch -> 5.0.

Comment 36

[:glob ✱]

(In reply to Byron Jones ‹:glob› from comment #33)
> what are the thoughts on staying on quoted-printable encoding, along with
> the other changes in the latest patch?  it'll simplify extensions which
> modify bugmail, and we don't risk running into issues with mta's which don't
> support 8bit transfer encoding.

no news is good news; i'll do this.

Comment 45

[:glob ✱]

Attachment: patch for master, v11

- create Bugzilla::MIME which subclasses Email::MIME and centralises our email related fixes
- updates whine.pl to use generate_email instead of hand-rolling mime multipart

encoding fixes:
- when reading a template treat the content as binary utf-8 before parsing with email::mime
- use body_str not body when setting the body text during multi-part email assembly
- ensure the charset and encoding is always set on single part emails

lpsolit - asking you for a review because i know you've hit encoding issues in the past, so hopefully you're in a position to test and review.  let me know if this isn't suitable and i'll find someone else to reivew.

Comment 47

[Frédéric Buclin]

I cannot reproduce encoding problems using SMTP, so it's a bit hard for me to check if your patch fixes these problems.

Comment 48

[Frédéric Buclin]

Comment on attachment 8610382 [details] [diff] [review]
patch for master, v11

Thunderbird is displaying all emails correctly with and without your patch. But when looking at the source code of emails, some characters weren't displayed correctly without your patch. I also tested the whining system with your patch, and emails are correctly generated and displayed.

I like the cleanup you did in both Bugzilla/Mailer.pm and whine.pl. :)

I don't know the internals of Email::MIME well enough to be 100% confident with the code you put in as_string(). As long as this doesn't break stuff, I'm fine with that.


r=LpSolit, but a 2nd review would probably be welcome, unless you are sure about what you did.

Comment 49

[Frédéric Buclin]

Do we want this fix for 5.0.1?

Comment 51

[Simon Green]

(In reply to Frédéric Buclin from comment #49)
> Do we want this fix for 5.0.1?

Yes.

Comment 52

[:glob ✱]

thorsten, is it possible for you to check that this patch addresses your issues from bug 1182004?

Comment 53

[Thorsten Schöning]

I'm not that familiar with git, so maybe did something wrong, but this patch didn't change anything for me. Here's what I did: Download, scp to my server, "git apply --check" aborted with errors, "git apply --3way" succeeded but printed errors and conflicts as well:

> error: Anwendung des Patches fehlgeschlagen: Bugzilla/Mailer.pm:17
> Falling back to three-way merge...
> Applied patch to 'Bugzilla/Mailer.pm' with conflicts.
> 714724_11.patch:343: new blank line at EOF.
> +
> U Bugzilla/Mailer.pm
> warning: 1 Zeile fügt Whitespace-Fehler hinzu.

I resolved the conflict in Mailer.pm using "git checkout --theirs" which resulted in added code with some EMAIL_LIMIT_* constants not available/imported properly:

> Bareword "EMAIL_LIMIT_PER_MINUTE" not allowed while "strict subs" in use at Bugzilla/Mailer.pm line 123.

I changed all of them to simply be "1", which shouldn't harm because I don't use mailer queue, but local sendmail instead. Because I got those errors at all I think I applied properly, but in that case the patch didn't change anything. The charset is still removed "somewhere".

> Content-Type: multipart/alternative; boundary="1439973522.bbA70ebb0.8876";
>  charset="UTF-8"

The newline is already there in the mail.

> MIME-Version: 1.0
> Content-Type: text/plain

Comment 54

[Thorsten Schöning]

To be honest it looks to me that things got worse now: I've debugged MessageToMTA like in my former bug report:

>     my $dbh = Bugzilla->dbh;
> use Data::Dumper;
> warn("msg: " . $msg->debug_structure());
>     my $email = ref($msg) ? $msg : Bugzilla::MIME->new($msg);

This produces the following output:

> msg: + multipart/alternative; boundary="1439975931.3AAfdAF0.11840"; charset="UTF-8"
>           + text/plain, referer: 
>           + text/html, referer: 

while the in my original bug post the output was as follows:

> process_bug.cgi: + multipart/alternative; boundary="1436433767.7a285e0.19230"; charset="UTF-8", referer:
> process_bug.cgi:      + text/plain; charset="UTF-8", referer:
> process_bug.cgi:      + text/html; charset="UTF-8", referer:

The charset per MIME part is already missing.

Comment 55

[Frédéric Buclin]

(In reply to Simon Green from comment #51)
> > Do we want this fix for 5.0.1?
> 
> Yes.

I just asked if we wanted this patch for 5.0. I didn't say it applied to 5.0. And in fact, it doesn't apply at all due to several conflicts in Mailer.pm. So a backport is needed first. :)

Comment 56

[Frédéric Buclin]

(In reply to Thorsten Schöning from comment #54)
> To be honest it looks to me that things got worse now

Could it be because the patch hasn't been applied correctly? I definitely see the correct content type for all parts with this patch, but I tested with 5.1, not 5.0. A big difference between 5.0 and 5.1 is bug 1105568, which landed in 5.1 only.

Comment 57

[Thorsten Schöning]

Of course I may have made a mistake, but the emails get sent, it's very unlikely that I didn't resolve the conflict properly but left Mailer.pm in a functional state. Which charset is printed in your Content-Type-headers? I think that's the important part for my problem.

Meanwhile I debugged a bit further and found that BugMail::_generate_bugmail creates the mails, but without setting any charset. From my understanding that was done in Mailer::MessageToMTA before the patch using the walk_parts-part, which has now been removed by the patch. So how is this supposed to work at all, where comes the charset into play? From my understanding of MIME it's not done there, because a content type is present at all, only the charset is missing, and the mails are multipart, but as_string is only checking for not multipart mails.

Comment 58

[Frédéric Buclin]

With this patch, the text+HTML emails have:

MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

MIME-Version: 1.0
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable


And the text-only emails have:

MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable


Note that the patch changes code in Bugzilla::Mailer::generate_mail(). In Bugzilla 5.0, this code was in Bugzilla::Bugmail::_generate_bugmail(). Also, this patch moves some code in Bugzilla/MIME.pm.

Comment 59

[Thorsten Schöning]

I really think my problem has nothing to do with this bug and the changes in this bug won't help me: I removed your patches, went back to release-5.0-stable and added debug statements in Bugzilla::Sender::Transport::Sendmail::send_email:

> use Data::Dumper;
> warn('Sendmail1: '.Dumper($email));
>     my $string = $email->as_string;
>     $string =~ s/\x0D\x0A/\x0A/g unless $^O eq 'MSWin32';
> warn("Sendmail2: $string");

Some output for the Dumper-line, which to me makes clear that the charset is still available:

>   'header' => bless( {
>                        'mycrlf' => '\r\n',
>                        'headers' => [
>                                       'Date',
>                                       [
>                                         'Wed, 19 Aug 2015 17:43:43 +0200',
>                                         'Date: Wed, 19 Aug 2015 17:43:43 +0200'
>                                       ],
>                                       'MIME-Version',
>                                       [
>                                         '1.0',
>                                         'MIME-Version: 1.0'
>                                       ],
>                                       'Content-Type',
>                                       'text/html; charset="UTF-8"',
>                                       'Content-Transfer-Encoding',
>                                       'quoted-printable'
>                                     ]
>                      }, 'Email::MIME::Header' ),
>   'ct' => {
>             'subtype' => 'html',
>             'attributes' => {
>                               'charset' => 'UTF-8'
>                             },
>             'composite' => 'html',
>             'discrete' => 'text',
>             'type' => 'text'
>           }
> }, 'Email::MIME' )

And after as_string is called the charset is missing for any reason:

> --1439999023.d7cB0.31706
> Date: Wed, 19 Aug 2015 17:43:43 +0200
> MIME-Version: 1.0
> Content-Type: text/html
>
> <html>
>     <head>
>       <base href="http://bugzilla.potsdam.am-soft.de/">
>     </head>
>     <body>
>       <p>
>         <div>
>             <b><a class="bz_bug_link
>           bz_status_ASSIGNED "
>    title="ASSIGNED - Testfehler"
>    href="http://bugzilla.potsdam.am-soft.de/show_bug.cgi?id=243#c257">Comment # 257</a>
>               on <a class="bz_bug_link
>           bz_status_ASSIGNED "
>    title="ASSIGNED - Testfehler"
>    href="http://bugzilla.potsdam.am-soft.de/show_bug.cgi?id=243">bug 243</a>
>               from <span class="vcard"><a class="email" href="mailto:tschoening&#64;am-soft.de" title="Thorsten Sch\xc3\xb6ning &lt;tschoening&#64;am-soft.de&gt;"> <span class="fn">Thorsten Sch\xc3\xb6ning</span></a>
> </span></b>
>         <pre>m\xc3\xb6\xc3\xb6p</pre>
>         </div>
>       </p>
>
>
>       <hr>
>       <span>You are receiving this mail because:</span>
>
>       <ul>
>           <li>You reported the bug.</li>
>           <li>You are the QA Contact for the bug.</li>
>           <li>You are the assignee for the bug.</li>
>       </ul>
>     </body>
> </html>
> --1439999023.d7cB0.31706--

I suggest to reopen my original bug instead...

Comment 60

[Frédéric Buclin]

Can you reproduce the issue if you use SMTP instead of sendmail?

Comment 61

[Thorsten Schöning]

(In reply to Frédéric Buclin from comment #58)
> Note that the patch changes code in Bugzilla::Mailer::generate_mail(). In
> Bugzilla 5.0, this code was in Bugzilla::Bugmail::_generate_bugmail(). Also,
> this patch moves some code in Bugzilla/MIME.pm.

I think I got it now: I can't find BugMail.pm mentioned in the patch. I fthis isn't changed, it will still use it's own _generate_bugmail method which created Email::MIME objects and forward those to Mailer::MessageToMTA, where the following line is:

>     my $email = ref($msg) ? $msg : Bugzilla::MIME->new($msg);

"ref($msg)" is true in my case, becaue I already have Email::MIME objects, and neither Bugzilla::MIME nor Mailer::generate_email are ever used. I already had debug statements in both and none got called, but I wans't sure why until now...

Comment 62

[Frédéric Buclin]

(In reply to Thorsten Schöning from comment #61)
> I think I got it now: I can't find BugMail.pm mentioned in the patch.

Sure, you won't find it in the patch as this code is no longer in BugMail.pm in 5.1. :)

You could either apply these changes manually in BugMail.pm, or first apply the patch from bug 1105568 which should let you apply this patch correctly, without any conflict.

Comment 63

[Thorsten Schöning]

(In reply to Frédéric Buclin from comment #60)
> Can you reproduce the issue if you use SMTP instead of sendmail?

Yes.

Comment 64

[Frédéric Buclin]

Comment on attachment 8610382 [details] [diff] [review]
patch for master, v11

>+++ b/Bugzilla/Mailer.pm

> use Encode qw(encode);
> use Encode::MIME::Header;
>-use Email::MIME;

I just realized that both Encode and Encode::MIME::Header should be moved into Bugzilla/MIME.pm. You copied Encode, but forgot to also copy (ideally move) Encode::MIME::Header there. Please fix that on checkin.

Comment 65

[Frédéric Buclin]

Attachment: patch for 5.0, v1

I backported glob's patch to 5.0 to make life easier to those who want to test it against 5.0. Note that I didn't test it at all. Thorsten, does it help you? :)

Comment 66

[Thorsten Schöning]

(In reply to Frédéric Buclin from comment #62)
> You could either apply these changes manually in BugMail.pm, or first apply
> the patch from bug 1105568 which should let you apply this patch correctly,
> without any conflict.

Neither of the patches applied cleanly, I needed to "git checkout --theirs" for both and again fix the EMAIL_LIMIT_* problems in Mailer.pm, BUT in the end I have my charset again in the subparts, along with all the other headers from Bugzilla::MIME like "Auto-Submitted":

> --1440007338.628E40.6909
> Date: Wed, 19 Aug 2015 20:02:18 +0200
> MIME-Version: 1.0
> Content-Type: text/plain; charset="UTF-8"
> Content-Transfer-Encoding: quoted-printable
> X-Bugzilla-URL: [...]
> Auto-Submitted: auto-generated

Looks to me like your Bugzilla::MIME::as_string overrides whatever is responsible for removing my charset.

So if you could backport both patches for one of the next minor releases that would be great! Thanks.

Comment 67

[Frédéric Buclin]

(In reply to Frédéric Buclin from comment #65)
> I backported glob's patch to 5.0

glob: note that I ignored the utf8 parameter in the backport, so I didn't bring back the few checks for Bugzilla->params->{utf8}. I will let you add them where needed. ;)

Comment 68

[Frédéric Buclin]

glob, I suggest you commit your patch in master before it bitrots. Also, do you have time to update my backport to include required Bugzilla->params->{utf8} checks? If that's too much work, then maybe we commit this patch in master only.

Comment 69

[:glob ✱]

master commit

To ssh://[e-mail]/bugzilla/bugzilla.git
   4864903..fedc614  master -> master

> do you have time to update my backport to include required Bugzilla->params->{utf8} checks?

not any time soon i'm sorry

Comment 70

[Frédéric Buclin]

Comment on attachment 8650004 [details] [diff] [review]
patch for 5.0, v1

>+++ b/Bugzilla/BugMail.pm

>+        Bugzilla::MIME->create(
>             attributes => {
>+                content_type => 'text/plain',
>+                charset      => 'UTF-8',
>+                encoding     => 'quoted-printable',
>             },
>+            body_str => $msg_text,
>         )

I'm not sure this is easily fixable on 5.0. If utf8 = 0, then we cannot pass |charset => 'UTF-8'|, but body_str requires the charset to be set else Email::MIME->create throws an error. We could fall back to us-ascii, but I doubt this will work correctly with all possible encodings used in the DB.

Maybe we should simply leave the backport here, and tell admins to apply it to their installation if they have utf8 = 1. What do you think?

Comment 71

[Frédéric Buclin]

glob, justdave: see comment 70.

Comment 72

[Dave Miller [:justdave]]

By definition an installation without utf8 set cannot use MIME-encoded email and stay in spec.  This is the whole reason Bugzilla started using utf8.  Encoding as iso-8859-1 and assuming that some things may encode wrong is probably the safest thing to do if utf8 is off.

Comment 73

[Frédéric Buclin]

Attachment: patch for 5.0, v1.1

I updated the backport to correctly take the utf8 parameter into account. In _generate_bugmail(), I fall back to iso-8859-1 as suggested by justdave. Note that Email::MIME will throw an error if you try to pass UTF8 data with the ISO encoding. But I suppose that's expected.

IMO, we should relnote that utf8 = 0 is deprecated and that admins are highly encouraged to convert their DB to UTF8 if they didn't yet.

justdave: could you give a look at the patch? I tested it briefly and it seems to work fine.

Comment 74

[Frédéric Buclin]

dkl: could you give a look at the backport? If you don't have time or don't plan to review it, please redirect the review to gerv.

Comment 75

[David Lawrence [:dkl]]

Comment on attachment 8658427 [details] [diff] [review]
patch for 5.0, v1.1

Review of attachment 8658427 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 76

[David Lawrence [:dkl]]

Clearing needinfo

Comment 78

[Frédéric Buclin]

To ssh://[e-mail]/bugzilla/bugzilla.git
   3b29cba..948b580  5.0 -> 5.0