Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
When you remove positional parameters, you have to make the following parameters keyword-only.
Sorry, something went wrong.
There was a problem hiding this comment.
Oh right, done. I didn't document this change. Is it worth to document it?
Sorry, something went wrong.
There was a problem hiding this comment.
Were similar changes documented in the past?
Please add tests that only such positional arguments are accepted. In most cases None is a valid argument both for the first deleted parameter and for the first remaining parameter, so you can just add None as an excessive positional argument.
Sorry, something went wrong.
There was a problem hiding this comment.
Were similar changes documented in the past?
In commit 9baf242, I removed a buffering parameter in bz2.BZ2File. I see that I wrote ".. versionchanged:: 3.9 (...) The compresslevel parameter became keyword-only." in the doc.
Honestly, I don't think that it should be documented. I don't expect people to call such constructor with 6 parameters or more. I'm only aware of people who called CodeType() with tons of arguments: problem solved with CodeType.replace() method addition.
Sorry, something went wrong.
There was a problem hiding this comment.
Why is it removed?
Sorry, something went wrong.
There was a problem hiding this comment.
If you provide a SSLContext, you get different protocol than FTP_TLS.ssl_version. This removal is not required, it's more to make the API consistent with the other modified modules: the SSLContext constructor is the only place setting default parameter values. Do you prefer to keep it?
Sorry, something went wrong.
There was a problem hiding this comment.
I do not know whether it is documented, but it specifies the default protocol if you do not provide an SSLContext. The user can override it in FTP_TLS or subclasses.
Who added this attribute? Ask him.
Sorry, something went wrong.
There was a problem hiding this comment.
All protocols except PROTOCOL_TLS, TLS_CLIENT, and TLS_SERVER are deprecated anyways because OpenSSL is going to remove them eventually.
Sorry, something went wrong.
There was a problem hiding this comment.
@tiran: Do you see a reason to keep ssl_version to specify a different "ssl version" when context parameter is omitted?
The practical problem is more that applications relying on this feature may not notice that overriden ssl_version are now ignored silently.
Note: I hate mentions of "ssl", since SSL no longer exists, it was replaced with TLS, but the Python module is still called "ssl"... as OpenSSL ;-)
Sorry, something went wrong.
There was a problem hiding this comment.
There is no reason to keep the variable here. ssl.PROTOCOL_TLS_CLIENT is the only correct and fully supported value. In 3.13 I will remove all the other protocols. Only ssl.PROTOCOL_TLS_CLIENT and ssl.PROTOCOL_TLS_SERVER will stay.
Sorry, something went wrong.
|
Oh, HTTPSConnection of urllib.request has context and check_hostname parameters, and context can be None. http.client code to create the default context is non trivial, I don't want to copy it in urllib/request.py. Maybe http.client check_hostname parameter should be kept. What do you think @serhiy-storchaka? |
Sorry, something went wrong.
|
Right now, 4 tests are failing because of the urllib issue: test_site test_ssl test_urllib test_urllib2_localnet |
Sorry, something went wrong.
There was a problem hiding this comment.
I do not know whether it is documented, but it specifies the default protocol if you do not provide an SSLContext. The user can override it in FTP_TLS or subclasses.
Who added this attribute? Ask him.
Sorry, something went wrong.
There was a problem hiding this comment.
If it is the right way to do, should this change be backported as a bug fix? I think it deserves a separate issue.
Sorry, something went wrong.
There was a problem hiding this comment.
That's a good question. Sadly, I don't know well the ftplib module. I'm not comfortable to change it in a stable Python version. In case of doubt, I prefer to leave the code as it is in stable branches, unless someone reports a bug.
Sorry, something went wrong.
|
Oh, HTTPSConnection of urllib.request has context and check_hostname parameters, and context can be None. http.client code to create the default context is non trivial, I don't want to copy it in urllib/request.py. Maybe http.client check_hostname parameter should be kept. Specifying check_hostname for urllib.request.HTTPSConnection currently produces a warning, right? Therefore it should be removed. |
Sorry, something went wrong.
There was a problem hiding this comment.
All protocols except PROTOCOL_TLS, TLS_CLIENT, and TLS_SERVER are deprecated anyways because OpenSSL is going to remove them eventually.
Sorry, something went wrong.
There was a problem hiding this comment.
SSLv2 and SSLv3 are no longer supported. You can remove the AUTH SSL block.
Sorry, something went wrong.
There was a problem hiding this comment.
When I read Modules/_ssl.c, it's not obvious that SSL is no longer supported. There is still conditonal code:
These constants are still documented but marked as "deprecated" in the doc: https://docs.python.org/dev/library/ssl.html#ssl.PROTOCOL_SSLv2
I would prefer to fully remove support for SSL in the _ssl/ssl modules, before changing the ftplib module.
Another example: https://docs.python.org/dev/library/ssl.html#ssl.OP_NO_SSLv3 still exists (at least in the doc).
Sorry, something went wrong.
There was a problem hiding this comment.
It's documented in ssl.rst. I'm using the word "supported" in the sense of a social contract. The presence of the constants do not mean that a user can expect they are doing anything sensible.
Sorry, something went wrong.
There was a problem hiding this comment.
You can remove the check. It is performed by SSLContext internally.
Sorry, something went wrong.
|
I wrote PR #94193 to prepare urllib.request. |
Sorry, something went wrong.
|
URLopener of urllib.request has key_file and cert_file parameters which are passed to http.client.HTTPSConnection. These parameters are not directly deprecated, but the whole URLopener class is deprecated since Python 3.3! |
Sorry, something went wrong.
|
URLopener of urllib.request has key_file and cert_file parameters which are passed to http.client.HTTPSConnection. I created PR #94232 to fix this issue. |
Sorry, something went wrong.
|
I created PR #94232 to fix this issue. Merged. I rebased this PR on top of it. Currently, the PR still removes FTP_TLS.ssl_version. This removal seems controversial, I may just revert it. |
Sorry, something went wrong.
|
I created PR #94232 to fix this issue. Merged. I rebased this PR on top of it. Currently, the PR still removes FTP_TLS.ssl_version. This removal seems controversial, I may just revert it. I suggest to remove it. In a future OpenSSL version it won't be able to change the TLS version. Even now it cannot be used to switch to TLS 1.3-only. |
Sorry, something went wrong.
|
FTP_TLS.ssl_version was added with FTP_TLS by commit ccd5e02 in 2009: patch by @giampaolo, merged by @pitrou. Changes: |
Sorry, something went wrong.
|
About removing resp = self.voidcmd('AUTH SSL'): I wrote PR #94312 to clarify that Python no longer supports SSLv2. |
Sorry, something went wrong.
|
I tried to run a code search with: ./search_pypi_top.py PYPI-2022-06-24-TOP-5000/ '(keyfile|key_file|certfile|cert_file|check_hostname)=' -o cert_file.txt -q
Problem: these parameter names are common and they are many usages which remain perfectly fine and supported, like (example from Python ssl module documentation): context.load_cert_chain(certfile="mycertfile", keyfile="mykeyfile")
The code search finds around 1976 lines in 188 projects. |
Sorry, something went wrong.
|
@tiran @serhiy-storchaka: Would you mind to review the updated PR? |
Sorry, something went wrong.
There was a problem hiding this comment.
You missed updating the documentation.
Please add also new tests in place of the deleted tests.
Sorry, something went wrong.
| """IMAP4 client class over SSL connection | ||
|
|
||
| Instantiate with: IMAP4_SSL([host[, port[, keyfile[, certfile[, ssl_context[, timeout=None]]]]]]) | ||
| Instantiate with: IMAP4_SSL([host[, port[, ssl_context[, timeout=None]]]]) |
There was a problem hiding this comment.
This line is completely redundant, because the output of pydoc already contains the constructor signature. The same in other classes in this module, and maybe in other modules.
I would prefer to remove this line and edit the following parameters descriptions, and do it before merging this PR to make backporting easier.
Sorry, something went wrong.
There was a problem hiding this comment.
This PR is already big enough. If you want to enhance the documentation, please change it in a separated change. I don't see how updating this doc is a pre-requirement to remove the two parameters.
Sorry, something went wrong.
|
I rebased my PR and addressed @serhiy-storchaka's review. @serhiy-storchaka: Would you mind to review my updated PR? |
Sorry, something went wrong.
|
I merged my PR which was open for 6 months. I prefer to merge it as soon as possible at the beginning of the Python 3.12 development cycle, to get feedback as soon as possible. If someone sees rooms for enhance, I suggest to open a separated PR ;-) |
Sorry, something went wrong.
|
Thanks a lot @tiran and @serhiy-storchaka for the many reviews! |
Sorry, something went wrong.
|
Please see PR #100431 for some docs updates I missed here. |
Sorry, something went wrong.
Remove the keyfile, certfile and check_hostname parameters,
deprecated since Python 3.6, in modules: ftplib, http.client,
imaplib, poplib and smtplib. Use the context parameter (ssl_context
in imaplib) instead.
ftplib: Remove the FTP_TLS.ssl_version class attribute: use the
context parameter instead.