unnecessary line breaks and excessive indentation in email subject lines

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
Email Notifications
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Jeff Lawson, Assigned: Jeff Lawson)

Tracking

({regression})

Bugzilla 3.0
regression
Dependency tree / graph
Bug Flags:
approval +
blocking3.1.3 +
approval3.0 +
blocking3.0.4 +

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 296191 [details]
test case demonstating broken fix, plus two alternative fixes

+++ This bug was initially created as a clone of Bug #374424 +++

if a bug's summary has an apostrophe in it (like this one) emails generated by it will have the Subject: line split by a newline.

This is a new bug because the fix performed in Bug 374424 was defective and incomplete.
(Assignee)

Updated

10 years ago
Flags: blocking3.0.4?
Target Milestone: --- → Bugzilla 3.0

Updated

10 years ago
Blocks: 410796
(Assignee)

Comment 1

10 years ago
Created attachment 296215 [details] [diff] [review]
avoid excessive line wrapping done by Encode using method 3 (patch for 3.0.x)

This patch changes the wrapping boundary used by Encode::MIME, rather than attempting to undo any wrapping that might have been done by it with a regex.
Attachment #296215 - Flags: review?
(Assignee)

Updated

10 years ago
Assignee: email-notifications → mkanat
Flags: approval?
Flags: approval3.0?
Assignee: mkanat → jlawson-mozilla
Flags: approval?
Flags: approval3.0?
Attachment #296215 - Flags: review? → review?(mkanat)

Comment 2

10 years ago
Comment on attachment 296215 [details] [diff] [review]
avoid excessive line wrapping done by Encode using method 3 (patch for 3.0.x)

No, editing undocumented internal variables is not safe in a distributed product like Bugzilla.
Attachment #296215 - Flags: review?(mkanat) → review-

Updated

10 years ago
Flags: blocking3.0.4? → blocking3.0.4+

Comment 3

10 years ago
Comment on attachment 296191 [details]
test case demonstating broken fix, plus two alternative fixes

I'll take your "method 2" here, if you want to attach it.

I like the technical aspects of method 3 better, but I haven't seen that variable documented anywhere, so I'm worried that it could change from under us and we'd have to fix things again later.
(Assignee)

Comment 4

10 years ago
Created attachment 296984 [details] [diff] [review]
Eliminate extra line breaks using method 2

I too agree method 3 would be better, since it avoids having to "undo" the work done by Encode.  But this is not a hill I wanna die on.
Attachment #296215 - Attachment is obsolete: true
Attachment #296984 - Flags: review?(mkanat)

Comment 5

10 years ago
Comment on attachment 296984 [details] [diff] [review]
Eliminate extra line breaks using method 2

Excellent! Thank you very much. :-)
Attachment #296984 - Flags: review?(mkanat) → review+

Updated

10 years ago
Status: NEW → ASSIGNED
Flags: approval+
Are the findings of this bug in any way applicable to bug 328628?

Updated

10 years ago
Flags: approval3.0+

Comment 7

10 years ago
(In reply to comment #6)
> Are the findings of this bug in any way applicable to bug 328628?

  No, they're unrelated.
Hrm, I'm coming late, so maybe we want a new bug for this. It seems to me there's a s/\?=\n =\?UTF-8\?Q\?//g; missing in here. If encoded parts are being split, then each new line starts with =?UTF-8?Q?, which breaks the re-combined string.
(Assignee)

Comment 9

10 years ago
Created attachment 296993 [details] [diff] [review]
Eliminate extra line breaks using method 2 and handle UTF-8 encoded passages

I can repro a problem when the merged lines contain UTF-8 encoded passages.  Here is an updated patch based on the attachment 280276 [details] [diff] [review] from bug 328628.
Attachment #296984 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #296993 - Flags: review?(mkanat)
(Assignee)

Updated

10 years ago
Attachment #296984 - Flags: review-

Comment 10

10 years ago
Please don't r- your own patches. If there is something wrong with them, simply mark them as obsolete.
(Assignee)

Comment 11

10 years ago
There is nothing on http://wiki.mozilla.org/Bugzilla:Review nor on http://wiki.mozilla.org/Bugzilla:Submitting_Patches that says I cannot r- my own.

Comment 12

10 years ago
Only reviewers can review patches. And now you know. ;)

