Suggested commit message (feel free to change / doublecheck contributors, I scanned manually):
#84883 by earnie, ximo, netbjarne, scor, greggles, pillarsdotnet, DuaelFr, roderik: make Unicode::mimeHeaderEncode() conform to RFC 2047.
Problem
1)
Drupal\Component\Utility\Unicode::mimeHeaderEncode()
( mime_header_encode()
in Drupal < 8) does not conform to RFC 2047 standards, particularly to one bit in section 2:
An 'encoded-word' may not be more than 75 characters long, including
'charset', 'encoding', 'encoded-text', and delimiters. If it is
desirable to encode more text than will fit in an 'encoded-word' of
75 characters, multiple 'encoded-word's (separated by CRLF SPACE) may
be used.
This non-conformance was apparently introduced on purpose, given the current comments on the method/function (introduced july 2005, commit #11a4aba by Steven):
* Using \n as the chunk separator may cause problems on some systems and
* may have to be changed to \r\n or \r.
...so people with problems would have to hack their own solution in case or problems. Which was apparently because of (issues related to) restrictions in PHP's mail() function:
$subject - Subject of the email to be sent.
Caution: This must not contain any newline characters, or the mail may not be sent properly.
This restriction on mail() has by now been removed (as reported around november 2009), however - so we should be able to just conform to RFC 2047.
2)
The 'chunk size' of 47 is not correct, resulting in errors because of the length of encoded lines being longer than 75 characters.
Issue history
The combination of 2 bugs probably contributed to confusion around pinpointing the exact cause(s).
Until #95, people were trying to remove any newlines from the (encoded) subject header, to solve reported problems. (Which does not conform to the RFC). Plus figuring out where they came from.
The separator and chunk size were first introduced in #105, after which there were relatively few diversions.
Proposed resolution
Use Symfony's mime classes to do the encoding.. See #195
Remaining tasks
Review
Commit
stackoverflow.com/help/someone-answers
User interface changes
none
API changes
none
Data model changes
none
Beta phase evaluation
Issue category | Bug because sending mail breaks (in easily reproducible situations) |
---|---|
Issue priority | Major because same |
Unfrozen changes | No. |
Prioritized changes | Improves stability; fixes clear bug; backport to D7/D6 |
Disruption | No |
Comment | File | Size | Author |
---|---|---|---|
#216 | 84883-216.patch | 13.06 KB | alexpott |
#209 | 84883-209.patch | 14.12 KB | quietone |
#209 | interdiff-205-209.txt | 1.17 KB | quietone |
#205 | 84883-205.patch | 14.14 KB | alexpott |
#205 | 203-205-interdiff.txt | 766 bytes | alexpott |
Issue fork drupal-84883
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
drewish CreditAttribution: drewish commentedWhat version of PHP are you using?
Comment #2
huss CreditAttribution: huss commentedPHP Version 4.3.11
Comment #3
huss CreditAttribution: huss commentedSory! In the previous message I changed the title of the issue and couldn't restore it. But now the issue has the same title.
Comment #4
netbjarne CreditAttribution: netbjarne commentedOh man - just spend 6 hours on a day off to narrow this down.
I can confirm that there indeed is a bug somewhere.
Symptom:
If sitename in /admin/settings contains national characters, in my case "æ", mailing of login info etc from the user module fails with this error:
User.module version info:
PHP Version: 4.4.4
Workaround:
Avoid using national characters in sitename. If I replace "æ" with "ae" in my sitename, the user module can once again email.
I can reproduce this error on several installations. Any help is greatly appreciated.
Comment #5
netbjarne CreditAttribution: netbjarne commentedA small side note, the contact module has not got the same problems with national characters - I have been sending several messages using the contact module, which included "æøå" and such in the subject line - all went through fine.
Please let me know if I can provide any useful info to help get this fixed. I'd really like my site to have the correct name ;-)
Comment #6
netbjarne CreditAttribution: netbjarne commentedScreenshots and phpinfo attachments for more details
Comment #7
netbjarne CreditAttribution: netbjarne commentedHere the second screenshot where mailing fails due to national characters in sitename.
Comment #8
netbjarne CreditAttribution: netbjarne commentedLastly, phpinfo dump.
Comment #9
netbjarne CreditAttribution: netbjarne commentedMore diagnostic results and a better workaround.
I found out that BOTH the user.module AND the contact.module, uses the user_mail function from the user.module to send emails. The contact.module succeeds, the user.module fails - sending emails the email subject includes national characters.
The subject line of emails sent by the user.module, can be modified in the administer >> settings >> users menu. If having a sitename with national characters, you can replace every instance of %site in the text fields, with plain ascii instead.. Please see screenshot.
Comment #10
gregglesFollowing on netbjarne's work...
In contact.module we have this subject:
383 : dries 1.65 $subject = '['. variable_get('site_name', 'drupal') .'] '. $form_values['subject'];
In user.module we have a subject that depends on the reason it is being sent, but they all looks similar to this:
1513 : unconed 1.655 return t('Account details for !username at !site', $variables);
Note that I'm pasting from annotate view so we know which line/commit is involved.
So, it seems to me that by using different methods of variable insertion into the email subject we are getting this different behavior of the mail succeeding in one case and failing in another.
In both cases we use drupal_mail to send the messages. Perhaps some cleanup of the subject would be appropriate in there, but I'm not sure what kind of cleanup is necessary.
Comment #11
scor CreditAttribution: scor commentedHi,
I have the same problem on a unix php 4.4.3 server. I recently migrated from php 4.4.1. which never caused this problem.
This problem occurs whenever the subject of the email sent by the server is longer than 46 characters and contains national characters. I also managed to get this message with contact module, when the subject of the email is long enough.
In the mime_header_encode function, when the subject of the email contains national characters, the subject is encoded in base64 by chunks of 47 characters. If there is only one chunk, there is no problem, the email is sent with the encoded subject. But if there are more than one chunk, a line break is put at the end of each chunk. I guess it's to comply with the RFC2047 :
Now, this is what the php mail function says about the subject of the email :
and it doesn't mention anything about the encoded subjects.
When I look at the source of some emails sent from a config which works (unix php 4.3.11) with a long subject with national characters, the line breaks of the subject have been deleted (by the mail function I assum). All the base64 chunks of the subject end up on the same line with a SPACE between each of them, so I figured there was no point to have the line break in the mime_header_encode function and decided to remove it.
The line break is added in unicode.inc, line 232:
I deleted the "\n" and it worked. The only thing is that the email sent doesn't comply with the rfc2047 since all the chunks of the subjects are on the same line, but it was already like that before on the working config. Try to send a message with contact module with a long subject like this :
Le sujet de ce message est très long!
even drupal.org puts all the chunks on the same line!
I'm not sure what triggers this problem (version of php, mb_string settings) but check your config and post more details about it!
Comment #12
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedmoving to HEAD since user_mail hasn't changed. But apparently PHP has(?).
Comment #13
gregglesSteps attempted:
1. send a message on the sitewide contact with subject containing: på and æ.
2. set sitename to contain æ
3. send message on user personal contact form
Expected results:
Error messages
Actual results:
Mails went through, no errors printed to screen nor watchdog.
My tests were only on Drupal5.x head as of today, so I imagine this is Drupal4.7 related. If my means of testing were wrong or if someone else can confirm it in 5.x then please comment and change the version back to Drupal5.x.
Comment #14
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedI've tried to repro this on 4.7-x running on php 5.1 and couldn't reproduce it. Downgrading.
Comment #15
netbjarne CreditAttribution: netbjarne commentedOn my host, surftown, the problem still exists. Also in 5.0 beta 2 - see attached screenshot.
Needless to say, that if in the "request new password" text, replace !site (that contains national characters), with !uri (that only contains ascii), the problem goes away.. Same ol' I
Bjarne
Comment #16
gregglesSo, I'd say this is a combination of national characters in the subject AND a certain configuration of PHP. I agree with Killes on priority, but move it back to 5.x since it affects that and should get attention there.
@netbjarne - the problem only affects the per user contact page, right? And since you are one of the few who can test this, would you be willing to try out a patch if one is provided?
Comment #17
Steven CreditAttribution: Steven commentedThis is really a bug in PHP that should be addressed. Removing the linebreak means disobeying the RFC.
Comment #18
scor CreditAttribution: scor commentedI had the same problem that I solved with the solution above. I think this issue concerns only a few PHP configurations and is not related to the version of drupal. I'd say it's directly related to the mail function of PHP, and maybe to the national settings. The problem does not only appear in the contact page, but as I stated previously
I first notice this problem with subscriptions.module which was not sending the emails to the users who had a username containing national characters... Try as an example to create an account with a long user name containing national characters. The subject of the resulting email has to be longer than 46 characters. This will force the subject of the email to be encoded and several chuncks will be generated... and the bug should appear... at least on the buggy PHP configurations.
Greggles, have you tried that? Does the encoded email subject in the email header contain any line break ?
I can also try a patch if provided.
Steven, I agree that the RFC is disobeyed, but shouldn't it be PHP that should add these linebreaks instead, since the PHP documentation requires no linebreak in the email subject ?
Comment #19
Steven CreditAttribution: Steven commentedPHP should not be adding the linebreaks, unless PHP were to actually accept a random unicode string and do the encoding itself.
PHP requires us to do the encoding, and separating chunks on lines is an essential part of this.
Comment #20
scor CreditAttribution: scor commentedThis is what the php mail function says about the subject of the email :
and it doesn't mention anything about the linebreaks in the case of an encoded subject.
Is there an exception for the encoded subjects ?
Moreover, when I send a message through drupal.org, the encoded subject chunks are all on the same line:
Comment #21
netbjarne CreditAttribution: netbjarne commentedMore info on this bug.
I wrote messages on the contact form. Some went through. Some didn't. I can confirm, that subject messages have to be of a certain lengt (46 has been mentioned, that looks about right) - AND that national characters must be present in the final subject of the email (for instance, if the sitename contains them).
Heres the subject of a message that passed through the user_mail function:
[Kegnæs Friskole] 0123456789012345678901234567
Add another "8" to the subject, and the mail fails.
I will try the hack of removing newline characters of in the subject, and return with additional info.
Bjarne
Comment #22
netbjarne CreditAttribution: netbjarne commentedI found me another workaround (gotta make a list of drupal workarounds, it gets hard to remember to re-apply them all every time a new version is installed ;-))
in include.inc, the mime_header_encode function, i replaced
with
Its an overly big chunk size, since the mail form does not allow lengthy subjects, however, utf-8 encoding seems to lengthen things a low...
Now - mail passes through just fine - no linebreaks in the subject (who came up with that idea anyway) - and no lost messages.
Bjarne
Comment #23
netbjarne CreditAttribution: netbjarne commentedMistake in last post. The edited file was not include.inc, but unicode.inc. Sorry..
Comment #24
yasenp CreditAttribution: yasenp commentedI had the similar problem, but "$chunk_size = 400;" solved it. Many thanks to netbjarne!
Comment #25
ximo CreditAttribution: ximo commentedWhat's the story on this bug? Has it been fixed in another issue, or is this bug still hanging around? This should be fixed for Drupal 6.
Comment #26
ximo CreditAttribution: ximo commentedI just got this error without having any national characters in the subject line. I was creating a new user from admin/user/user/create, and the subject of the email message to be sent out was
where !site = . I tried some of the mentioned workarounds, but found simply removing the\n</codefrom the end of <code>$output .= ' =?UTF-8?B?'. base64_encode($chunk) ."?=\n";
did the trick. Why is the newline there in the first place? It's looks like it's the newline that breaks the mail() function.I'm not sure why I got this error when I had no national characters in the subject line or the site name. This was on Drupal 5.1, PHP 4.4.6, Apache/1.3.37 (Unix).
Comment #27
ximo CreditAttribution: ximo commentedI'm obviously blind, I did have an
in the subject line!I looked into the background of this issue, and found these links:
http://www.php-security.org/MOPB/MOPB-34-2007.html
http://bugs.php.net/bug.php?id=15841
Didn't make me any wiser though..
Then I saw this note in the mime_header_encode() function_
Changing
\n
to\r
did the trick, Drupal could now send the mail with a subject containing . I know this doesn't follow the RFC, but it works with the mail() function and I believe that's more important. Having to manually change this deep inside the core shouldn't be necessary, why can't the chunk separator be\r
by default? See the attached patch.Comment #28
Anonymous (not verified) CreditAttribution: Anonymous commentedPerhaps the following would take care of the issue? I don't have a good scenario to test with.
Comment #29
siammoney.net CreditAttribution: siammoney.net commentedwarning: mail(): Bad parameters to mail() function, mail not sent. in /home/content/p/u/r/purihostsiam/html/includes/common.inc on line 1979.
this message occurs when create a new account . somebody help me please!.
Comment #30
scor CreditAttribution: scor commentedsiammoney.net: are you using accented characters in your username when creating your account?
Comment #31
siammoney.net CreditAttribution: siammoney.net commentedyes I have test create a new account for quest by use accented characters in username
Comment #32
scor CreditAttribution: scor commentedaggregator???
can you check that you don't have the problem with non accented chars.
@ximo: whether you use \r, anything else or nothing at all, it's the same as long as it's not \n (see #11)
Comment #33
siammoney.net CreditAttribution: siammoney.net commentedthank you, but when I use a common chars such as puri007 that always show this errors.How 's about syntax in line 1976 on common.inc? I confuse in $mimeheaders and $subject.
Comment #34
siammoney.net CreditAttribution: siammoney.net commentedthank you for suggestion,I try new install drupal 5.2 that good work.I think it 's cloud be errors about theme or module that after install. so happy
Comment #35
scor CreditAttribution: scor commenteda simple username like puri007 should not give you any error. did you modify the code of the core? I suggest you install a fresh drupal-5.3 on the same server without any contributed module to see if you still get the error when creating an account with puri007.
Comment #36
siammoney.net CreditAttribution: siammoney.net commentedthank you, I use to drupal-5.2 because it's no error after new install.Now,I have suspect to log in of drupal members why accept a cookies because another's website had not to opened the cookie.
Comment #37
drummPlease reopen when this has been tested with a current version of 5.x. 5.2 is now an older release.
Comment #38
yraffah CreditAttribution: yraffah commentedI'm having the same issue with D6.1. my site phsite.org/main is using an Arabic name in it's site name, I tried to change !site by explicitly defining it but that didn't help :(
I used to overcome this with the smtp.module, but it is not ported to 6.x yet, not sure if it even will be ported!
Server PHP version is 4.4.6
Comment #39
scor CreditAttribution: scor commentedComment #40
yraffah CreditAttribution: yraffah commentedI'm still having the same issue, does any one know of a solution?
Comment #41
scor CreditAttribution: scor commented@liquid crystal: can you try the fix I described in #11 http://drupal.org/node/84883#comment-449438
it's not ideal, but it might help you. please report whether it works for you or not.
also, how many characters does the subject of the email contain?
Comment #42
yraffah CreditAttribution: yraffah commentedHi scor,
It seems Drupal 6 is a lot different than Drupal 5 in terms of the unicode.inc. I'm not a programmer by any mean but at least that how it looks to my eyes:
279 function mime_header_encode($string) {
280 if (preg_match('/[^\x20-\x7E]/', $string)) {
281 $chunk_size = 47; // floor((75 - strlen("=?UTF-8?B??=")) * 0.75);
282 $len = strlen($string);
283 $output = '';
284 while ($len > 0) {
285 $chunk = drupal_truncate_bytes($string, $chunk_size);
286 $output .= ' =?UTF-8?B?'. base64_encode($chunk) ."?=\n"; <--- should I remove this \n?
287 $c = strlen($chunk);
288 $string = substr($string, $c);
289 $len -= $c;
290 }
291 return trim($output);
292 }
293 return $string;
294 }
The subject is 69 characters long, do you think I should try and make it less than 64?
Comment #43
yraffah CreditAttribution: yraffah commentedHi scor,
Removing the \n from that line solved the problem, thanks a zillion time :D
Comment #44
Pr0v0dn1k CreditAttribution: Pr0v0dn1k commentedI've solved the problem on v6.2, PHP 4.4.7 in the following way:
in file 'includes/mail.inc', in line 186 replaced
mime_header_encode($message['subject']),
with
str_replace("\n", ' ', mime_header_encode($message['subject']) ),
It just replaces LF with SPACE in the subject after it is received from the mime_header_encode.
Comment #45
jcnventura CreditAttribution: jcnventura commentedThis problem is encountered by all modules that send e-mail, including the contact module.
In the print module, a user was unable to send a message with the subject:
"admin has sent you a message from Urduseek.com انگریزی اردو لغت"
He later tried the same in the contact module, and it also failed. However, I have tried it in my system and it works fine.. He tried it in another system and was it was also fine.. The original print module issue is : #290517: bad parameters in mail() function.
João
Comment #46
yuREKLI CreditAttribution: yuREKLI commentedI encountered this error in a site which is hosted by Godaddy. I have solved this error by using SMTP Module: http://drupal.org/project/smtp.
Comment #47
Anonymous (not verified) CreditAttribution: Anonymous commentedThere is no patch so setting back to active. Also bumping to 7.x since the problem still exists. Should we consider that Drupal core should use the logic from smtp module?
Comment #48
jcnventura CreditAttribution: jcnventura commentedThere is a patch (in #44). It's not a machine-readable patch to use with the patch program, but it is a patch anyway.
João
Comment #49
gregglesWhich makes it not a patch. We only set the status to patch (code needs review) when there is a machine readable patch.
This patch was rolled against 6.x but also applies to 7.x.
Comment #50
scor CreditAttribution: scor commentednote that this breaks the RFC2047, but seems to be the only way to fix the problem, unless it is fixed on a PHP level (as suggested by Steven).
patch documented.
Comment #51
Anonymous (not verified) CreditAttribution: Anonymous commentedThe patch looks pretty. I don't have a means to test it though.
Comment #52
Anonymous (not verified) CreditAttribution: Anonymous commentedThe last submitted patch failed testing.
Comment #53
manoloka CreditAttribution: manoloka commentedThis worked for me :-)
Thanks
Comment #54
Anonymous (not verified) CreditAttribution: Anonymous commentedResetting for the testbot one more time.
Comment #55
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #56
Dries CreditAttribution: Dries commentedInstead of writing 'see #84883', please reference the correct article in the RFC. Thanks.
Comment #57
apadernoComment #58
scor CreditAttribution: scor commentedso far this issue has not been reproduced on PHP 5 so it might NOT be necessary to commit it to HEAD (and just to 6.x and 5.x). I'll try to reproduce on PHP 5.
EDIT: missed the NOT
Comment #59
apadernoI tried to send an email from Drupal 6 installed in a local web site using PHP5, but I didn't have any problems. I changed the user name to Espero ååå, and then I sent a message using the contact form; Drupal didn't return any error messages.
I must then say that the local web site is running on a Mac using Mac OS X 10.5.6.
Comment #60
scor CreditAttribution: scor commentedrerolling patch against HEAD with link to this issue. I wasn't able to reproduce the issue on PHP 5, therefore I'm not convinced this needs to go into HEAD.
Would this issue deserve a test?
Comment #61
scor CreditAttribution: scor commented@kiamlaluno the subject of your email must be at least 46 character long to reproduce the error. Try with the following subject:
Comment #62
apadernoI tried it, and it didn't receive any error message.
I also tried it on Drupal 6.9 on my web site, running on Apache/1.3.41 (Unix) with PHP 5.2.6; the result didn't change.
It seems the problem is not present in PHP5.
Comment #63
yraffah CreditAttribution: yraffah commentedI still have the same problem on Drupal 6.9 PHP 5.2.6 when I send mail through the printmail functionality of Printer-friendly pages module.
Please try it here:
http://yousef.raffah.com/printmail/node
Comment #64
yraffah CreditAttribution: yraffah commentedAnybody else is still affected by this problem except me? :(
Comment #65
jcnventura CreditAttribution: jcnventura commentedHi,
I have just used Drupal 5.15 to send a mail with subject "Le sujet de ce message est très très très long!Le sujet de ce message est très très très long!Le sujet de ce message est très très très long!" (3x the subject in #61) and it made it across OK..
I was using the print_mail module to send the message. I do notice that the contents of the message arrive as:
Anyway, if this seems to with both Drupal 5.15 and Drupal 6.9 (both on PHP5), and it didn't work with another Drupal 6.9 in PHP5, it must be something outside of Drupal.. I am moving this to fixed status, unless liquid crystal can pinpoint the problem.
@liquid crystal: does applying this patch solve the problem for you??
João
Comment #66
scor CreditAttribution: scor commentedthis is not fixed yet!
Comment #67
yraffah CreditAttribution: yraffah commentedI just tried the patch in #60 and didn't get the error message I used to get earlier, however, I will still investigate further and report back if I stumbled upon any issue.
Thanks for the tip jcnventura
Comment #68
kenorb CreditAttribution: kenorb commentedThis can be the reason: http://drupal.org/node/196792#comment-1238167
Comment #70
mdupontAny confirmation of #68?
Comment #71
Rafael CansinosLanguaje: Spanish, with letters like ñ á é í ó ú.
Drupal 6.9
PHP Version 5.2.6
The problem begins when I installed the es.po for Spanish. I think (?) before it works properly.
Now, even if I come back to English languaje the problems is there.
Comment #72
Rafael CansinosI can confirm: this issue happens to me when I installed the Spanish translation for drupal (es.po). Before it works properly.
I have solved this error by using SMTP Module: http://drupal.org/project/smtp.
See #46.
Comment #73
Bairnsfather CreditAttribution: Bairnsfather commentedI'm on Drupal 6.10 using php 5.2.8 and the problem(s) described above apply to me too. (In case it matters, I also have this on the Status page: Unicode library PHP Mbstring Extension.)
Here is the issue. When the title of a node (book page) has "curly quotes" aka "smart quotes" in it, there is an error after submitting a comment to the page:
Appears at the top of the page upon comment submission.
In the Recent Log entries (/admin/reports/dblog) there are two errors:
If I go in to the book page, and edit the title and put plain quotes in place of the curly quotes, the email is sent without an error.
Best I can tell, my specific issue is in /includes/mail.inc on line 186. But I don't know enough to know how to address the issue, except for the workaround of not using fancy quotes.
Comment #74
scor CreditAttribution: scor commented@hayworth and @Bairnsfather could you try with the patch at #60 and see if you still have the same problem?
Comment #75
josepvalls CreditAttribution: josepvalls commentedSame issue.
I've been looking at some transliteration implementation to replace those characters with either plain english or the html entities (preferred).
I'm new to drupal and not a php expert so any help would be appreciated.
Comment #76
magnjorg CreditAttribution: magnjorg commentedI too had this issue. Patch in #60 did not work. Workaround in #22 works.
Using drupal 6.12, php 5.2.6.
Comment #77
scor CreditAttribution: scor commentedrerolling
Comment #78
PetrL CreditAttribution: PetrL commentedI had the same problem. In my case it was solved by solution sugested in comment #22. (drupal 6.11 , php 5.2.5)
Comment #79
czoper CreditAttribution: czoper commented#22 + #23 works just fine and solves the problem
THANKS netbjarne!!!
Comment #80
avillager CreditAttribution: avillager commentedOne more thing. To debug this I changed mail.inc to send mails with a fixed subject of "test" and put the encoded subject in the body so I can see what's output by the function.
It is:
=?UTF-8?B?W9Ce0LHRidC90L7RgdGCINCW0LjQstC+0YLQstC+0YAg0L7RgiDQoNC+0LTQvg==?=
=?UTF-8?B?0LLQviDQodC10LvQuNGJ0LVdINGC0LXRgdGC0LLQsNC5INGC0L7QstCw?=
As you can see, the first encoded word is 76 characters long which is one more than what the RFC allows. I tried to reduce chunk to 39 and now encoded word is less than 70 characters but it doesn't help at all.
One more thing against the RFC is that multiple lines in the RFC are separated by CRLF and in mime_header_encode it is only \n
Comment #81
avillager CreditAttribution: avillager commentedI can confirm that patch from #77 works for me:
godaddy linux hosting vs PHP 5.2.8 (ask if you need more details)
drupal 6.13
The issue that encoded_word is 76chars is still worrying me though.
Thanks!
Comment #82
mics.programming CreditAttribution: mics.programming commentedPatch from #77 works also for me.
I uses Hebrew lang.,
hosting - GoDuddy.
Thanks!
Comment #83
kenorb CreditAttribution: kenorb commented#77 working fine.
Hosting: home.pl
Comment #84
Gábor Hojtsy1. Bugfixes are first committed to Drupal 7 to ensure we do not introduce regressions.
2. Dries already asked above to reference the proper RFC instead of this issue in the comment.
Comment #85
scor CreditAttribution: scor commentedrerolling just in case. The php mail function specs have changed and there is no such warning about the newlines in the subject anymore, and all it requires is to comply with RFC 2047. I'm tempted to close this issue except that some people have reported the same problem on PHP 5 (while I'm not able to reproduce it on my dev laptop). I'm leaving this open until they report back. Again, as a reminder, the problem occurs when the subject of the email is more than 46 characters and contains national characters like
Comment #87
scor CreditAttribution: scor commentedI was working on an outdated mail.sending.inc
Comment #88
jdeg CreditAttribution: jdeg commented"$chunk_size = 400;" solved my problems :D
Comment #89
Dries CreditAttribution: Dries commentedIf this problem is not supposed to happen with modern versions of PHP, we should at least add a @todo.
Comment #90
Dave ReidComment #91
LatinBoy CreditAttribution: LatinBoy commentedI have a spanish site (6.14) n' had the same problem. "$chunk_size = 400;" worked for me!!! thanks netbjarne!!!
Comment #92
klausi#87: 84883_subject_newline_d7_4.patch queued for re-testing.
Comment #94
ecedenyo CreditAttribution: ecedenyo commented$chunk_size (./includes/unicode.inc) to 400, solved my problem. PD: I got the spanish translation
Comment #95
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-rolled patch from #87.
Comment #96
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #97
MichaelP CreditAttribution: MichaelP commentedPatch at #95 [remove_linefeeds_from_subject-84883-95.patch] worked for me on godaddy hosting a non-english d7 site for user registration e-mails (contact form e-mails were working OK).
Tks @pillarsdotnet.
Comment #98
pillarsdotnet CreditAttribution: pillarsdotnet commentedBumping and tagging as per standards.
Comment #99
pillarsdotnet CreditAttribution: pillarsdotnet commentedYou can help this issue by posting a review:
Decide whether all code and comments comply with Drupal coding standards.
Decide whether any included tests are necessary and sufficient.
Decide whether the patch actually solves the problem. There should be a reproducible test where the current code fails and the patched code succeeds.
A review that affirms success in all of the above should also:
Change the issue status from "Needs Review" to "Reviewed and Tested By the Community".
Otherwise, the review should:
Explain what is wrong with the proposed patch and/or supply a corrections to it.
Change the issue status from "Needs Review" to "Needs Work".
Comment #100
pillarsdotnet CreditAttribution: pillarsdotnet commentedPerhaps this issue is doomed to failure, since there has only been one patch against Drupal core mail system that ever became RTBC or fixed.
EDIT: Correction. There have been two. The other one is a four-year-old patch against D5.
Comment #101
Dave ReidIt's a relatively new component for issues...
Comment #102
pillarsdotnet CreditAttribution: pillarsdotnet commented#95: remove_linefeeds_from_subject-84883-95.patch queued for re-testing.
Comment #103
pillarsdotnet CreditAttribution: pillarsdotnet commentedBetter title.
Since this bug results in data loss (lost emails), does it qualify as "Major" ?
Comment #104
Anonymous (not verified) CreditAttribution: Anonymous commentedThe patch supplied will break subject lines longer than 75 characters since \r\n can be inserted. The problem is actually in mime_header_encode(), the appended \n to the chunks should be \r\n as per http://www.faqs.org/rfcs/rfc2047.html.
Comment #105
Anonymous (not verified) CreditAttribution: Anonymous commentedThe patch removed a comment concerning the chunk separator because I don't see any relevance to it. All systems compliant with rfc2047 must use CRLF SPACE as the chunk separator and since we use PHP mail() function it is a requirement to follow rfc2047. The patch in #27 was almost correct but didn't follow rfc2047 either.
Comment #106
Anonymous (not verified) CreditAttribution: Anonymous commentedHere is the same patch for D7 and D6.
Comment #107
pillarsdotnet CreditAttribution: pillarsdotnet commentedPassing the torch to earnie.
Somebody who has gained more respect than I (@sun? @Damien Tournoud? @Dave Reid?) should review the patch.Also, it should come with tests.EDIT: Webchick says she'd accept an RTBC from scor. And instead of "gained more respect", I should have said "established a pattern of being really knowledgeable about this area of code."
Comment #108
Anonymous (not verified) CreditAttribution: Anonymous commentedAdd the test is a different issue and one I don't have time to do. The patch is simple and if the testing doesn't exist it should be handled separately IMO.
Comment #109
pillarsdotnet CreditAttribution: pillarsdotnet commentedThe usual practice for urgent fixes is that the fix is committed first, then the issue changes back to "needs work" while tests are added. I'd be willing to write tests myself, but first I want to see a review from someone who is qualified to get this thing approved. I'd mark this issue RTBC myself except that I'm absolutely sure that webchick would bump it back to "Needs review" with a note that a core developer should look it over.
Comment #110
noomz CreditAttribution: noomz commented"$chunk_size = 400;" worked for me too. Thanks so much
Comment #111
emmene-moi CreditAttribution: emmene-moi commentedmime_header_encode should not encode Content-type definition... even if there's CRLF.
Why?
multipart/mixed; can add CRLF for boundary definition.
Check: http://www.w3.org/Protocols/rfc1341/7_2_Multipart.html
Thanks!
Comment #112
pillarsdotnet CreditAttribution: pillarsdotnet commentedPlease don't change the version.
Comment #113
Anonymous (not verified) CreditAttribution: Anonymous commentedI don't see this as having anything to do with the change purposed in #106 which has to do with encoding and not mixed/multipart. In fact the two can co-exist in the same email, i.e. I can encode a multipart part.
Comment #115
apadernoComment #116
Devin Carlson CreditAttribution: Devin Carlson commentedMarked #1209018: From: in headers not properly encoded as a duplicate.
Comment #118
Anonymous (not verified) CreditAttribution: Anonymous commentedThis issue is 6 years old and getting older. The patch is simple. Should we bump to major just to get a few core maintainer eyes this way?
Comment #119
klausiNo, it would be better to provide a working patch first + tests + reviews from people that set it to RTBC.
Comment #120
apadernoComment #121
Anonymous (not verified) CreditAttribution: Anonymous commentedThe problem exists that the RFC 2047 is not followed. Per PHP documentation this is the RFC that they use. Therefore Drupal is broken.
Comment #122
klausiNo patch for D8 here, so needs work.
Comment #123
apaderno"needs work" is used when a patch has not been provided, or when the current patch needs to be changed. In this case, there isn't a patch that applies for Drupal 8.
"needs review" would say to the users "there is something to review," which is not true as there isn't a patch that applies to the current Drupal version.
Comment #124
Anonymous (not verified) CreditAttribution: Anonymous commentedSee #84883-105: Unicode::mimeHeaderEncode() doesn't correctly follow RFC 2047
Comment #125
klausi#105: mime_header_encode-84883-105.patch queued for re-testing.
Comment #126
Anonymous (not verified) CreditAttribution: Anonymous commentedRerolled patch.
Comment #127
apaderno@earnie That proves my point: The test bot was not able to apply that patch; ergo, that patch needs to be re-written. "needs work" is the right status, in such case.
If somebody submitted a different patch, then "needs review" should be the correct status; differently, the test bot would not test the new patch.
Comment #128
mzwyssig CreditAttribution: mzwyssig commentedIs this going to be ported to D7 ?
Comment #129
pillarsdotnet CreditAttribution: pillarsdotnet commented@#128 -- That's the plan. See http://drupal.org/node/1207020#needs-backport-to-d7
Comment #130
kscheirerWe're going to need a test to prevent future regression, marking as "needs work". If you don't have time to write the test (totally understandable), then just describing how to test it would be very helpful. If it's simple enough, we might even be able to put a Novice tag on this and get a new test-writer to take a crack at it. The patch itself looks sane enough.
Comment #131
Anonymous (not verified) CreditAttribution: Anonymous commentedYes, I agree that we need a test but that can be a separate issue. This is a long standing bug that never had a test before and it needs to get fixed regardless of the testing. The RFC specification is clear and Drupal is not following it.
Comment #132
kscheirerWell, you can try for sure! :)
For the unit test: call mime_header_encode() and then verify the chunk size (45) and the line endings are \r\n instead of just \n. Does that sound about right?
Comment #133
bgm CreditAttribution: bgm commentedI had a few issues related to this, and the patch fixed it for me. Thanks!
Tested on D6 and D7.
Comment #134
catchYes this needs a test before it can go in per #130.
Comment #135
stewart.adam CreditAttribution: stewart.adam commentedNote: for those that tried the patches in this issue and still have the 'bad parameter' error, you may want to see #300387: drupal_mail_send with long UTF-8 subject puts \n to subject line, the mail() function won't send it then..
Comment #136
DuaelFrPatch rerolled and test attached (plus a tiny comment improvement)
Comment #138
DuaelFrFixed a double space issue in the separator.
It seems to conform to the RFC's specs and examples now.
Comment #139
roderikDone:
Leaving Needs Review for those changes.
Comment #140
roderikSo yeah, the code.
Comment #141
DuaelFrThank you Roderik. It's a lot more understandable now :)
Comment #142
roderikHm. One slightly scary thing.
The issue summary now mentions that the restriction of php's mail() function not being able to handle subject lines containing \n, is gone since 2009.
But #300387-13: drupal_mail_send with long UTF-8 subject puts \n to subject line, the mail() function won't send it then. reports (one specific FreeBSD hoster, not FreeBSD in general) not being able to handle subject lines containing \n.... and also the patch (which replaces things by \r\n) not working.... in 2013.
The patch does not make much of a difference (it changes from "not working" to "not working") but also removes the comment that basically says "you may have to hack things yourself, on your own system".
Comment #143
DuaelFrI don't know which version of PHP fixed that bug but, even if it's not supported by Drupal anymore we should add a comment that explain what happens.
Comment #144
roderikThat PHP version is very old. I guess you're still right though. It's a bit hard to decide what to say exactly, but I added a blurb.
Also: my 'formula/calculation' was completely bogus. I guess I finally get it now, and also get what was wrong with the comment (from july 2005):
It's not as the comment basically says
but should be
Comment #145
DuaelFrThat looks good to me.
The bug fix respects the RFC, includes tests and is well documented now.
Comment #147
roderikSeems the fail in DownloadTest (returning a 403 in the 2nd test run of 3) was a hiccup.
Comment #149
roderikAfter 4 days of succesful tests Jenkins says "aborted' (not "failure"). Assuming temporary hiccup again.
Comment #150
alexpottIt'd be could to have a test with \n in.
Linking to an issue and saying reporting there if there are issues is interesting documentation. I'm not sure that this is the correct instruction or even if we should be making an instruction at all.
Why remove this test? I don't think there is any harm in keeping this text to prove that the changes have not changed this.
Comment #152
DuaelFrNew patch according to #150
Comment #153
DuaelFrComment #155
DuaelFrRetesting in progress.
Let's try to awake this issue a bit :)
Comment #156
DuaelFrReuploading the patch to run the tests against 8.2.x
Comment #157
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedAdding #2822334: Unicode::mimeHeaderDecode() doesn't support lowercased encoding as related.
Comment #160
roderikRe #150.2 removing the link to the other issue in comments is fine. I just think that my sentence "there have been occasional problem reports" gets strange then (because we effectively decide to ignore them. Which IMHO is fine). So I reworded that to an assertion about what the code does.
Sorry for still making a change so I can't officially set RTBC. Changes since #152:
(Changes since alexpott last saw it in #150: the interdiff in #152 + the interdiff in this comment.)
Comment #165
jungleThe current dev branch is 9.1.x.
Comment #166
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commentedRe-rolling patch for 9.1.x.
Comment #168
jungleComments could be transferred to keys. For example:
Comment #169
TR CreditAttribution: TR commentedThat's a real test fail in #166 - it happens because this patch changes the chunk size from 47 to 45. That matters in the failed test because mimeHeaderEncode() is called with $shorten = TRUE in the test, which truncates the string before encoding. When you truncate at a different place, the expected return value is different. The failed test needs to be fixed in this patch.
Also, the shorten option is not tested - a test is needed to ensure it does the right thing.
Comment #171
larowlanComment #172
kim.pepperTaking a look.
Comment #173
kim.pepperRan out of time.
Comment #174
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedShould we update the expected string in tests or the mimeHeaderEncode() ?
Comment #175
TR CreditAttribution: TR commentedHere's a new patch. Changes from #166 are:
Note that this patch also applies to 9.1.x, so it can be safely committed to that branch as well.
Comment #176
TR CreditAttribution: TR commentedAgain, with coding standards fixed.
Comment #177
quietone CreditAttribution: quietone as a volunteer commentedCan't this just say, 'Encodes MIME/HTTP headers.'?
It is CRLF and the 'we split' sounds wrong when we are complying with an RFC. I suggest, '- According to RFC 2047, long lines use CRLF SPACE and the separator.'
Do we need to keep a comment that refers to changes in PHP made 11 years ago? I don't think so, and it is not adding anything that helps understand the code as it is now. I think this can be deleted.
I think that => here means 'therefor' here? If so, let's use therefor instead of two characters. And for the final equation add the solution so that it is even clearer, 'floor(63 / 4) * 3' to 'floor(63 / 4) * 3 which is 45'.
Let's be consistent with other comments and use CRLF here.
Why is setting $shorten to FALSE needed? As the comment says it is the default. Is there some reason to think that the mimeHeaderEncode will not work if this is not set? There are other examples of this in the file that do not set the default. I'd say remove it.
Comment #178
ayushmishra206 CreditAttribution: ayushmishra206 at OpenSense Labs commentedI have addressed #177.1,2,3 and 4 in this patch. Attached is an interdiff to reflect that, please review.
Comment #179
quietone CreditAttribution: quietone as a volunteer commentedPatch didn't pass core/scripts/dev/commit-code-check.sh .
Comment #180
apadernocore/scripts/dev/commit-code-check.sh is checking the word spelling. Two failures are caused by CLRF, which has been reported in comment #177 as typo for CRLF.
Comment #181
quietone CreditAttribution: quietone as a volunteer commentedLet's get this moving again.
Making the changes to pass commit-code-check.
Comment #182
quietone CreditAttribution: quietone as a volunteer commentedWhat I hope are improvements to commetns.
Comment #183
longwaveInstead of explaining it we could just do the calculation in PHP, ie.
Comment #184
apadernoI would not replace
$chunk_size = 45
with$chunk_size = floor((75 - strlen("=?UTF-8?B??=")) / 4) * 3;
but I would rather replace the calculation shown in the comment with that.Comment #185
Kristen PolMoving back to needs work.
Comment #187
jungleChange made per #184
Comment #188
longwaveLooks good - the tests are comprehensive and it will be nice to close a 14 year old bug!
Fixing the title as the original function is long gone.
Comment #190
alexpottWe should work on deprecating our mime encoding and use https://www.php.net/manual/en/function.iconv-mime-encode.php instead. We've got Symfony's polyfill in place so this code is not that useful. Also the implementation is still incorrect as you can't actually get the length correct unless the function also takes the field name. PHP's built-in mb_encode_mimeheader() suffers from the same problem which is why the Symfony polyfill does
or perhaps even better we should be using Swiftmailer and leave all this complexity to someone else.
The big problem is that in order to correctly mime encode a header and set it to the correct length we need to know the length of the field name as that is included in the length. Compare:
The important part of the RFC is:
The line includes the field name.
Comment #191
quietone CreditAttribution: quietone as a volunteer commented@alexpott, thanks for the review.
So, this should either work on switching to use https://www.php.net/manual/en/function.iconv-mime-encode.php or use SwfitMailer. There is an issue in Drupal core ideas project, that started as a move to SwiftMailer but is now about using the Symfony mailer #1803948: [META] Adopt the symfony mailer component. It is not yet an approved plan though. Not sure what the next step here is.
If it were up to me I would postpone this and put effort in getting a decision on moving to Symfony mailer and only fix critical bugs in the current mail system.
Changing to NR for ways to keep this moving.
Comment #192
quietone CreditAttribution: quietone as a volunteer commentedAsked in #bugsmash for ideas on what to do next here. longwave responded with "hard to say when both issues have been open a very long time!" I couldn't agree more.
In an effort to make a decision I decided to see what breaks if iconv_mime_encode is used. I wonder what will break?
Comment #193
alexpott@quietone this needs a bit more than direct replacement in the Unicode::mimeHeaderEncode() code. I think we need a completely new method that takes the field name and value. The output of
iconv_mime_encode()
includes the field name because the only way you can get the length correct is by including the field name in the encoding.I think maybe we can explore swapping out some of our functionality for symfony/mime and then build on that to use symfony/mail. In the mime package there is the Symfony\Component\Mime\Header namespace full of stuff we should be using.
Comment #194
quietone CreditAttribution: quietone as a volunteer commentedArg another bad fix. I think I will give up.
Darn, I didn't see #193 before I made the coding standard fix.
@alexpott, I didn't expect this to be a good solution. Frankly, I was just curious to see what breaks and since at least one mail related test fails locally when on HEAD I can't find out otherwise. But it seems I am just making bad MRs right now so I should stop :-) I'll see if I can make sense of Symfony\Component\Mime\Header namespace.
Comment #195
alexpottSo here's how I think we can leverage Symfony's mime classes without having to re-write the mail sub-system and solve a few mime encoding bugs. I do agree with others that want to replace some of the Mail ArrayPI with objects from Symfony mail and mime but this is a small step in that direction which solves bugs we have now and deprecates APIs that don't conform to the RFCs and can't. No interdiff because a new approach.
Comment #196
alexpott#195 incorrectly adds the subject to the headers... fixing that.
Comment #197
longwave+1 to the approach in #195.
I was looking at this earlier and wondered if this is actually correct. Is a Content-Type ever a non-ASCII string? Why do we do this here, for this header only, and nowhere else in our HTTP output? Shouldn't Symfony handle this encoding if it's necessary?
edit: traced the source of this line back 15 years to https://git.drupalcode.org/project/drupal/-/commit/0f6067fc849f749ca4356... but that doesn't help explain why it's needed or if it's actually correct nowadays.
edit 2: https://tools.ietf.org/html/rfc2047 says
so I think this should be removed, let's do that in a separate issue.
Comment #199
alexpott@longwave fwiw I think we can fix that before or after - the change included here is a like-for-like.
Comment #200
alexpottWe don't have a direct dependency of symfony/mime in HEAD - even though we should as we use it already in core. See #3207456: Drupal 9 is dependent on symfony/mime directly
Comment #202
quietone CreditAttribution: quietone as a volunteer commentedHow ironic that the two failing tests are migration tests. So, I took a look and learned that \Symfony\Component\Mime\Header\Headers::addHeader will call other methods and they expect the data to be of particular types. And sure enough converting the header data to an array when needed allows the tests to pass. I just used a simple switch statement for the header types that will use \Symfony\Component\Mime\Header\MailboxListHeader. I've posted the work but not testing because it doesn't cover all possible header values and there is probably a more robust way to handle the problem.
Comment #203
alexpott@quietone based this on your work in #202. There's not much we can do nice here... but we can assume that comma is the expected separator and trust \Symfony\Component\Mime\Address::__construct() to trim as appropriate and \Symfony\Component\Mime\Address::createArray() to work out which bit is an address and which is the name.
Comment #204
alexpottI used a really really long site name full of greek characters. It generated the following headers. The longest line is 75 characters (not including the carriage returns)! Yay.
On head this header looks like:
Where we are shortening the sitename - even though we don't actually need to... but even with the shortening the line length is 102 not including the carriage returns
Comment #205
alexpottSo one problem with #203 is that we're now mixing line endings since Symfony obeys the RFC line endings ie "\r\n" internally but then will replace these with \n when using the sendmail transport (see https://github.com/symfony/symfony/blob/5.x/src/Symfony/Component/Mailer... ). So we need to do the replace too and keep using \n - there's some discussion of this fun here - https://stackoverflow.com/questions/4415654/which-line-break-in-php-mail....
Comment #206
quietone CreditAttribution: quietone as a volunteer commentedUpdated proposed resolution and added examples to the change record.
Comment #207
quietone CreditAttribution: quietone as a volunteer commentedFound and closed two duplicates,
#3163492: Emails "From" name gets cut if Cyrillic site name has more than 24 letters
#2407117: Unicode::mimeHeaderEncode() ignores header field name
Comment #208
dwwThe new approach looks very promising! Thanks to everyone who's worked on it.
A few final concerns / nits before RTBC:
Slightly confused by this deprecation test. On a quick glace, it seems like calling
Mail::formatDisplayName()
is supposed to trigger a different deprecation error. Probably outside the patch context,Mail::formatDisplayName()
ends up callingmimeHeaderEncode()
, but it seems a bit less magical to check for the direct deprecation message about the function we're calling, not relying on "side effect deprecations" like this.Can/should we expected both deprecation errors? If not, I think I'd prefer testing for the explicit deprecation we added for that method...
Does this want to explode with
', '
(with the trailing space)?I need to re-read all the recent comments to understand why these changes in behavior are now expected and legit. ;)
Otherwise, looks great! Test coverage seems pretty solid.
Thanks again,
-Derek
Comment #209
quietone CreditAttribution: quietone as a volunteer commented#208
1. Yes, that does seem odd. I've changed the deprecation message in the test.
2. RFC 5322 just says a comma separated list.
3. I wish you happy reading.
Comment #210
longwaveRe #208.2/#209.2 if so do we need to
trim()
the exploded values before passing toaddHeader()
?Comment #211
alexpott@longwave - nope we don't need to do that. Ensuring the address matches the RFC spec already happens in \Symfony\Component\Mime\Header\MailboxListHeader and \Symfony\Component\Mime\Address::__construct() - there's a trim in there...
Comment #212
alexpottRe #209.3 these changes are because we've moved from base64 encoding to Q-encoding - both are valid for mime headers and because we now correctly support long display names in email addresses. As you can see one other benefit from moving to the Symfony component is only the necessary words are encoded and more is left in plain ascii which makes debugging and reading raw emails much simpler.
Comment #213
longwave> one other benefit from moving to the Symfony component is only the necessary words are encoded and more is left in plain ascii which makes debugging and reading raw emails much simpler
+1 to changing it for this alone, I'm sure I've run into this before when trying to debug email output.
All nits were addressed. I also opened #3207476: file_get_content_headers() does not need to encode the Content-Type header as a followup to #197.
This both solves the problem at hand (after 14 years!) and also removes our custom implementations of something handled better by third party code, so to me this is RTBC.
Comment #214
dww@quietone: Thanks for fixing the deprecation test, definitely looks better now.
@alexpott: Thanks for the explanation re: #209.3, makes sense.
@longwave: Agreed on RTBC!
Thanks everyone,
-Derek
Comment #215
longwaveNeeds a reroll as #3207476: file_get_content_headers() does not need to encode the Content-Type header got in first.
Comment #216
alexpottRerolled - considering we're only removing change - back to rtbc.
Comment #219
catchRestoring status after HEAD was broken.
Comment #220
larowlanI've asked if it's too late for this to go into 9.2. If it is, we'll need to re-roll so the deprecation messages say 'in 9.3' instead of 'in 9.2'
Will report back.
Comment #221
larowlan@catch and @effulgentsia were ok with this going into 9.2
Updating issue credits
Comment #223
larowlanCommitted ab49a62 and pushed to 9.3.x. Thanks!
Backported to 9.2.x
To the Bug-Smash folks and TR who breathed life back into this 5-digit NID issue from 15 years ago and alexpott for the final solution 🏆 - thank you
Published the change notice.
Comment #225
super_romeo CreditAttribution: super_romeo commentedRelated issue: Call to undefined method Swift_Mime_Headers_UnstructuredHeader::getAddresses()
Comment #226
super_romeo CreditAttribution: super_romeo commentedComment #227
tsuyopin CreditAttribution: tsuyopin commentedHow to apply a patch to Drupal 9.2.6 ?
I could not apply "84883-216.patch" at #216.
"composer"
composer require cweagans/composer-patches:~1.0 --update-with-dependencies
"composer.json"
"composer"
Comment #228
mdupontIt's already included in Drupal 9.2.6, it has been committed to the 9.2.x branch 4 months ago, nothing to do on your side.
Comment #229
tsuyopin CreditAttribution: tsuyopin commentedThanks, mdupont.