DEP 0018 Dictionary-based EMAIL_PROVIDERS (ticket-35514)#105
DEP 0018 Dictionary-based EMAIL_PROVIDERS (ticket-35514)#105medmunds wants to merge 37 commits intodjango:mainfrom
Conversation
dbbd52c to
cb21f11
Compare
Move deprecated get_connection() kwargs handling entirely into the backwards compatibility section, and rework it to make the intent clear.
This comment was marked as outdated.
This comment was marked as outdated.
nessita
left a comment
There was a problem hiding this comment.
Submitting my initial set of comments to see if there are any red flags.
nessita
left a comment
There was a problem hiding this comment.
Amazing work, super well laid out, clear and precise. Thank you @medmunds! I'm overall onboard with the proposal as a whole pending some minor fixes. I tried to provide guidance on every 🤔 callout.
Let me know how to progress this!
|
@medmunds I'm happy to be added as Shepherd, and I think we could use number 18 for this and putting it up for review once the small bits are taken care of. What do you think? |
|
@nessita thanks as always for the thorough review and thoughtful suggestions 🙌 I'll update to incorporate all the feedback. (I suspect this may unearth some new questions about I hadn't really thought about handling this as a real DEP; I was just borrowing the format and the repo for discussion. (And I was going to close it unmerged once the details were worked out, since the ticket is already approved.) But now that you suggest it, a DEP makes sense. Thanks for shepherding! Once there's a new draft, I think it's important to get Jacob R's input. (Recognizing the enormous amount of time he's already put into this, I wanted to wait to @ him until there were fewer open questions.) I'll also try to reach out directly to some third-party packages that would be affected (django-mailer, django-celery-email, django-ses, etc.), to make sure they're aware of the proposal and have a chance to weigh in. And it would be helpful to get feedback from people who are actively using the SMTP EmailBackend in production, particularly on some of the deprecations. Maybe a forum post to re-introduce the feature? (My own production experience is with http-API-based backends like django-anymail and django-ses, plus a private fork of django-mailer for queuing and error recovery. So I may have some blind spots around things like get_connection() with username/password overrides.) |
|
@medmunds On a related tanget, I just came across this PR that allows cache backends to "know" their |
Ouroboros! I've been citing the new-features discussion that led to that PR as justification here for providing |
Notable changes: - Moved `fail_silently` from EmailBackend implementations to high-level APIs that call backend.send_messages(). - Renamed `provider` alias string parameter to `using`. (And generally resolved name and type decisions.) - Deprecated all `connection` args. - Made BACKEND optional, defaulting to smtp.EmailBackend. - Eliminated suggestions to use "magic" provider aliases. - Assigned DEP number and added shepherd.
4e9de4f to
58ebef1
Compare
|
I've updated the DEP to incorporate @nessita's feedback. There are still a few open questions (including a few new ones), but I think it's ready for @jrief to take a look if he's available. Based on discussion in this pr, the DEP now proposes moving The |
EmailMessage.send() needs to return the sent count.
Missed in earlier edit.
(sigh)
LilyFirefly
left a comment
There was a problem hiding this comment.
I've given this a full read through and I didn't spot any red flags.
Clean up some TODOs and prep to be a real DEP someday. - Rewrite abstract as an abstract - Split old abstract into history and status, rework - Move motivation earlier - Add myself to the implementation team
|
Hi Mike, I'm Ahmed I worked on the I've been following the DEP discussion and I'm interested in contributing Is there any part of the work where you could use help? Even smaller tasks |
@ahmedasar00 I appreciate the offer. Because the elements of this feature are so tightly intertwined, I don't think it would easily lend itself to dividing up the coding or even docs. One area Django always needs assistance is with code reviews, and that can be a great way to learn more about both the codebase and the contribution process. There are some tips in Django's contributor docs under Reviewing patches, and I highly recommend Sarah Boyce's Django needs you! (to do code review) keynote video from DjangoCon Europe last year. (While you're waiting for this PR to show up, there are around 100 other tickets with PRs waiting for review.) |
|
To improve DX around email configuration, when EMAIL_PROVIDERS is in use I'd like to have the SMTP EmailBackend:
Both of these should help avoid unexpected behavior. They would only apply in updated configurations where the user has added EMAIL_PROVIDERS to their settings; the old defaults would still apply in compatibility configurations. (In adding EMAIL_PROVIDERS, we have the opportunity to revisit some of the email config defaults without having to go through a separate deprecation period—but only in its initial release.) (Related: I'm getting to the point in the coding where I discover a bunch of details that aren't quite right in the DEP. I'll push some corrections later this week once I've opened the draft PR.) |
|
We have a new draft PR: django/django#21052 I just pushed a handful of DEP changes motivated by experience with the actual code. More details in the commit messages, but briefly:
|
Rework handling for `alias` and unknown `**kwargs`.
Django's own tests need an easy way to tell which email provider was used. Promote outbox annotation from a future feature to a current requirement.
…cation Clarify and simplify the compatibility mechanisms.
ca9a991 to
d6a1cab
Compare
`providers.default if using is None else providers[using]` works fine (and still avoids exposing DEFAULT_EMAIL_PROVIDER_ALIAS constant).
tim-schilling
left a comment
There was a problem hiding this comment.
I'm on board with this as is! Thank you @medmunds and @nessita for your work on this. I had a few nitpick comments, some formatting questions, and then provided my opinion around the 🤔 points.
I don't have any substantive requests for change, though if we engage other 3rd party package maintainers, they may have thoughts on the upgrade process.
Incorporate things learned while implementing the feature. Remove suggested implementations from the DEP. (The actual implementation in the reference PR is more accurate now.) Ensure the DEP specifies expected behavior. Rework "Upgrading EmailBackend implementations" to provide more flexibility for third-party backends that might reasonably want to keep existing settings alongside EMAIL_PROVIDERS OPTIONS support. Clean up the document structure and move some supplemental material later in the document for readability. Clean up punctuation, wrapping and stray whitespace.
| * When "default settings" are in use (no specific email configuration in | ||
| settings.py), the settings module issues a warning that the default email | ||
| provider will change from SMTP to none after the deprecation period. | ||
| Something like (exact wording TBD), "Django 7.0 will not have a default email |
There was a problem hiding this comment.
| Something like (exact wording TBD), "Django 7.0 will not have a default email | |
| Something like, "Django 7.0 will not have a default email |
nit: This makes it seem like there's a TODO in the DEP. I don't think we need the exact wording to match and "Something like" implies the wording will change.
There was a problem hiding this comment.
We're actively discussing this warning in the implementation PR. There's a concern it's unnecessarily noisy to warn Django 7.0 won't have a default email provider for projects that aren't even using email.
I'll update the DEP to say that projects that are using email with default settings need a deprecation warning, explaining they'll need to add EMAIL_PROVIDERS to avoid errors in Django 7.0. (But I'll leave both the timing and wording details to the implementation PR.)
📄 Read the formatted draft DEP 0018, latest revision.
See ticket-35514#comment:24
Django ticket-35514 will implement a dictionary-based
EMAIL_PROVIDERSsetting, similar toCACHES,DATABASES,STORAGESandTASKS. The ticket was approved following discussion on the django-developers list and at DjangoCon Europe 2024 sprints.The purpose of this DEP is to facilitate discussion and decisions on the proposed API and related deprecations.