Closed Bug 714724 Opened 8 years ago Closed 4 years ago

correctly encode emails as quoted-printable

Categories

(Bugzilla :: Email Notifications, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 5.0

People

(Reporter: glob, Assigned: glob)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 9 obsolete files)

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.
Blocks: 714488
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #587260 - Flags: review?(mkanat)
Attached patch patch v2 (obsolete) — Splinter Review
i forgot to revert bug 696005
Attachment #587260 - Attachment is obsolete: true
Attachment #587260 - Flags: review?(mkanat)
Attachment #587623 - Flags: review?(mkanat)
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.
Attachment #587623 - Flags: review?(mkanat) → review-
Attached patch patch v3 (obsolete) — Splinter Review
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.
Attachment #587623 - Attachment is obsolete: true
Attachment #588811 - Flags: review?(mkanat)
Attachment #588811 - Flags: review?(mkanat) → review+
Flags: approval+
Target Milestone: --- → Bugzilla 4.4
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Mailer.pm
Committed revision 8093.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
after committing this i found a problem with Email::MIME, backing out:

Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 587623 [details] [diff] [review]
patch v2

re-requesting review given patch3's failure.
Attachment #587623 - Attachment is obsolete: false
Attachment #587623 - Flags: review- → review?
Attachment #588811 - Attachment is obsolete: true
Flags: approval+
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.
Attached patch patch v5 (obsolete) — Splinter Review
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.
Attachment #593009 - Flags: review?(mkanat)
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.
Attachment #593009 - Flags: review?(mkanat) → review-
(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.
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 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.
Attachment #593009 - Flags: review-
(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.
(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.
Attached patch patch v7 (obsolete) — Splinter Review
here's another approach to attack this problem.

this sub-classes Email:MIME to ensure the headers are correct before the body is set.
Attachment #587623 - Attachment is obsolete: true
Attachment #593009 - Attachment is obsolete: true
Attachment #587623 - Flags: review?
Attachment #610496 - Flags: review?
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.
(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.
Status: REOPENED → ASSIGNED
(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.
Attached patch patch v8 (obsolete) — Splinter Review
Attachment #610496 - Attachment is obsolete: true
Attachment #610496 - Flags: review?
Attachment #613601 - Flags: review?
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. :(
(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.
(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 on attachment 613601 [details] [diff] [review]
patch v8

found a problem -- triggers "Wide character in print at Bugzilla/Send/Sendmail.pm line 34."
Attachment #613601 - Flags: review?
glob: any progress on this bug?
note to self: also need to fix whine.pl
Attached patch patch v9 (obsolete) — Splinter Review
fixes the "Wide character in print" error, and updates whine to use the MIME modules instead of hand rolling multipart email.
Attachment #613601 - Attachment is obsolete: true
Attachment #658519 - Flags: review?(LpSolit)
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.
Attachment #658519 - Attachment is obsolete: true
Attachment #658519 - Flags: review?(LpSolit)
Attached patch patch v10 (obsolete) — Splinter Review
switch from 8bit to binary encoding.
Attachment #660736 - Flags: review?(LpSolit)
Summary: encode all emails as 8bit instead of quoted-printable → encode all emails as binary instead of quoted-printable
(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?
http://en.kioskea.net/contents/courrier-electronique/mime.php3 specifically calls out binary as "binary format; not recommended." Just saying... ;)
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
(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 on attachment 660736 [details] [diff] [review]
patch v10

r- due to the lack of support for binary, see previous comments.
Attachment #660736 - Flags: review?(LpSolit) → review-
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.
Target Milestone: Bugzilla 4.4 → Bugzilla 5.0
(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.
Summary: encode all emails as binary instead of quoted-printable → correctly encode emails as quoted-printable
Blocks: 713208
Duplicate of this bug: 939126
Blocks: 110773
Duplicate of this bug: 1038629
Target Milestone: Bugzilla 5.0 → ---
Duplicate of this bug: 820806
Duplicate of this bug: 1072288
Duplicate of this bug: 1078135
Duplicate of this bug: 1081045
Duplicate of this bug: 1094663
Duplicate of this bug: 1116134
- 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.
Attachment #660736 - Attachment is obsolete: true
Attachment #8610382 - Flags: review?(LpSolit)
Duplicate of this bug: 1178737
I cannot reproduce encoding problems using SMTP, so it's a bit hard for me to check if your patch fixes these problems.
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.
Attachment #8610382 - Flags: review?(LpSolit) → review+
Do we want this fix for 5.0.1?
Flags: approval?
Duplicate of this bug: 1182004
(In reply to Frédéric Buclin from comment #49)
> Do we want this fix for 5.0.1?

Yes.
Flags: approval?
Flags: approval5.0+
Flags: approval+
Target Milestone: --- → Bugzilla 5.0
thorsten, is it possible for you to check that this patch addresses your issues from bug 1182004?
Flags: needinfo?(tschoening)
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
Flags: needinfo?(tschoening)
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.
(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. :)
Attachment #8610382 - Attachment description: 714724_11.patch → patch for master, v11
(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.
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.
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.
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...
Can you reproduce the issue if you use SMTP instead of sendmail?
(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...
(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.
(In reply to Frédéric Buclin from comment #60)
> Can you reproduce the issue if you use SMTP instead of sendmail?

Yes.
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.
Attached patch patch for 5.0, v1 (obsolete) — Splinter Review
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? :)
Attachment #8650004 - Flags: review?(glob)
(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.
(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. ;)
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.
Flags: needinfo?(glob)
master commit

To ssh://gitolite3@git.mozilla.org/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
Flags: needinfo?(glob)
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?
glob, justdave: see comment 70.
Flags: needinfo?(justdave)
Flags: needinfo?(glob)
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.
Flags: needinfo?(justdave)
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.
Attachment #8650004 - Attachment is obsolete: true
Attachment #8650004 - Flags: review?(glob)
Flags: needinfo?(glob)
Attachment #8658427 - Flags: review?(justdave)
Attachment #8658427 - Flags: review?(justdave) → review?(dkl)
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.
Flags: needinfo?(dkl)
Comment on attachment 8658427 [details] [diff] [review]
patch for 5.0, v1.1

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

r=dkl
Attachment #8658427 - Flags: review?(dkl) → review+
Clearing needinfo
Flags: needinfo?(dkl)
Duplicate of this bug: 733162
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   3b29cba..948b580  5.0 -> 5.0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago4 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1187264
Duplicate of this bug: 1219006
Blocks: 1235395
You need to log in before you can comment on or make changes to this bug.