Till now, Drupal is blind to e-mail sending errors, giving serious trouble to site-admin under certain conditions. It's also inconsistent, as "Request new password" is logging errors properly, but no other mail-sending occasion do.

Bug description: A long-standing bug (I encountered exactly the same on D 4.7, 5 and 6-dev).
How to reproduce: Fresh D6-dev install. Configure your php, so that mail() fails and returns FALSE. Register a new user.
Expected behaviour: Error message to both user and log.
What happens instead: Fails silently, claims that the mail was sent successfully.

Background and why this is important: Real situation on production site, half a year ago: The webhosting provider blocked outgoing mail, due to false spam-suspicion (contact module problem on 4.7). Site was up for quite a while, so we didn't keep an eye on it too closely, and the logs gave no hint that e-mail stopped working suddenly! New users were registering, contact forms sent, logs clean, but no e-mails arriving. Complaining users were silenced by contact module's mails failing the same way. It took us a month on low-traffic site to notice that no new users were logging in, and so realize that something was wrong. Sorting the suspicion with hosting provider took half a day, but publishing apologies and attracting lost users back was much greater problem.

Possible solutions: Excepting the new-password case, there's nothing to catch false result of drupal_mail() and notify the admin. The necessary code might be added to either:

--- The drupal_mail() function itself.
pros: Reliable error-logging provided for all mail-sending occasions at once, including contrib. Easy implementation on just one place, not much new code and patched files.
cons: Contradictory messages in the case of error, unless other patches done.

--- To all places in the code (both core and contrib) where drupal_mail() is called. This basically means to follow suit with the way "new-password" does it.
pros: More specific error messages.
cons: Huge number of places to patch, makes a call to drupal_mail() slightly complicated.

Proposed solution: I've chosen the first alternative, i.e. logging inside drupal_mail(). I think that error-logging should be a part of the service provided here, and should not be required from every single module using the function. (There are a lot of code-locations calling drupal_mail() to be patched otherwise, including several occurences in user.module, contact.module, and different contrib modules.) My approach also means less new code and less probability of bugs in contrib drupal_mail() call implementations.
The contradictory messages (i.e. new "Unable to send e-mail" message followed by original "E-mail sent") seems to me temporarily acceptable, as this happens only upon serious site-environment problems - on most sites users will never see it. This may be corrected by another low-priority patches in future, if seen as necessary; important part now is to get the error-logging right. Another solution would be to remove error-message to the user, and keep only logging - but I feel that it's fair to let the user know, when his operation failed.

The patch: The patch provides error-reporting to both user and log. In the log, sender and recipient mail addresses are included, to allow site-admin to contact the unhappy user, if he wants to for some reason. Both addresses are included, because drupal_mail() can't know which one is the real user's address ("to" upon registration, "from" on site-wide contact form, and so on...). The operation, where the mail failed, is already sufficiently identified by logged URL of the page.
Since this patch is basically moving the existing check from "new-password" to a more general level, another patch follows to remove it from its original place in turn. This makes the drupal_mail() calls seen around core modules consistent.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JirkaRybka’s picture

Attaching the second patch, removing the check from abandoned original location.

Also marking my own older 5.x bug-reports as duplicates:
http://drupal.org/node/159093
http://drupal.org/node/159094

Dries’s picture

I agree with the approach taken, and the patch looks OK except for the fact that drupal_set_message() should probably set an error message (pass 'error' as the second parameter) instead of a success-message. Feel free to merge both patches into one.

The return address of mail() isn't reliable -- it doesn't always return FALSE on failure. See PHP documentation. This might be worth pointing out at the appropriate place in the PHPdoc.

Otherwise looks good. :)

JirkaRybka’s picture

FileSize
1.24 KB

Thanks for comment, good points.

I added the 'error' message status and notice about success/failure meaning - attaching new patch. (Relying on the previously existing code, I overlooked it.) Please check the notice added to @return description, I'm not a native English speaker...

As for merging the patches together - I certainly would, if I knew how to do so safely. My apologies here, but I'm rather new to patches, currently using only just a 'diff' on my local files, as provided out-of-box in my Linux desktop. I'm going to learn more soon, but for time matters with Drupal6 progressing I hope you can accept my patch as two-part for now. Thanks in advance.

Dries’s picture

Status: Needs review » Fixed

Committed both patches to CVS HEAD. Thanks! :)

Anonymous’s picture

Status: Fixed » Closed (fixed)