New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #28598: Document lack of BCC in console & filebased email backends #10152

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@jschneier
Contributor

jschneier commented Jul 8, 2018

@jschneier

This comment has been minimized.

Show comment
Hide comment
@jschneier

jschneier Jul 11, 2018

Contributor

@carltongibson do you receive notice when I reply there, I added a few questions/my thoughts on the ticket. Sorry if you do, don't mean to spam.

Contributor

jschneier commented Jul 11, 2018

@carltongibson do you receive notice when I reply there, I added a few questions/my thoughts on the ticket. Sorry if you do, don't mean to spam.

@jschneier jschneier closed this Jul 11, 2018

@jschneier jschneier reopened this Jul 11, 2018

@jschneier

This comment has been minimized.

Show comment
Hide comment
@jschneier

jschneier Jul 11, 2018

Contributor

Is there a policy about whether or not the ..method doc bits includes self? Seems to go with mostly no but occasionally yes.

Contributor

jschneier commented Jul 11, 2018

Is there a policy about whether or not the ..method doc bits includes self? Seems to go with mostly no but occasionally yes.

@jschneier

This comment has been minimized.

Show comment
Hide comment
@jschneier

jschneier Jul 14, 2018

Contributor

@carltongibson indeed this patch passes all the tests re: writing bytes.

index ddcd9ed97b..e949846ab1 100644
--- a/django/core/mail/backends/filebased.py
+++ b/django/core/mail/backends/filebased.py
@@ -43,11 +43,6 @@ class EmailBackend(ConsoleEmailBackend):
         kwargs['stream'] = None
         super().__init__(*args, **kwargs)
 
-    def write_message(self, message):
-        self.stream.write(message.message().as_bytes() + b'\n')
-        self.stream.write(b'-' * 79)
-        self.stream.write(b'\n')
-
     def _get_filename(self):
         """Return a unique file name."""
         if self._fname is None:
@@ -58,7 +53,7 @@ class EmailBackend(ConsoleEmailBackend):
 
     def open(self):
         if self.stream is None:
-            self.stream = open(self._get_filename(), 'ab')
+            self.stream = open(self._get_filename(), 'a')
             return True
         return False
 

The change comes from this patch which mine would partly reverse: c988745. I do wonder if it was a Python2/Python3 thing.

Contributor

jschneier commented Jul 14, 2018

@carltongibson indeed this patch passes all the tests re: writing bytes.

index ddcd9ed97b..e949846ab1 100644
--- a/django/core/mail/backends/filebased.py
+++ b/django/core/mail/backends/filebased.py
@@ -43,11 +43,6 @@ class EmailBackend(ConsoleEmailBackend):
         kwargs['stream'] = None
         super().__init__(*args, **kwargs)
 
-    def write_message(self, message):
-        self.stream.write(message.message().as_bytes() + b'\n')
-        self.stream.write(b'-' * 79)
-        self.stream.write(b'\n')
-
     def _get_filename(self):
         """Return a unique file name."""
         if self._fname is None:
@@ -58,7 +53,7 @@ class EmailBackend(ConsoleEmailBackend):
 
     def open(self):
         if self.stream is None:
-            self.stream = open(self._get_filename(), 'ab')
+            self.stream = open(self._get_filename(), 'a')
             return True
         return False
 

The change comes from this patch which mine would partly reverse: c988745. I do wonder if it was a Python2/Python3 thing.

@jschneier

This comment has been minimized.

Show comment
Hide comment
@jschneier

jschneier Jul 26, 2018

Contributor

@carltongibson here's a different possible iteration since we are in agreement about displaying the BCC. Obviously there might be a backward compatibility issue here. I'm not really in favor of adding the hook you mentioned although I could compromise to that if it's what we need to fix-up backwards compatibility.

Thoughts?

Contributor

jschneier commented Jul 26, 2018

@carltongibson here's a different possible iteration since we are in agreement about displaying the BCC. Obviously there might be a backward compatibility issue here. I'm not really in favor of adding the hook you mentioned although I could compromise to that if it's what we need to fix-up backwards compatibility.

Thoughts?

@jschneier

This comment has been minimized.

