django
django copied to clipboard
Add settings EMAIL_FQDN
@jrief Thanks for this patch, however all new features require an accepted ticket. Also adding a new setting is always a bit controversial so I would recommend to start a discussion on DevelopersMailingList.
OK, there is a ticket-6989.
The triage state was set to Accepted. Discussion is 13 years long, and the ticket was never closed.
I see both pull requests as completely independent.
That one I fixed was accepted by Malcolm Tredinnick in 2010. Very sad that we can't ask him anymore. Having to override the backend to add configuration directives, creates a bunch of other problems; imagine using the same project image for different implementations.
Anyway, how about moving all email related configuration directives into a dictionary? But that's a different story.
I think the scope of this patch could be broadened a little more. Currently, it only improves the message-ID and names the setting accordingly. However, the EHLO/HELO SMTP command is still using the old FQDN behavior, which is also discussed in ticket-6989.
Some context: when a SMTP connection is made, often an EHLO or HELO command is send to define the connecting server. In our environment we run django in containers , meaning FQDN are more detailed then the domain from which e-mails are sent. The DNS_NAME.get_fqdn()
would resolve to app1.cluster2.example.com
, while we'd only want to EHLO using example.com
.
In the ticket there exists a 10 year old patch, which covers this correctly.
@MaicoTimmerman
I think the scope of this patch could be broadened a little more. Currently, it only improves the message-ID and names the setting accordingly. However, the EHLO/HELO SMTP command is still using the old FQDN behavior, which is also discussed in ticket-6989.
Some context: when a SMTP connection is made, often an EHLO or HELO command is send to define the connecting server. In our environment we run django in containers , meaning FQDN are more detailed then the domain from which e-mails are sent. The
DNS_NAME.get_fqdn()
would resolve toapp1.cluster2.example.com
, while we'd only want to EHLO usingexample.com
.
This is a valid point! So my current proposal naming that setting EMAIL_MESSAGEID_FQDN
wouldn't be appropriate anymore. EMAIL_LOCAL_HOSTNAME
doesn't seem to fit into a clustered environment. How about EMAIL_FULL_QUALIFIED_DOMAIN_NAME
or EMAIL_FQDN
? @carltongibson Other proposals?
Anyway, those pull requests shall remain independent.
@carltongibson Other proposals?
TBH I would (still) much prefer us to move towards pulling the numerous options here inside the email backend, rather than adding to an ever growing list of settings (with then matching parameters to the backend class...) — Configure your backend and then put that in settings…
I know that's a change in strategy, but my feeling is this is getting out of hand, and we'd be better served by a tidy-up.
It's open that you don't agree with that.
Assuming you're not totally opposed, would creating a modern backend, with all the right options, outside of Django, so we can iterate, and then proposing a migration to that once it was finished/stable be feasible? (I'm reluctant to press on adding new settings only to move to something else...)
The alternative is to just stick with how we do it now, and keep adding to the settings. (I'm less keen on that.)
Moving the Email backend out of Django (but as part of the ecosystem), certainly is an option we shall talk about. This however removes one battery, otherwise included into the Django project. How about this proposal:
For version 5, the Email backend shall be moved into a separate package.
This pull request won't make it into 3.2, so the next version would be 4. This means that we can make a bigger modification. How about putting all Email configuration directives into a Python dictionary, similar to DATABASES
or TEMPLATES
. We then could add a list of Email backends, rather than offering just one. Each of those backends then would offer the directive we have now.
For instance, EMAIL_BACKEND
, EMAIL_HOST
, EMAIL_PORT
, EMAIL_HOST_USER
, EMAIL_HOST_PASSWORD
, etc. would translate into
EMAIL: {
'default': {
'BACKEND': ...,
'SMTP_HOST': ...,
'SMTP_PORT': ...,
'HOST_USER': ...,
'HOST_PASSWORD': ...,
...
},
'other': {
...
},
}
When sending Email, one can choose the backend, or if unspecified, it falls back to default
. This behaviour is well known for other parts of the Django project.
I'm not suggesting we move it out of core permanently (although...🙂)
Rather, that we come up with a solution we're going to be happy with before pushing everyone down one path, and then immediately having them turn around and come back another way. Especially adding settings that's going to be painful.
I think the growth of the number of options being suggested for settings demonstrates that we need to rethink and modularise it in a better way.
My thought there is to have people configure these things internally to a backend, maybe leaving just one or two main options exposed (but arguably not.)
These issues are old, so I'm not sure too worried if they take an extra cycle to get right.
Does that make sense?
I'll have a look at this in the week, and think about the details of your suggestions here @jrief. (Don't have the bandwidth to consider them fully on the weekend.)
@jrief — just to update you — I'm thinking about this — trying to do the right thing for users…
There are only 5 email tickets in total — maybe we could (should) just add the setting here and those for ticket-31885 and then address a possible adjustment in a single step afterwards. 🤔
As I've said, I'm not particularly keen on adding the settings and then switching to something else, but the advantage of doing it this way would be to get those two tickets resolved for 4.0. We'd then buy the time needed (n versions likely) to come up with a more encapsulated solution.
Not sure how I feel about that, but your take is appreciated.
@carltongibson I'm completely with you. This probably is the beast approach!
As a side note, a more modular configuration system would be highly appreciated: Then each component gets its own prefix and all configuration settings would start with something like DJANGO.EMAIL.FQDN = '...'
or DJANGO.DATABASE.DEFAULT...
or CRISPYFORMS.TEMPLATE_PACK = '...'
.
@jrief @MaicoTimmerman — What's the conclusion of the discussion on using EMAIL_FQDN
? — Do you want to update the patch to include the suggested change?
@carltongibson @MaicoTimmerman
I will update this pull request, if we can agree, that EMAIL_FQDN
is the proper name for this configuration directive.
@jrief It seems fine. We can always adjust on review, but I can't push this forwards unless it's actually in the "yes, this is the proposed change" state. From the discussion that's not the case.
@carltongibson
I can't push this forwards unless it's actually in the "yes, this is the proposed change" state. From the discussion that's not the case.
What can I do to fix this?
@jrief @carltongibson I just ran into this thread again. I think the change is nearly complete, however we are still missing the fix for the EHLO
/ HELO
.
My proposed addtion would be:
diff --git a/django/core/mail/backends/smtp.py b/django/core/mail/backends/smtp.py
index 13ed4a2798..218365c6d3 100644
--- a/django/core/mail/backends/smtp.py
+++ b/django/core/mail/backends/smtp.py
@@ -50,7 +50,11 @@ class EmailBackend(BaseEmailBackend):
# If local_hostname is not specified, socket.getfqdn() gets used.
# For performance, we use the cached FQDN for local_hostname.
- connection_params = {'local_hostname': DNS_NAME.get_fqdn()}
+ connection_params = {}
+ if settings.EMAIL_FQDN:
+ connection_params['local_hostname'] = settings.EMAIL_FQDN
+ else:
+ connection_params['local_hostname'] = DNS_NAME.get_fqdn()
if self.timeout is not None:
connection_params['timeout'] = self.timeout
if self.use_ssl:
@MaicoTimmerman @carltongibson I'm willing to push this forward, but the Django board must agree on details, for instance if EMAIL_FQDN
is the right name for this extra settings variable. I asked for this on April 13th, but didn't get any response yet. Shall I retry through the Django developer mailing list?
This settings variable then would influence the right hand side of the Message-ID
and the HELO
/EHLO
connection string. The implementation for the latter is currently missing.
Hi @jrief — thanks for the work on this. I was hoping to return to it in time for 4.0, but time didn't allow. It's on my list to resolve for 4.1. What we want to avoid is rushing to add a partial solution only to deprecate that immediately. Measure twice… and so on.
Related to #13305, the issue is whether there's a better option than continuing to proliferate settings for every possible configuration option. i.e. encapsulate that better so that folks would just set EMAIL_BACKEND
(as an example).
The issue/delay is just thinking through how an API for that would look — where exactly would the email configuration live… I looked into this a bit and am working on the thought that if get_connection()
took a class as well as a import-path string then it might[^]
be neat enough to do something like:
from functools import partial
from django.core.mail.backends.smtp import EmailBackend
EMAIL_BACKEND = partial(
EmailBackend,
**my_many_custom_options
)
Rather than (having to) declare an actual subclass somewhere and then use the import path for the setting — but I'm yet to formalise that. Implementation apart, the goal would be to encapsulate the SMTP related email features inside django.core.mail
, and the docs for that, reducing the spread in django.conf
. Ultimately if something along those lines is not feasible then we can add ever more settings but...
[^]
: Might — I'm very happy to take input here: What's the API you'd like to see?
@carltongibson wouldn't it then make sense, to allow multiple SMTP backends, similar to the database?
In the project's settings.py
this then would look like:
EMAIL = {
'default': {
'BACKEND': 'django.core.mail.backends.smtp.EmailBackend',
'HOST': 'smtp.example.org',
'HOST_USER': 'smtp_user',
'HOST_PASSWORD': 'smtp_secret',
'PORT': 25,
'FQDN': 'example.org',
# and all other related directives
}
}
Then instead of passing a class or import-path string to get_connection()
we would pass in None
(default) or an alternative name of the Email backend. This would allow users to configure more than one Email backend and let the application choose one of them, depending on their business logic.
It would also remove all but one (EMAIL
) Email configuration directives from the global namespace.
Personally, I would prefer SMTP = {...}
as the main configuration directive, but EMAIL
seems to be more popular in Django.
@carltongibson wouldn't it then make sense, to allow multiple SMTP backends, similar to the database?
Interesting. I hadn't considered that option — is it something you'd want? (Moving the config into the backend would allow multiple backends so it's certainly related.)
Interesting. I hadn't considered that option — is it something you'd want? (Moving the config into the backend would allow multiple backends so it's certainly related.)
For consistency it would make sense to handle the SMTP-backends similar to database and template-engine backends.
Currently I have no immediate need for different backends, but it the future this could be an issue. It also would make the configuration cleaner, if someone needs different backends when switching from a developer to a production environment.
BTW.: I am the second maintainer of https://github.com/ui/django-post_office and there we actually need a different Email backend.
Shall we move this discussion to the Django developer mailing list?
Shall we move this discussion to the Django developer mailing list?
If you wish, yes, super.
My list looks like: finish the SECRET_KEYS PR; process Andrew's Async ORM interface PR; look at these Email backend tickets, but if you'd like to pick it up happy to input and review!
Thanks!
I moved this issue to the discussion group: https://groups.google.com/g/django-developers/c/R8ebGynQjK0 Please provide feedback, because as it seems currently we're stuck on this.
@jrief Hello! I have just caught up with this PR, the related ones, and the linked tickets (this is in the context of ticket #35118). Are you available to continue contributing as concluded in the django-developers email thread?
I'm keen to close this PR in favor of the new/future ticket for the multiple email backends work. Let me know! Thank you.
Hi Natalia, I'd like to work on the ticket implementing multiple email backends. However, I currently have no resources to work on it. So please feel free to close it. Jacob
Hi Natalia, I'd like to work on the ticket implementing multiple email backends. However, I currently have no resources to work on it. So please feel free to close it. Jacob
Thank you Jacob for your honest response. I will close this PR, hopefully you can go back to this contribution some time in the future. Looking forward to that! :100: