bpo-32814: Handle 8BITMIME availabilty in smtplib.SMTP.send_message#8303
bpo-32814: Handle 8BITMIME availabilty in smtplib.SMTP.send_message#8303segevfiner wants to merge 6 commits intopython:mainfrom
Conversation
segevfiner
force-pushed
the
bpo-32814-8bitmime-handling
branch
from
9ae6a30 to
d3a75d4
Compare
… the policy requests an 8bit content transfer encoding, than we willcheck for the 8BITMIME extension and use it if available. Otherwise wewill use a 7bit content transfer encoding since the server doesn'tsupport 8bit messages.
segevfiner
force-pushed
the
bpo-32814-8bitmime-handling
branch
from
d3a75d4 to
f1d84c0
Compare
bitdancer
left a comment
bitdancer
left a comment
There was a problem hiding this comment.
Thanks for the patch. I have a few suggestions, but for the most part it looks good.
| Support for internationalized addresses (``SMTPUTF8``). | ||
|
|
||
| .. versionadded:: 3.8 | ||
| Handling ``8BITMIME`` (:rfc:`6152`) availability automatically. |
There was a problem hiding this comment.
I think this is a bug fix, not a new feature.
There was a problem hiding this comment.
It's kinda both, hard to decide... 😛
I documented this like this since it's a breaking change. Do you think it should be documented differently? If so, then how?
| to_addrs = [a[1] for a in email.utils.getaddresses(addr_fields)] | ||
| # Make a local copy so we can delete the bcc headers. | ||
| msg_copy = copy.copy(msg) | ||
| policy = msg.policy.clone() |
There was a problem hiding this comment.
There doesn't appear to be any reason to make an unchanged clone here.
There was a problem hiding this comment.
There isn't. I think I originally intended to clone once and modify as needed, but later figured that Policy objects are immutable and forgot to remove this.
| self.assertEqual(rcpttos, ['you']) | ||
| self.assertIn('\nSubject: Log\n', data) | ||
| self.assertTrue(data.endswith('\n\nHello \u2713')) | ||
| self.assertTrue(data.endswith('\n\nSGVsbG8g4pyTCg==')) |
There was a problem hiding this comment.
This is problematic. I'm guessing the test server needs to be told to support utf8 so the test itself doesn't change, but I haven't looked at the code. Regardless, we'd need to get Vinay Sajep to sign off on any changes here.
There was a problem hiding this comment.
I changed this like this because the SMTP server used in this tests uses decode_data=True which makes it not support 8BITMIME. See smtpd.SMTPServer's documentation. What's the correct fix for this is open for discussion.
There was a problem hiding this comment.
Ah, OK, then not using decode_data=True would be the right fix. decode_data=True is deprecated.
| self.addCleanup(smtp.close) | ||
| self.assertRaises(smtplib.SMTPNotSupportedError, | ||
| smtp.send_message(msg)) | ||
| lambda: smtp.send_message(msg)) |
There was a problem hiding this comment.
This assertRaises call should read self.assertRaises(smtplib.SMTPNotSupportedError, smtp.send_message, msg). The fact that this test isn't failing as-is means it isn't being run, which is issue #32663, and we should fix that one before applying your patch.
There was a problem hiding this comment.
After I changed this. I saw you merged a fix for bpo-32663. I will merge.
| if policy.cte_type == '8bit': | ||
| if self.has_extn('8bitmime'): | ||
| if 'BODY=8BITMIME' not in mail_options: | ||
| mail_options.append('BODY=8BITMIME') |
There was a problem hiding this comment.
Looks like we could end up appending BODY=8BITMIME twice (once in the except, and then again here). Probably want to keep the international flag and append SMTPUTF8 iff it is true.
There was a problem hiding this comment.
I called the boolean differently since this is about an 8-bit body and not about internationalized mail.
| # Make a local copy so we can delete the bcc headers. | ||
| msg_copy = copy.copy(msg) | ||
| policy = msg.policy.clone() | ||
| mail_options = mail_options[:] |
There was a problem hiding this comment.
Move this line up to the top of the function, please. I'm not sure how those default argument values got pass the original review; I thought there was an issue to fix them but I can't find it.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be poked with soft cushions! |
|
I have made the requested changes; please review again. Well... except the ones I wasn't sure about and left a reply that is... |
|
Thanks for making the requested changes! @bitdancer: please review the changes made to this pull request. |
|
cc @maxking |
|
Hi, any chance to get this merged? |
|
Seconding the bump |
|
This PR is stale because it has been open for 30 days with no activity. |
8 participants
If the policy requests an 8bit content transfer encoding, than we will check for the 8BITMIME (RFC6152) extension and use it if available. Otherwise we will use a 7bit content transfer encoding since the server doesn't support 8bit messages.
Hopefully I got this right... RFCs tend to be long/verbose, and considering extensions, what you need to read is essentially spread across multiple RFCs with no central location that links them all, well... at least one that I could find that is.
Note the weird behavior in the tests regarding
Content-Transfer-Encoding. One test ends up usingquoted-printablewhile the otherbase64. I didn't dig into why...@bitdancer
https://bugs.python.org/issue32814