Opened 4 years ago

Closed 2 years ago

#4963 task closed fixed (fixed)

Deprecate twisted.mail.protocols.SSLContextFactory

Reported by: exarkun Owned by: exarkun
Priority: normal Milestone:
Component: mail Keywords: easy
Cc: moijes12@… Branch: branches/deprecate-redundant-ssl-context-4963
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

SSLContextFactory provides no particular value over something like twisted.internet.ssl.DefaultOpenSSLContextFactory (in fact, less, since it allows SSLv2). We should change our internal usage of the former (probably to the latter) and deprecate the former.

Attachments (3)

4963.patch (2.3 KB) - added by moijes12 2 years ago.
doubt.patch (3.0 KB) - added by moijes12 2 years ago.
4963.2.patch (6.2 KB) - added by moijes12 2 years ago.
The tests for pop3sBackwardCompatibilty now use DefaultOpenSSLContextFactory. For this, a copy of server.pem has been placed in twisted/mail/test. All other changes that were supplied in the first patch are in here too. So, now, SSLContextFactory has been deprecated and its usage has been replaced with DefaultOpenSSLContextFactory.

Download all attachments as: .zip

Change History (14)

Changed 2 years ago by moijes12

comment:1 Changed 2 years ago by moijes12

  • Cc moijes12@… added
  • Keywords review added

comment:2 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to moijes12

Thanks for working on this, moijes12. It looks like your patch does a good job addressing the second half of this ticket, but I don't think it covers the first half.

Twisted itself should never trigger its own deprecation warnings. Therefore, when deprecating a Twisted API, we also need to remove all usages of that API within Twisted itself. (You may have noticed that we don't always do a good job of this, by the number of warnings emitted when running the test suite. This is an area where we need to improve.)

Apart from that:

  1. make sure you wrap lines at 80 columns or before
  2. name tests like test_foo instead of testFoo
  3. Don't start test method docstrings with "Test ..."

Thanks again!

Changed 2 years ago by moijes12

comment:3 Changed 2 years ago by moijes12

  • Keywords review added
  • Owner moijes12 deleted

comment:4 Changed 2 years ago by moijes12

  • Keywords review removed

Changed 2 years ago by moijes12

The tests for pop3sBackwardCompatibilty now use DefaultOpenSSLContextFactory. For this, a copy of server.pem has been placed in twisted/mail/test. All other changes that were supplied in the first patch are in here too. So, now, SSLContextFactory has been deprecated and its usage has been replaced with DefaultOpenSSLContextFactory.

comment:5 Changed 2 years ago by moijes12

  • Keywords review added

comment:6 Changed 2 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/deprecate-redundant-ssl-context-4963

(In [34763]) Branching to 'deprecate-redundant-ssl-context-4963'

comment:7 Changed 2 years ago by exarkun

(In [34765]) Just use the server.pem from the source tree, no need to copy it to a temporary file first.

refs #4963

comment:8 Changed 2 years ago by exarkun

(In [34766]) - Rewrite the test method docstring in the conventional style

  • Remove the call of SSLContextFactory, attribute access is enough
  • wrap the long warning assertion line to < 80 columns

refs #4963

comment:9 Changed 2 years ago by exarkun

(In [34767]) - Remove trailing whitespace

  • Wrap long deprecation line to < 80 columns

refs #4963

comment:10 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner set to exarkun

Thanks! I made a few small changes to the patch, mostly whitespace. Otherwise it looks good, so I'll apply it. Thanks again!

comment:11 Changed 2 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(In [34769]) Merge deprecate-redundant-ssl-context-4963

Author: moijes12
Reviewer: exarkun
Fixes: #4963

Deprecate twisted.mail.protocols.SSLContextFactory.

Note: See TracTickets for help on using tickets.