#22734 closed Cleanup/optimization (wontfix)

Move SMTP / email settings to a dictionary

Reported by: jwa Owned by: nobody
Component: Core (Mail) Version: master
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 Changed 13 months ago by jwa

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement set

comment:2 Changed 13 months ago by jorgecarleitao

  • Cc jorgecarleitao@… added

comment:3 Changed 13 months ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 13 months ago by jezdez

Oh god, YES!!

comment:5 Changed 13 months ago by claudep

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 Changed 12 months ago by timo

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 Changed 12 months ago by claudep

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 Changed 12 months ago by timo

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 Changed 12 months ago by claudep

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 Changed 12 months ago by timo

Another option: we could deprecate the SMTP settings completely and recommend setting them the same way as timeout.

comment:11 Changed 12 months ago by claudep

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 Changed 12 months ago by timo

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 Changed 12 months ago by claudep

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 Changed 12 months ago by timo

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 Changed 12 months ago by aaugustin

I like timo's latest proposal. I'd just use OPTIONS instead of BACKEND_OPTIONS.

comment:16 Changed 12 months ago by claudep

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 Changed 12 months ago by loic84

  • Cc loic84 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 Changed 10 months ago by collinanderson

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.

Last edited 10 months ago by collinanderson (previous) (diff)

comment:19 Changed 10 months ago by claudep

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 Changed 10 months ago by carljm

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 Changed 10 months ago by claudep

... 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 Changed 10 months ago by timgraham

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:

comment:23 Changed 10 months ago by timgraham

  • Resolution set to wontfix
  • Status changed from new to closed

I think we should abandon this given the replies to https://groups.google.com/d/msg/django-developers/ZWpRbTwTxmo/OhorZcsuJnEJ

Note: See TracTickets for help on using tickets.
Back to Top