Show comment
Hide comment
@jschneier

jschneier Jul 26, 2018

Contributor

Also curious if your investigation fo the string/bytes has turned up anything? That is the reason that there is duplication between file-based and console-based and I didn't really understand the reasoning from the linked PR.

Contributor

jschneier commented Jul 26, 2018

Also curious if your investigation fo the string/bytes has turned up anything? That is the reason that there is duplication between file-based and console-based and I didn't really understand the reasoning from the linked PR.

@carltongibson

This comment has been minimized.

Show comment
Hide comment
@carltongibson

carltongibson Jul 27, 2018

Member

Hey @jschneier. Thanks for the follow-up.

I didn't get a chance to look at this again yet. I will do so in the next week.

I can't see much of a BC issue here... (?!)

Member

carltongibson commented Jul 27, 2018

Hey @jschneier. Thanks for the follow-up.

I didn't get a chance to look at this again yet. I will do so in the next week.

I can't see much of a BC issue here... (?!)

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Jul 27, 2018

Member

Regarding c988745, it looks to me like there aren't any tests for that change. I rolled back to that commit and reverted the changes in django.core.mail and all tests pass on Python 2 and 3.

Member

timgraham commented Jul 27, 2018

Regarding c988745, it looks to me like there aren't any tests for that change. I rolled back to that commit and reverted the changes in django.core.mail and all tests pass on Python 2 and 3.

@jschneier

This comment has been minimized.

Show comment
Hide comment
@jschneier

jschneier Jul 27, 2018

Contributor

I can't see much of a BC issue here... (?!)

In the same way that the tests depends on what is output (and thus are currently failing) couldn't someone's code be depending on it?

Contributor

jschneier commented Jul 27, 2018

I can't see much of a BC issue here... (?!)

In the same way that the tests depends on what is output (and thus are currently failing) couldn't someone's code be depending on it?

@jschneier

This comment has been minimized.

Show comment
Hide comment
@jschneier

jschneier Jul 27, 2018

Contributor

Regarding c988745, it looks to me like there aren't any tests for that change. I rolled back to that commit and reverted the changes in django.core.mail and all tests pass on Python 2 and 3.

Maybe we can ask @apollo13?

Contributor

jschneier commented Jul 27, 2018

Regarding c988745, it looks to me like there aren't any tests for that change. I rolled back to that commit and reverted the changes in django.core.mail and all tests pass on Python 2 and 3.

Maybe we can ask @apollo13?

@carltongibson

This comment has been minimized.

Show comment
Hide comment
@carltongibson

carltongibson Jul 27, 2018

Member

...couldn't someone's code be depending on it?

Yes. Hmmm. I suppose so. But then I steer back towards thinking this should just be a wontfix, plus “create your own backend if BCCs are important to you”. (I kind of agree with you that the hook idea is suboptimal — we should just fix it — but if BC is a concern then how else do we satisfy both requirements?)

Member

carltongibson commented Jul 27, 2018

...couldn't someone's code be depending on it?

Yes. Hmmm. I suppose so. But then I steer back towards thinking this should just be a wontfix, plus “create your own backend if BCCs are important to you”. (I kind of agree with you that the hook idea is suboptimal — we should just fix it — but if BC is a concern then how else do we satisfy both requirements?)

@jschneier

This comment has been minimized.

Show comment
Hide comment
@jschneier

jschneier Jul 27, 2018

Contributor
Contributor

jschneier commented Jul 27, 2018

@apollo13

This comment has been minimized.

Show comment
Hide comment
@apollo13

apollo13 Jul 27, 2018

Member

@jschneier #2126 The main reason was consistency. I had an issue with my emails and wanted to debug it with the console backend. This did not work because the unicode representation used a different encoding then when writing to bytes. Imo my change still makes sense, even if there are not tests showing it.

Member

apollo13 commented Jul 27, 2018

@jschneier #2126 The main reason was consistency. I had an issue with my emails and wanted to debug it with the console backend. This did not work because the unicode representation used a different encoding then when writing to bytes. Imo my change still makes sense, even if there are not tests showing it.

@jschneier

This comment has been minimized.

Show comment
Hide comment
@jschneier