Updated

10 years ago
Flags: blocking3.1.3+

Comment 13

10 years ago
Comment on attachment 296993 [details] [diff] [review]
Eliminate extra line breaks using method 2 and handle UTF-8 encoded passages

Ugh. Um, could you maybe contact the Encode maintainer and ask him if your "method 3" is safe and future-proof? It's looking more like we'll want to use that instead. This is a very hacky-looking regex.

Comment 14

10 years ago
Also, there's nothing technically wrong with r-'ing your own patch. I can't see who that hurts. It's not what we standardly do (we just obsolete things usually), but we've done it ourselves before.
(Assignee)

Comment 15

10 years ago
(In reply to comment #13)
> (From update of attachment 296993 [details] [diff] [review])
> Ugh. Um, could you maybe contact the Encode maintainer and ask him if your
> "method 3" is safe and future-proof? It's looking more like we'll want to use
> that instead. This is a very hacky-looking regex.
> 

Ok, I've emailed 'dankogai@dan.co.jp', who is listed as the maintainer according to http://search.cpan.org/dist/Encode/

I agree that it would be better to use the internal variable and just eliminate the wrapping, rather than attempting to "undo" all variations of the wrapping/escapement possibilities.  Even if it breaks or changes in the future, I argue it would be reasonable to just re-address it again if/when it does.

Comment 16

10 years ago
Except that you don't know the version of Encode being used, and you would have to list all possible versions and what to do for each of them.

Comment 17

10 years ago
You know the minimum version is whatever is shipped with perl 5.8.1.
(Assignee)

Comment 18

10 years ago
If there is ever a change in the future that eliminates the availability of "bpl", then we can add a comparison against the version of $Encode::MIME::Header::VERSION and do different logic as needed.

However, I'm not yet aware of any incompatibilities yet.  Encode-2.12 (released 08 Sep 2005) through the current Encode-2.23 (released 29 May 2007) all seem to include use of the "bpl" variable.  It's probable that earlier versions than 2.12 also made use of that variable but that is the earliest that seems to be available in CPAN:  http://cpan.belfry.net/modules/by-module/Encode/
(Assignee)

Comment 19

10 years ago
For reference, Perl 5.8.1 appears to ship with Encode 1.9801 (released 2003/08/20) and it also includes use of the "bpl" variable.

Updated

10 years ago
Flags: approval3.0+
Flags: approval+

Updated

10 years ago
Duplicate of this bug: 410796
(Assignee)

Comment 21

10 years ago
I have received a reply from the Encode maintainer endorsing the technique used in method 3 (attachment 296215 [details] [diff] [review]).

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jeff,

Thank you for your report.  As fo the use of bpl, that is okay (you are even careful enough to localize it).  Maybe I should document this on Encode::MIME::Header.

Dan the Encode Maintainer

On Jan 26, 2008, at 02:59 , Jeff Lawson wrote:

> Hello Dan,
>
> I am currently investigating a problem in Mozilla's Bugzilla project 
> that deals with Encode::MIME::Header and its line wrapping of headers 
> that are too long.  This problem is most apparent in the "Subject:" 
> headers since the wrapping causes a visible space to occur in mail 
> readers, sometimes in the middle of a word or after certain 
> punctuation.
>
> I have developed a workaround, but it involves accessing the 
> undocumented "bpl" element, however there have been some concerns over 
> using such an undocumented variable.  Here is an example of the code I 
> am using:
>
>   use Encode::MIME::Header;
>   local $Encode::Encoding{'MIME-Q'}->{'bpl'} = 998;
>   my $encoded = encode('MIME-Q', $value);
>
> I would like to get your opinion about the legitimacy of modifying 
> this internal variable and whether you foresee any immediate concerns 
> about future compatibility.
>
> I have a self-contained Perl test-case that demonstrates the wrapping 
> here:
> https://bugzilla.mozilla.org/attachment.cgi?id=296186
>
>
> Also, if you could comment on why the line wrapping is causing 
> problems with email readers at all, that would be helpful too.
>
>
> The bugs that relate to this issue are available here:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=411544
> https://bugzilla.mozilla.org/show_bug.cgi?id=374424
> http://rt.cpan.org/Public/Bug/Display.html?id=27395
>
>
> Thanks,
> Jeff
>
>

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)

