Issue32657
Created on 2018-01-24 22:44 by Kenny Trytek, last changed 2022-04-11 14:58 by admin. This issue is now closed.
| Pull Requests | |||
|---|---|---|---|
| URL | Status | Linked | Edit |
| PR 5334 | closed | chason.chaffin, 2018-01-26 03:40 | |
| Messages (10) | |||
|---|---|---|---|
| msg310641 - (view) | Author: Kenny Trytek (Kenny Trytek) | Date: 2018-01-24 22:44 | |
Mutable arguments in signature of send_message: https://github.com/python/cpython/blob/master/Lib/smtplib.py#L892-L893 More Information: http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-arguments |
|||
| msg310658 - (view) | Author: Steven D'Aprano (steven.daprano) * ![]() |
Date: 2018-01-25 07:00 | |
Hi Kenny, and thanks, but I'm not sure what your point is. Are you claiming this is a bug in the code or the documentation?
For what it is worth... mutable defaults *are* a "gotcha", so the documentation isn't wrong. And mutable defaults are *not* illegal nor a bug and are occasionally useful. So I'm not sure why you are reporting this? Have you found a problem?
(I see that send_message gives rcpt_options a default of {} (empty dict) and then passes it to sendmail, which expects rcpt_options to be a list. I don't know if that matters.)
|
|||
| msg310660 - (view) | Author: Chason Chaffin (chason.chaffin) * | Date: 2018-01-25 07:25 | |
Hi Steven, I have nothing to do with the original submitter but this bug did catch my eye. Does using mutable variables in the SMTP library serve a purpose? Looking through the code, I wasn't able to spot anything obvious, and I did spot a place with a potential bug: https://github.com/python/cpython/blob/master/Lib/smtplib.py#L960 Here mail_options is mutated in the case of a non-ASCII email address. Subsequent calls to send_message, even to a different SMTP server (if I'm not mistaken) would also send these headers along. |
|||
| msg310677 - (view) | Author: R. David Murray (r.david.murray) * ![]() |
Date: 2018-01-25 14:28 | |
Chason: that does look like a bug. Mutable defaults are best to avoid, but if they are used read-only and not passed down further it isn't a problem. send_message was modeled on sendmail, and so copied it's use of defaults (which date from quite some time back :), but obviously I missed that mutation of the value in the code review of the patch that added those lines :( |
|||
| msg310679 - (view) | Author: Steven D'Aprano (steven.daprano) * ![]() |
Date: 2018-01-25 15:18 | |
On Thu, Jan 25, 2018 at 02:28:17PM +0000, R. David Murray wrote:
> obviously I missed that mutation of the value
> in the code review of the patch that added those lines :(
The docstring for send_message does say
If the sender or any of the recipient addresses contain non-ASCII
and the server advertises the SMTPUTF8 capability, the policy is
cloned with utf8 set to True for the serialization, and SMTPUTF8
and BODY=8BITMIME are asserted on the send.
which I don't really understand, but I thought that perhaps it was a
typo for *inserted* on the send, in the sense of inserted into the mail
options:
mail_options += ['SMTPUTF8', 'BODY=8BITMIME']
So are we agreed this is a bug? What about the default for rcpt_options
being a dict?
|
|||
| msg310713 - (view) | Author: R. David Murray (r.david.murray) * ![]() |
Date: 2018-01-25 22:05 | |
On Thu, 25 Jan 2018 15:18:01 +0000, Steven D'Aprano <report@bugs.python.org> wrote: > On Thu, Jan 25, 2018 at 02:28:17PM +0000, R. David Murray wrote: > The docstring for send_message does say > > If the sender or any of the recipient addresses contain non-ASCII > and the server advertises the SMTPUTF8 capability, the policy is > cloned with utf8 set to True for the serialization, and SMTPUTF8 > and BODY=8BITMIME are asserted on the send. "Asserted" means sent with the SMTP commands. It could be reworded to be clearer. > which I don't really understand, but I thought that perhaps it was a > typo for *inserted* on the send, in the sense of inserted into the mail > options: > > mail_options += ['SMTPUTF8', 'BODY=8BITMIME'] Even if that had been true, it would still be a bug to do it to the mutable argument :) > So are we agreed this is a bug? What about the default for rcpt_options > being a dict? I didn't look at that, but it probably is. |
|||
| msg310727 - (view) | Author: Chason Chaffin (chason.chaffin) * | Date: 2018-01-26 02:27 | |
I'm writing a PR for this right now. Is it worth it to write a test to make sure that mail_options isn't being mutated or is it enough just to update the code to use non-mutable defaults? Interesting side effect of looking into this is #32663 where I found the tests for the UTF8 code weren't being run. |
|||
| msg310732 - (view) | Author: R. David Murray (r.david.murray) * ![]() |
Date: 2018-01-26 04:17 | |
It's probably enough to fix it. I fear that if we also change them in sendmail we'll break someone's code, but maybe we should do that anyway, for 3.7 only. |
|||
| msg310734 - (view) | Author: Chason Chaffin (chason.chaffin) * | Date: 2018-01-26 04:43 | |
> It's probably enough to fix it. I fear that if we also change them in sendmail we'll break someone's code, but maybe we should do that anyway, for 3.7 only. Should I make a separate issue for that, or attach it to this PR? |
|||
| msg323779 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * ![]() |
Date: 2018-08-20 08:26 | |
See also issue34246. |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022-04-11 14:58:57 | admin | set | github: 76838 |
| 2019-02-22 07:52:16 | methane | set | versions: + Python 3.8, - Python 3.6, Python 3.7 |
| 2019-02-22 07:51:58 | methane | set | status: open -> closed resolution: fixed stage: patch review -> resolved |
| 2018-08-20 08:26:51 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages: + msg323779 |
| 2018-01-26 04:43:17 | chason.chaffin | set | messages: + msg310734 |
| 2018-01-26 04:17:55 | r.david.murray | set | messages:
+ msg310732 versions: - Python 2.7, Python 3.4, Python 3.5, Python 3.8 |
| 2018-01-26 03:40:35 | chason.chaffin | set | keywords:
+ patch stage: patch review pull_requests: + pull_request5180 |
| 2018-01-26 02:27:01 | chason.chaffin | set | messages: + msg310727 |
| 2018-01-25 22:05:01 | r.david.murray | set | messages: + msg310713 |
| 2018-01-25 15:18:01 | steven.daprano | set | messages: + msg310679 |
| 2018-01-25 14:28:17 | r.david.murray | set | nosy:
+ r.david.murray messages: + msg310677 |
| 2018-01-25 07:25:23 | chason.chaffin | set | nosy:
+ chason.chaffin messages: + msg310660 |
| 2018-01-25 07:00:02 | steven.daprano | set | nosy:
+ steven.daprano messages: + msg310658 |
| 2018-01-24 22:44:57 | Kenny Trytek | create | |
