Deprecating tlsMode

[argonemyth]

The class attribute tlsMode in twisted.mail.smtp.ESMTPClient is no longer an functional attribute - the usage of secure transport doesn't depend on the value anymore. It's time to remove it from the class.

[moijes12]

Are you looking for something like this ?

[exarkun]

Thanks for working on this.

This patch doesn't accomplish the desired result, which is to emit a deprecation when the tlsMode attribute is read or written. The goal is to warn developers with code that manipulates this attribute so they know the attribute no longer means anything. Introducing a new API that emits a deprecation warning doesn't accomplish the goal, since existing code won't use that API.

A property can be used to achieve the desired effect for when tlsMode is read. Unfortunately, since it's on a classic class, write properties don't work, so a __setattr__ hook may be necessary.

[namn]

Line 1246 in smtp.py should be reflowed to 79 columns.

Line 612 in test_smtp.py looks redundant. I think line 1529 in that same file is also not needed. Line 1542 in the same file should be reflowed.

I would replace is with has been in 5852.removal.

[exarkun]

Thanks for your continuing work on this issue.

1. warnings.catch_warnings is not how we unit test deprecations. Please stick to the convention established in other deprecation unit tests in Twisted.
2. The implementation of __setattr__ breaks all attribute setting, completely.
3. The news fragment can just say that the attribute is deprecated, the version information is not needed (the news fragment will appear in the news file under a section for a particular version of Twisted).

[moijes12]

twisted.mail.smtp.ESMTPClient being a classic class, I couldn't use descriptors. So I had to use setattr hook. What I did notice was that the test messages were getting suppressed because of previous function definitions and therefore I had to reset the C{.suppress} . Atleast that is what my understanding of it is.

[exarkun]

Thanks. A few issues remain:

1. The new test asserts the wrong thing. Creating an ESMTPClient instance should not cause a deprecation warning, twisted.mail.smtp.ESMTPClient is deprecated since Twisted 12.2. ESMTPClient is not, in fact deprecated. Only the tlsMode attribute is deprecated. This means:
   1. The deprecation warning message needs to be changed
   2. The warning needs to be emitted only if the tlsMode attribute is set. Perhaps it is set as a side effect of instantiating ESMTPClient now, but if so that is a problem that needs to be addressed. Using non-deprecated Twisted APIs must not trigger deprecation warnings.
2. 12.2 has been released. The next release will probably be 12.3, so that's the version tlsMode needs to be deprecated in.
3. Pass an argument to the flushWarnings call. Otherwise you flush every single warning that has happened, and you avoid testing that the stacklevel argument passed to warnings.warn was given the correct value. The argument is a list of functions which could have triggered the warning. If the stacklevel argument has the wrong value, then the wrong function will be marked as triggering the warning and flushWarnings will tell you about this by not returning the warning, so your test can fail.

Thanks again.

[moijes12]

tlsMode being deprecated, I've removed the its initialisation from the init method of ESMTPClient. The messages have been updated.

[exarkun]

This needs to happen after #3989.

[lvh]

The bug exarkun mentioned is now closed. The patch looks okay to me, apart from the obvious fact that 12.3 is released now, so it should be deprecated in 13.0. I'll fix that and resubmit for review.

[lvh]

Same as last patch, but 12.3 -> 13.0

[lvh]

New patch added, please review.

[rwall]

Code Review

1. Patch didn't apply cleanly to trunk for me - probably since #3989.

       $ patch -p0 < ../5852.7.patch
       patching file twisted/mail/smtp.py
       Hunk #4 FAILED at 1247.
       1 out of 4 hunks FAILED -- saving rejects to file twisted/mail/smtp.py.rej
       patching file twisted/mail/test/test_smtp.py
       Hunk #1 FAILED at 6.
       Hunk #2 succeeded at 611 (offset 2 lines).
       Hunk #3 FAILED at 1518.
       2 out of 3 hunks FAILED -- saving rejects to file twisted/mail/test/test_smtp.py.rej
       patching file twisted/mail/topfiles/5852.removal
   

2. There's some trailing Whitespace in the patch.
3. There are still lines > 80 chars
4. Remove the word "Test " from test method docstring - instead just "ESMTPClient.tlsMode is deprecated"
5. This ticket title is quite misleading

I'll clean up the patch and resubmit.

[rwall]

Ready for review in attachment:5852.8.patch

1. Patch should now merge cleanly to trunk
2. Re-flowed long lines
3. Moved deprecation test to alongside existing ESMTPClient TLS tests.
4. Removed unused warning module in test_smtp.py
5. Removed the "suppress" attribute from the test method - it looked unnecessary.
6. Added epytext ref to the test docstring.

-RichardW.

[tom.prince]

This only emits a deprecation warning when writing to tlsMode, not when reading. Both should be handled. Otherwise, this looks good.

[rwall]

Now with a getattr method as suggested by tomprince

[rwall]

Replying to tom.prince:

This only emits a deprecation warning when writing to tlsMode, not when reading. Both should be handled. Otherwise, this looks good.

Ok.

• I renamed the actual tlsMode value to _tlsMode
• I added a getattr method and modified setattr to access the private var
• ...and some new tests.

I didn't try and do anything about accessing or setting it as a class variable

See attachment:5852.9.patch

-RichardW.

[thijs]

Putting this in a branch and see what the buildfarm thinks of it.

[thijs]

(In [37187]) Branching to 'deprecate-tlsmode-5852'

[thijs]

(In [37188]) Apply 5852.9.patch, refs #5852

[thijs]

Forced a  build.

[tom.prince]

One minor point:

• The AttributeError should use self.__class__.__name__ instead of hardcoding it.

Otherwise, this looks good. Thanks for your work on this. Please commit after fixing that.

[rwall]

(In [37202]) Merge deprecate-tlsmode-5852: tlsMode attribute of twisted.mail.smtp.ESMTPClient is deprecated.

Author: moijes12, rwall Reviewer: exarkun, namn, lvh, rwall, tom.prince, thijs Fixes: #5852

The tlsMode attribute of twisted.mail.smtp.ESMTPClient is deprecated.