-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add DSN mail option #2157
base: master
Are you sure you want to change the base?
Add DSN mail option #2157
Conversation
It can be used to support the section 4.3 and 4.4 regarding RET={FULL|HDRS} and ENVID=xyzIDxyz of the RFC: https://tools.ietf.org/html/rfc3461
While this could be useful for some, I see a few issues with this implementation.
|
Regarding 1, it follows the same design than the support of recipient() https://github.com/PHPMailer/PHPMailer/blob/master/src/SMTP.php#L912 ; RFC6533 is a work to be done, but it shall be part of another pull request serie. Regarding 2, you are right, I'll think about a better API, close to the support of $dsn (https://github.com/PHPMailer/PHPMailer/blob/master/src/PHPMailer.php#L375). Regarding 3, of course it is widely supported for a while. On the sender side, for instance, postfix does support it too: https://manpages.debian.org/testing/postfix/sendmail.1.en.html |
Regarding a DSN/ENVID reference, see: https://github.com/mozilla/releases-comm-central/blob/master/mailnews/compose/src/nsSmtpProtocol.cpp#L149 |
Another consideration – the DSN extension to MAIL FROM should only be used if the DSN extension is listed in the server's capabilities, otherwise you're likely to have problems with servers that don't support DSNs. You can check this by calling |
The previous commit added the support of DSN thru a loose support of DSN for the MAIL FROM command. This commit checks that only FULL|HDRS and the optionaly ENVID can be used which prevent any wrong syntax from the users.
WIP with this previous commit. I'll provide an update using getServerExt() |
Only DSN capable servers should get the optional MAIL FROM commands. Suggested-by: Marcus Bointon (@Synchro)
@Synchro thanks for your comments. See enclosed the updates of this pull request. |
Dam'd ! I forgot php-cs-fixer, hold on please. I'll post an update. |
Apply php-cs-fixer --diff --dry-run --verbose fix
done |
Add some basic unit tests of the DSN run: php dsn.php
@Synchro : please do you have some comments about this serie ? I included the features you requested. thank you, |
Please, do you have some comments ? |
src/SMTP.php
Outdated
@@ -947,7 +947,7 @@ public function mail($from, $ret = '') | |||
$useVerp = ($this->do_verp ? ' XVERP' : ''); | |||
|
|||
$useRet = ''; | |||
if (!empty($ret)) { | |||
if ($this->getServerExt('DSN') && !empty($ret)) { | |||
$useRet = self::dsnize($ret); | |||
if (!is_string($useRet)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to make this more explicit:
if ($useRet === false)
Sorry, lots of other things going on. For the tests, could you reformulate that as tests within the test suite (in |
OK, I'll give a try by end of the week. |
It can be used to support the section 4.3 and 4.4 regarding
RET={FULL|HDRS} and ENVID=xyzIDxyz of the RFC:
https://tools.ietf.org/html/rfc3461