Closed Bug 411544 Opened 16 years ago Closed 16 years ago

unnecessary line breaks and excessive indentation in email subject lines

Categories

(Bugzilla :: Email Notifications, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.0

People

(Reporter: jlawson-mozbug, Assigned: jlawson-mozbug)

References

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

+++ 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.
Flags: blocking3.0.4?
Target Milestone: --- → Bugzilla 3.0
Blocks: 410796
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: email-notifications → mkanat
Flags: approval?
Flags: approval3.0?
Assignee: mkanat → jlawson-mozilla
Flags: approval?
Flags: approval3.0?
Attachment #296215 - Flags: review? → review?(mkanat)
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-
Flags: blocking3.0.4? → blocking3.0.4+
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.
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 on attachment 296984 [details] [diff] [review]
Eliminate extra line breaks using method 2

Excellent! Thank you very much. :-)
Attachment #296984 - Flags: review?(mkanat) → review+
Status: NEW → ASSIGNED
Flags: approval+
Are the findings of this bug in any way applicable to bug 328628?
Flags: approval3.0+
(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.
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
Attachment #296993 - Flags: review?(mkanat)
Attachment #296984 - Flags: review-
Please don't r- your own patches. If there is something wrong with them, simply mark them as obsolete.
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.
Only reviewers can review patches. And now you know. ;)
Flags: blocking3.1.3+
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.
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.
(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.
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.
You know the minimum version is whatever is shipped with perl 5.8.1.
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/
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.
Flags: approval3.0+
Flags: approval+
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-----

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 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+
Attachment #296993 - Attachment is obsolete: true
Attachment #296993 - Flags: review?(mkanat)
Flags: approval3.0+
Flags: approval+
Assignee: jlawson-mozilla → mkanat
Status: ASSIGNED → NEW
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
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+
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
patch against Bugzilla-3.2 (tip)
Attachment #299434 - Flags: review?(mkanat)
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+
Flags: approval+
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
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.