skip mail_admins/mail_managers if ADMINS or MANAGERS is empty

[Jesse Young]

As discussed on ​http://groups.google.com/group/django-developers/browse_thread/thread/5c27ac4703da33a9, when DEBUG = False, internal server errors will always cause mail_admins to be called (unless overridden in a custom handler class), and the current implementation of mail_admins always opens a connection to the SMTP server even if the email has no recipients (i.e., ADMINS=[]).

This patch makes mail_admins and mail_managers a no-op if there are no recipients. It also avoids constructing the internal server error email if it has no recipients.

[James Bennett]

Seems like a good idea; this needs documentation, though (to explain the behavior when ADMINS and/or MANAGERS is empty), and potentially unit tests (though I'm not certain how you'd go about testing this behavior).

[Jesse Young]

In regards to documentation, I'm not sure what there is to document, or where to document it. The only functional difference is that instead of opening a connection to the SMTP server and then immediately closing it without sending anything, it just doesn't do anything at all.

As far as unit tests, perhaps something like this?

settings.ADMINS = []
old_smtp_connection = django.core.mail.SMTPConnection
django.core.mail.SMTPConnection = None # overwrite it with something that would fail if we tried to use it
mail_admins('hi','there') # should fail if it actually tries to make an SMTP connection
django.core.mail.SMTPConnection = old_smtp_connection # clean up

[Jesse Young]

okay, my new diff includes a regression test that fails before and passes after, as well as a minor update to the documentation for mail_admins.

[Malcolm Tredinnick]

I'm not going to worry about the documentation change, since whether or not a network connection is made is an implementation detail. It doesn't affect user-visible functionality. I'm also dropping the change to core/handlers/, since the overhead isn't really significant and it's breaking encapsulation a bit (the code at that level doesn't care about settings.ADMIN; it just calls mail_admins() and is done with it).

I've introduced an extra bit to the EmailMessage class so that it also doesn't create the network connection if there's no intended recipients. After all, that's really the more extensible interface for sending email, so we should be consistent and make it possible there. I've still kept the checks for settings.ADMIN and settings.MANAGERS in place, although it's a bit borderline. It's only two lines of code in each case and we already use the settings in those functions anyway.

[Malcolm Tredinnick]

(In [9248]) Fixed #9383 -- Don't open a network connection for sending email if there's
nothing to send. Saves a bit of time when, for example, processing 500-error
emails with no ADMINs configured. Based on a patch from Jesse Young.

[Malcolm Tredinnick]

(In [9250]) [1.0.X] Fixed #9383 -- Don't open a network connection for sending email if
there's nothing to send. Saves a bit of time when, for example, processing
500-error emails with no ADMINs configured. Based on a patch from Jesse Young.

Backport of r9248 from trunk.

[Malcolm Tredinnick]

(In [9253]) [1.0.X] Backed out r9250. I committed this to the branch by mistake; there's no
bug in functionality fixed by this. Refs #9383.

[(none)]

Milestone post-1.0 deleted