iD8DBQFHmi6vErJia/WXtBsRAurVAJ9TXHxWhm0yfkdz0U4dIPMo2GmLrACePzwL
l0UQ8CUztCvncH8Ix0SNHn8=
=hCvu
-----END PGP SIGNATURE-----

(Assignee)

Comment 22

10 years ago
Comment on attachment 296215 [details] [diff] [review]
avoid excessive line wrapping done by Encode using method 3 (patch for 3.0.x)

seek re-approval after endorsement of technique.
Attachment #296215 - Attachment is obsolete: false
Attachment #296215 - Flags: review?(mkanat)

Comment 23

10 years ago
Comment on attachment 296215 [details] [diff] [review]
avoid excessive line wrapping done by Encode using method 3 (patch for 3.0.x)

Okay, looks good! :-)
Attachment #296215 - Flags: review?(mkanat)
Attachment #296215 - Flags: review-
Attachment #296215 - Flags: review+

Updated

10 years ago
Attachment #296993 - Attachment is obsolete: true
Attachment #296993 - Flags: review?(mkanat)

Updated

10 years ago
Flags: approval3.0+
Flags: approval+
(Assignee)

Updated

10 years ago
Assignee: jlawson-mozilla → mkanat
Status: ASSIGNED → NEW

Comment 24

10 years ago
No, process is that it stays assigned to you. :-) I keep track of everything that needs to be checked in by a saved search, no worries.
Assignee: mkanat → jlawson-mozilla

Comment 25

10 years ago
Canceling approval+ for 3.2. This patch doesn't apply cleanly on tip as this part of the code changed quite a bit:

http://mxr.mozilla.org/mozilla/source/webtools/bugzilla/Bugzilla/Mailer.pm#75

It requires a separate patch for 3.2. Jeff, can you write the patch?
Status: NEW → ASSIGNED
Flags: approval+

Updated

10 years ago
Attachment #296215 - Attachment description: avoid excessive line wrapping done by Encode using method 3 → avoid excessive line wrapping done by Encode using method 3 (patch for 3.0.x)
Keywords: checkin-needed
(Assignee)

Comment 26

10 years ago
Created attachment 299434 [details] [diff] [review]
 avoid excessive line wrapping done by Encode using method 3 (patch for 3.2 Tip)  

patch against Bugzilla-3.2 (tip)
(Assignee)

Updated

10 years ago
Attachment #299434 - Flags: review?(mkanat)

Comment 27

10 years ago
Comment on attachment 299434 [details] [diff] [review]
 avoid excessive line wrapping done by Encode using method 3 (patch for 3.2 Tip)  

Actually, I'm just realizing that there's a "use" statement in the middle of the code here. We should be "use"ing that at the top, or this should be a "require".

That needs to be fixed on checkin for both patches.
Attachment #299434 - Flags: review?(mkanat) → review+

Updated

10 years ago
Flags: approval+

Comment 28

10 years ago
Thank you very much for this patch, Jeff!

tip:

Checking in Bugzilla/Mailer.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Mailer.pm,v  <--  Mailer.pm
new revision: 1.17; previous revision: 1.16
done

3.0 branch:

Checking in Bugzilla/Mailer.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Mailer.pm,v  <--  Mailer.pm
new revision: 1.7.2.7; previous revision: 1.7.2.6
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.