Troubleshooting link instead of connection error message

[AndreKR]

Whenever an SMTP connection error happens, a troubleshooting link is returned instead of the error message.

Some example code:

$phpmailer = new PHPMailer(true);
$phpmailer->isSMTP();
$phpmailer->Host = 'thishostdoesnotexist';
$phpmailer->addAddress('info@example.com');
$phpmailer->Body = 'hello';
try {
	$phpmailer->send();
} catch (Exception $e) {
	echo $e->getMessage();
}

This will print

SMTP connect() failed. https://github.com/PHPMailer/PHPMailer/wiki/Troubleshooting

That link contains many many paragraphs on how to manually diagnose what could be wrong with the mail server.

The thing is: There would be no need for guessing and manual troubleshooting. The exact cause of the connection error is known, PHPMailer just deletes the error information. If PHPMailer would return the actual error message there wouldn't be any need for guessing and manual troubleshooting.

You can see that if you set a breakpoint here:

PHPMailer/src/PHPMailer.php

Line 2089 in 640e68d

$this->smtp->close();

then you can still read the error message:

But when the $this->smtp->close() runs, it clears the error message.

A similar problem happens when the $this->smtp->startTLS() (a few lines above) fails, then the $this->smtp->quit() runs and it also clears the error message.

By the way, there is actually a function (PHPMailer::setError()) that can read the error from the SMTP object and build a single error message from it. But that function is never called before the error gets deleted.

[Synchro]

I think the right way to deal with this would be to make the error status an array that accumulates error messages. Because the SMTP commands are independent, and a single bad response is not necessarily fatal, the idea behind the current approach is that commands can continue, and their status should be reflected in the stored error message. I don't think trying to fit all of them into a single message is particularly helpful. The problem then though is that this would be a BC break. The vast majority of the time errors are encountered when first configuring a connection, and in that situation the debug output includes error responses from every command; enabling that is also the first thing the troubleshooting guide recommends doing. Got any better ideas?

[AndreKR]

What would be an example of an SMTP error that is not fatal? Sure, there are things like "try EHLO first, then try HELO" but IMO that's a single step and only when both fails that counts as an error, and yes, in this case the error message gets complicated ("SMTP greeting failed: EHLO error: ... HELO error: ..."). But for most of the commands there seems to be a clear notion of "this failed" and I don't understand the weird mix of throw, try/catch and return true/false in smtpSend(), smtpConnect() and SMTP->connect() at all.

So unless I'm missing something my idea would be to

• keep the happy path at the left
• remove all if (doSomething()) "error handling"
• identify the steps where something can go wrong and throw an exception there
• If the old &$error API is enabled, catch that exception at the topmost level and put its message into &$error.

On a side note, personally I would also prefer a structured error. That's not possible with the old &$error API, but with exceptions extra fields can easily be added. I think it's ok if you only get the full error info when you enable exceptions (and maybe enable them by default for 7.x).

[Synchro]

I wrote the original exception handling many years ago, and the maintainer at the time really didn't understand how exceptions worked, and made a complete mess of integrating it (introducing those weird stop/continue codes), leading us to this unholy convoluted mess. The link to troubleshooting isn't instead of an error message, it's as well as; it's simply appended in that one common case. There are many parts that need a radical rebuild, but none of it can happen without major work and redesign, along with all the BC breaks & docs that they imply. In the mean time, it works ok in practice – if it breaks, enable debug and see what's going on. There are other libraries that take a more complex approach that do provide finer-grained exceptions (like SwiftMailer's 450 classes), but I feel we need something simpler yet modern. I'm exploring better ways of doing this in my DKIM Validator project, and much of the code in there might make a good basis for a major revision of what's in PHPMailer, particularly the representation of MIME parts, headers, and bodies.

[AndreKR]

The link to troubleshooting isn't instead of an error message, it's as well as

It might be intended to be in addition to the error message, but since the actual error message never makes it out of SMTP and into PHPMailer, all you get is the link.

none of it can happen without major work and redesign, along with all the BC breaks

To me it seems the trouble is entirely contained inside smtpSend(), smtpConnect() and SMTP::connect(), so unless you consider the actual contents (or lack thereof) part of the public interface, I believe this could probably be rewritten without breaking BC.

if it breaks, enable debug and see what's going on

Yeah, I figured something like that. I wrote a wrapper and I'm now calling PHPMailer like this:

class MailerException extends Exception {
	private $debug_log;
	public function __construct($message, Throwable $previous, array $debug_log) {
		parent::__construct($message, 0, $previous);
		$this->debug_log = $debug_log;
	}
}

$error_log = [];
$mailer = new PHPMailer(true);
$mailer->Debugoutput = function ($str, /* @noinspection PhpUnusedParameterInspection */ $level) use (&$error_log) {
	$error_log []= $str;
};
$mailer->SMTPDebug = SMTP::DEBUG_CONNECTION;
try {
	...
	$mailer->send()
} catch (Exception $e) {
	throw new MailerException($e->getMessage(), $e, $error_log);
}

[Synchro]

The link is literally appended to whatever error message is present. Any issue you have with the way error messages are handled has nothing to do with the link, the addition of which probably halved the number of support requests.

Doing that kind of thing with an injected callable in Debugoutput is exactly what I intended it for, so that's cool.

While I know that error reporting could be improved (see also #1579), I'm not going to do a major overhaul of internal error handling within the current structure, and you seem to have something workable here, so I'm closing this. PRs welcome of course!