django icon indicating copy to clipboard operation
django copied to clipboard

Add settings EMAIL_FQDN

Open jrief opened this issue 3 years ago • 23 comments

jrief avatar Nov 27 '20 20:11 jrief

@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.

felixxm avatar Nov 27 '20 20:11 felixxm

OK, there is a ticket-6989.

felixxm avatar Nov 27 '20 20:11 felixxm

The triage state was set to Accepted. Discussion is 13 years long, and the ticket was never closed.

jrief avatar Nov 27 '20 20:11 jrief

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.

jrief avatar Dec 08 '20 14:12 jrief

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 avatar Feb 12 '21 10:02 MaicoTimmerman

@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 to app1.cluster2.example.com, while we'd only want to EHLO using example.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.

jrief avatar Mar 19 '21 08:03 jrief

@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.)

carltongibson avatar Mar 19 '21 09:03 carltongibson

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.

jrief avatar Mar 20 '21 15:03 jrief

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.)

carltongibson avatar Mar 20 '21 17:03 carltongibson

@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 avatar Mar 24 '21 10:03 carltongibson

@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 avatar Mar 24 '21 12:03 jrief

@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 avatar Mar 31 '21 07:03 carltongibson

@carltongibson @MaicoTimmerman I will update this pull request, if we can agree, that EMAIL_FQDN is the proper name for this configuration directive.

jrief avatar Mar 31 '21 08:03 jrief

@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 avatar Mar 31 '21 08:03 carltongibson

@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 avatar Apr 12 '21 22:04 jrief

@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 avatar Dec 06 '21 11:12 MaicoTimmerman

@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.

jrief avatar Dec 06 '21 12:12 jrief

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 avatar Dec 06 '21 13:12 carltongibson

@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.

jrief avatar Dec 06 '21 15:12 jrief

@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.)

carltongibson avatar Dec 07 '21 09:12 carltongibson

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?

jrief avatar Dec 07 '21 09:12 jrief

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!

carltongibson avatar Dec 07 '21 10:12 carltongibson

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 avatar Jan 30 '22 08:01 jrief

@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.

nessita avatar Jan 16 '24 13:01 nessita

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

jrief avatar Jan 16 '24 14:01 jrief

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:

nessita avatar Jan 16 '24 15:01 nessita