jschneier Jul 27, 2018

Contributor
Contributor

jschneier commented Jul 27, 2018

@apollo13

This comment has been minimized.

Show comment
Hide comment
@apollo13

apollo13 Jul 27, 2018

Member

@jschneier It should be fine to write to the file in non-binary mode as long as you use as_bytes and then decode (ie just don't use as_string; which will result in a different serialization then for a normal smtp transaction). Maybe also add a comment on why the bytes -> decode dance is there that it doesn't get replaced by accident.

Member

apollo13 commented Jul 27, 2018

@jschneier It should be fine to write to the file in non-binary mode as long as you use as_bytes and then decode (ie just don't use as_string; which will result in a different serialization then for a normal smtp transaction). Maybe also add a comment on why the bytes -> decode dance is there that it doesn't get replaced by accident.

@jschneier

This comment has been minimized.

Show comment
Hide comment
@jschneier

jschneier Jul 27, 2018

Contributor

Just opened #10241 pursuant to this discussion. Probably if we really want to mimic the smtp backend we should also add linesep='\r\n'? Appreciate any feedback there.

Contributor

jschneier commented Jul 27, 2018

Just opened #10241 pursuant to this discussion. Probably if we really want to mimic the smtp backend we should also add linesep='\r\n'? Appreciate any feedback there.

@apollo13

This comment has been minimized.

Show comment
Hide comment
@apollo13

apollo13 Jul 27, 2018

Member

Good catch, yes. I think I added \r\n after I rewrote the filebackend and forgot to add it there :)

Member

apollo13 commented Jul 27, 2018

Good catch, yes. I think I added \r\n after I rewrote the filebackend and forgot to add it there :)

@jschneier

This comment has been minimized.

Show comment
Hide comment
@jschneier

jschneier Jul 28, 2018

Contributor

Interestingly using linesep='\r\n' results in test failures on Windows.

Contributor

jschneier commented Jul 28, 2018

Interestingly using linesep='\r\n' results in test failures on Windows.

@apollo13

This comment has been minimized.

Show comment
Hide comment
@apollo13

apollo13 Jul 28, 2018

Member

Mind showing which? I don't have a windows system currently :)

Member

apollo13 commented Jul 28, 2018

Mind showing which? I don't have a windows system currently :)

@jschneier

This comment has been minimized.

Show comment
Hide comment
@jschneier

jschneier Jul 28, 2018

Contributor
Contributor

jschneier commented Jul 28, 2018

@carltongibson

This comment has been minimized.

Show comment
Hide comment
@carltongibson

carltongibson Aug 6, 2018

Member

Right, lets see if we can roll this up... (It shouldn't be that complicated... 😄)

@jschneier: What do you want to do here? What's the idea outcome for you?

@ALL: can we accept a BC change? Surely it's easier for people to test the logic that generates the arguments to the send_mail() call (e.g. recipients_list) rather than call re-parsing the output later (?)) (If so the change suggested here looks fine to me in principle.)

Failing that: My idea for the format_message() hook was easier subclassing with a behaviour change. If that's not worth the effort (which it may not be) then I'm inclined towards wontfix. I had thought even the doc change wasn't really necessary but, since Josh ran into it to, it probably is worth mentioning.

If we nail down the strategy, I can help debug the windows issues etc. (A bit of de-duplication here seems feasible.)

Member

carltongibson commented Aug 6, 2018

Right, lets see if we can roll this up... (It shouldn't be that complicated... 😄)

@jschneier: What do you want to do here? What's the idea outcome for you?

@ALL: can we accept a BC change? Surely it's easier for people to test the logic that generates the arguments to the send_mail() call (e.g. recipients_list) rather than call re-parsing the output later (?)) (If so the change suggested here looks fine to me in principle.)

Failing that: My idea for the format_message() hook was easier subclassing with a behaviour change. If that's not worth the effort (which it may not be) then I'm inclined towards wontfix. I had thought even the doc change wasn't really necessary but, since Josh ran into it to, it probably is worth mentioning.

If we nail down the strategy, I can help debug the windows issues etc. (A bit of de-duplication here seems feasible.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment