Opened 10 years ago
Closed 10 years ago
#22734 closed Cleanup/optimization (wontfix)
Move SMTP / email settings to a dictionary
Reported by: | Julian Wachholz | Owned by: | nobody |
---|---|---|---|
Component: | Core (Mail) | Version: | dev |
Severity: | Normal | Keywords: | settings, email, mail, smtp |
Cc: | jorgecarleitao@…, loic84 | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | yes | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Email settings for the SMTP backend are currently configured using 6 different variables. As there will be more and more to configure in the future (see #20743) it might be an idea to move these settings into a dictionary, similar to the way DATABASES
or CACHES
are configured.
An initial patch can be found here: https://github.com/julianwachholz/django/tree/feature/12factor-smtp
Things to note:
Backwards compatibility is a large concern here, as previously detected in #21051. One way to introduce these setting without breaking user-space code that directly accesses the EMAIL_*
settings would be to have lazy settings within global_settings.py
.
Change History (23)
comment:1 by , 10 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
comment:2 by , 10 years ago
Cc: | added |
---|
comment:3 by , 10 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:4 by , 10 years ago
comment:5 by , 10 years ago
I've written an alternative patch: https://github.com/django/django/pull/2836.
The dict is now called SMTP_CONFIG. I also slightly modified the BaseSettings
object so as it would be possible to only partially overwrite an existing dictionary setting. For example, defining SMTP_CONFIG = {'USE_TLS': True}
in custom settings would not overwrite SMTP_CONFIG
from global settings, but copy and update that dict, keeping the other default values.
I'm not absolutely sure it's a good idea in general, as it might render more difficult deleting an entry from a global setting (e.g. defining CACHES={'custom': ...}
would keep the default
entry). An another idea could be making this mechanism opt-in/opt-out by having a special key on the dict (like _keep_defaults
).
comment:6 by , 10 years ago
Another idea I had while mulling this over was something like this:
EMAIL = { 'default': { 'BACKEND': 'django.core.mail.backends.smtp.EmailBackend', 'HOST': 'localhost', 'PORT': 25, }, 'file': { 'BACKEND': 'django.core.mail.backends.filebased.EmailBackend', 'FILE_PATH': '/tmp/app-messages', }, }
The implementation would obviously be a lot trickier and it may be overkill since it's probably a rare use case to want to use multiple mail providers in a single project, but it would allow us to consolidate more than just the SMTP related settings.
comment:7 by , 10 years ago
Why not, but as you said, it's hard to find real use cases for that structure. My current feelings tend to be -0 on that idea.
comment:8 by , 10 years ago
Thoughts on:
EMAIL = { 'BACKEND': 'django.core.mail.backends.smtp.EmailBackend', 'HOST': 'localhost', 'PORT': 25, }
(to allow consolidating more than just the SMTP settings)
comment:9 by , 10 years ago
For me, email backend is one thing, and SMTP settings an other. I don't see the gain in trying to group them, but... it's just me :-)
comment:10 by , 10 years ago
Another option: we could deprecate the SMTP settings completely and recommend setting them the same way as timeout.
comment:11 by , 10 years ago
I think that choosing to customize some behavior by a setting or by code depends on:
- the configured value is strongly instance-specific, and might change for each environment: -> setting
- the configured value is project-specific and is not supposed to change on each deployment environment: -> code
For example, the SMTP host and port are completely dependent on the machine on which the code is running. I find a bit strange to ask people to change the code of a project or application each time they want to deploy it on another environment. With this reasoning, I think that even the timeout might be better set by a setting :-/
comment:12 by , 10 years ago
I agree the current implementation of timeout
is odd and we should move it to the dictionary setting.
In terms of grouping email settings, I was thinking it would allow us to group other backend options like EMAIL_FILE_PATH
:
EMAIL = { 'BACKEND': 'django.core.mail.backends.filebased.FileBackend', 'FILE_PATH': '...', }
A better grouping might be to have an EMAIL['BACKEND_OPTIONS']
key (mirroring how we have OPTIONS
in DATABASES
) with value {'FILE_PATH': '...'}
(or the dictionary that's equivalent to the proposed SMTP_CONFIG
if 'BACKEND'
were smtp.EmailBackend
).
comment:13 by , 10 years ago
I can understand your desire to unify all email settings. In summary and if I understand it well, here are the two proposed designs:
a.
EMAIL_BACKEND = '...' EMAIL_FILE_PATH = '...' # If file backend is chosen SMTP_CONFIG = { # If smtp backend is chosen 'HOST': '...', 'PORT': ?, 'USER': '', 'PASSWORD': '', 'USE_TLS': '' 'USE_SSL': '' }
b.
EMAIL = { 'BACKEND': '...' 'FILE_PATH = '...' # If file backend is chosen # If smtp backend is chosen: 'HOST': '...', 'PORT': ?, 'USER': '', 'PASSWORD': '', 'USE_TLS': '' 'USE_SSL': '' }
I'm still sticking with the first design (but not -1 on the second), because I don't think we gain much in grouping all settings (and assuming generally that less change is better).
The proposal in comment:6 seems overengineered in my opinion.
comment:14 by , 10 years ago
I think external backends (e.g. django-ses) could benefit from a BACKEND_OPTIONS
idea like this:
EMAIL = { 'DEFAULT_FROM_ADDRESS': '...', # formerly DEFAULT_FROM_EMAIL 'SERVER_ADDRESS': '...', # formerly SERVER_EMAIL 'SUBJECT_PREFIX': '...', 'BACKEND': '...', 'BACKEND_OPTIONS': { 'HOST': '...', ... # All SMTP, filebased, or (external backend) options }, }
but I won't push for it if no one else cares for it.
comment:15 by , 10 years ago
I like timo's latest proposal. I'd just use OPTIONS
instead of BACKEND_OPTIONS
.
comment:16 by , 10 years ago
The difficulty will be to not force the user to redefine all settings just to change the SMTP port, for example. There was such a tentative in my patch, not sure if it will works for multilevel dicts.
That is:
EMAIL = { 'OPTIONS': { 'PORT': 587, } }
should have the same effect as the former EMAIL_PORT = 587
comment:17 by , 10 years ago
Cc: | added |
---|
OPTIONS
would be consistent with what we have for DATABASES
: +1 over BACKEND_OPTIONS
.
dict
composability can be a bit of a pain point as demonstrated by logging's dictConfig
, but for SMTP I'd say we can get away with updating the defaults with what's found in OPTIONS
.
class EmailBackend(BaseEmailBackend): def __init__(self, host=None, port=None, **kwargs): options = settings.EMAIL.get('OPTIONS', {}) self.host = host or options.get('HOST', 'localhost') self.port = host or options.get('PORT', 25)
comment:18 by , 10 years ago
I don't see any benefit to moving email settings to a dictionary. It is helpful for databases and caches because there can be multiple backends. It makes the popular "from local_settings import *" convention harder to use. What's wrong with 6 individual settings? If the goal is to allow multiple email backends, then let's make that the ticket goal.
comment:19 by , 10 years ago
Let's debate the usefulness of this, but meanwhile, if we would decide for an implementation of comment:14, here is the pull request updated: https://github.com/django/django/pull/2836
comment:20 by , 10 years ago
I agree with Collin; unless we are adding new capabilities (i.e. multiple configured email backends, which it seems nobody really wants), it's hard to find any actual benefit from this change to justify the churn (and the additional complexities of working with dictionary settings in partial-override scenarios).
Even if we want the ability to pass arbitrary options to a backend (i.e. the OPTION
key proposal), that could just as easily be a new EMAIL_BACKEND_OPTIONS
setting.
Am I missing the benefits of doing this?
comment:21 by , 10 years ago
... that could just as easily be a new EMAIL_BACKEND_OPTIONS setting
Sure, as far as the default dictionary setting is empty in global_settings.py, unless we would again be confronted to the same issue of needing to redefine the entire dictionary to redefine only one key.
Am I missing the benefits of doing this?
The main benefit is to have all logically-grouped settings in one dictionary. And yes, there is some price to pay (cf. the django-developers discussion: https://groups.google.com/d/msg/django-developers/t8ybImtdnpM/esy-UDPTjoIJ).
comment:22 by , 10 years ago
If there are no objections to adding more settings when additional configuration of the email backends is needed, then I don't care to argue for the dictionary complexity. IMO, this dictionary idea was to placate the people who say "no new settings!". As Aymeric wrote on the mailing list, moving settings to a dictionary is an artificial fix for this.
At the moment, the new settings would be:
EMAIL_TIMEOUT
(to replace the current subclass recommendation)EMAIL_SSL_CERTFILE
andEMAIL_SSL_KEYFILE
for #20743
comment:23 by , 10 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
I think we should abandon this given the replies to https://groups.google.com/d/msg/django-developers/ZWpRbTwTxmo/OhorZcsuJnEJ
Oh god, YES!!