Fixed "emails are displayed incorrectly" with PHP 8.1+

[fballiano]

This PR should fix issue #274 and issue OpenMage/magento-lts#3174 in OpenMage.

EDIT: this PR ports the fix used by laminas in https://github.com/laminas/laminas-mail/blob/2.23.x/src/Transport/Sendmail.php#L142-L147

[kiatng]

Tested on PHP7.3 and PHP8, Linux.

[addison74]

I used DDEV and as PHP versions 7.4, 8 and 8.1. No issuess occurred. What was reported related to OpenMage is now resolved.

[glensc]

Is this fixed in zendframework 2 and/or laminas the same way?

[fballiano]

i've no idea, I don't know much about those two

[glensc]

those are both successors of zf1, so they might have fixed this too.

this may safe change, i.e change for all users, could break some other agents:

• Wrong line endings with PHP 8.1 and nullmailer PHPMailer/PHPMailer#2870

you probably should set the property yourself, as it's public property,
unless you find they have fixed it same way or some rfc supports that \r\n must be used or PHP 8.1 own docs?

[fballiano]

since multiple people in #274 are facing this problem I think it should be fixed at a framework level anyway.

[fballiano]

There's also this: php/php-src#8086

[kiatng]

I know nothing about Laminas, but the fix is about the same. First, the EOL const is defined same as this PR:

https://github.com/laminas/laminas-mail/blob/2.23.x/src/Headers.php#L51

Second, it is converted to "\n" for non-WIN OS:

https://github.com/laminas/laminas-mail/blob/2.23.x/src/Transport/Sendmail.php#L142-L147

This PR doesn't check for WIN OS and PHP ver.

[edit] But when testing PHP 7.3 and 7.4 in Linux, the fix in this PR works. So, this PR seems good.

[glensc]

since multiple people in #274 are facing this problem I think it should be fixed at a framework level anyway.

this not a good excuse. because if this is a breaking change the rest of the users start to complain for whom it worked before.

[fballiano]

did you check php/php-src#8086?

I see the worry about the possible BC, but not doing anything is BC for an opposite equal amount of people...

[fballiano]

would you like a PR which is more similar to laminas?

[addison74]

@glensc - My personal opinion is that ZF1-Future must have the source code related to this issue similar to Laminas-Mail. It is obvious that this change is necessary, but the one in Laminas-Mail covers all the issues that could arise with its adoption.

[glensc]

I've read along:

• Mail() function not working correctly in PHP 8.x php/php-src#8086

mostly people are frustrated by the change, but PHP devs say \r\n is standards conformant. but really standards is for smtp saying must be \r\n terminated, no standard how one should talk to /usr/lib/sendmail.

perhaps introduce similar behavior to:
-php/php-src#10191

i.e respect mail.mixed_lf_and_crlf in setting, so if that option is set, go back to previous behavior otherwise go with \r\n.
must consider that the option may be missing on older PHP versions.

not sure what is "similar to laminas", you just linked to a line. maybe commit or pull request making the change has more context?

[fballiano]

Most similar to laminas i mean the code posted by @kiatng obviously.

I’m doing it if you’ll consider it, otherwise I’ll check other solutions internally, sorry but time is precious for everybody.

[glensc]

phpmailer seems has taken totally different approach. tells to use smtp rather sendmail driver :)

• Wrong line endings with PHP 8.1 and nullmailer PHPMailer/PHPMailer#2870 (comment)
• Always use CRLF if php version is 8.0 or newer PHPMailer/PHPMailer#2188 (comment)

[glensc]

php mail() and mb_send_mail() also say (now) to use \r\n, and as last result fall back to \n:

• https://www.php.net/manual/en/function.mail.php
• https://www.php.net/manual/en/function.mb-send-mail.php

[glensc]

Seems checking mail.mixed_lf_and_crlf is not a viable solution. it's not present in any release PHP version

php -d mail.mixed_lf_and_crlf=1 -r 'var_dump([ini_get("mail.mixed_lf_and_crlf"), PHP_VERSION]);'        
array(2) {
  [0]=>
  bool(false)
  [1]=>
  string(5) "8.2.3"
}

See also:

• https://stackoverflow.com/questions/70502065/php8-mail-function-adding-space-prefixes-to-headers/75317793#75317793
• Mail() function not working correctly in PHP 8.x php/php-src#8086 (comment)
• Fix GH-8086: Introduce mail.mixed_lf_and_crlf INI php/php-src#10191
• https://www.php.net/manual-lookup.php?pattern=mail.mixed_lf_and_crlf&scope=quickref - docs not found

[glensc]

@fballiano laminas way is the recommended solution, I said as first note here:

• Fixed "emails are displayed incorrectly" with PHP 8.1+ #344 (comment)

likely battle-tested by them as the changes already seem 3yr old.

but it may be more changes than just those lines printed here:

• Fixed "emails are displayed incorrectly" with PHP 8.1+ #344 (comment)

[fballiano]

I rewrote the PR, not sure about the $fromEmailHeader, which shouldn't contain an EOL anyway...

[glensc]

What abuot EOL constant? in zf2/laminas it's there since 2012:

• zendframework/zendframework@5f899d3#diff-cd2e235ede76dad36d06a755991777edb55781b2fc4321c74fb7d17605522834R45

Describe your changes and thinking logic in PR body.

[fballiano]

I see no other EOL constant in the Zend/Mail folder.

[addison74]

I guess @glensc wants to use self::EOL instead of "\r\n", like here

zendframework/zendframework@5f899d3#diff-cd2e235ede76dad36d06a755991777edb55781b2fc4321c74fb7d17605522834

[glensc]

Indeed, somehow I looked only the new commit, and assumed it was force pushed, but the original commit is still here:

• 48ad3c3 (#344)

[fballiano]

I’d keep it this way in case somebody extends the class. I think it’s the same thought the laminas people did

[fballiano]

any news on the merge of this PR?

[develart-projects]

@fballiano I'll review that in comming days, now focusing on the bugfix release.

[develart-projects]

Checked all information presented, I think we are good to go here.