-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
bpo-43433: Reattach query parameters after parsing URI #25045
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
According to Chris A., this patch fixes a regression introduced by 9c4c459, authored by Dong-he Na and merged by Serhiy for bpo-43614. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not fragment be added too?
Also the code is not equivalent to the past code if path is empty. I do not know what is better, but all this should be tested.
Please add tests and NEWS entry.
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 |
I'm not the one to analyze this as I don't actually use xmlrpc. Will ping the person with the original concern via python-list. |
The "new" behavior breaks the DokuWiki XMLRPC interface (which is using the query string for authentication). I maintain a client package for DokuWiki which is broken by the behavior change addressed in this PR (kynan/dokuwikixmlrpc#8). My opinion:
cc @Aluriak who originally reported this (to me and python-list). Anything I can help with to move this along? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest also restoring the previous behavior where /RPC2
is only used as the default if neither path nor query string are set. I'm indifferent on also adding fragment.
@@ -1426,6 +1426,8 @@ def __init__(self, uri, transport=None, encoding=None, verbose=False, | |||
raise OSError("unsupported XML-RPC protocol") | |||
self.__host = p.netloc | |||
self.__handler = p.path or "/RPC2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.__handler = p.path or "/RPC2" | |
self.__handler = p.path |
@@ -1426,6 +1426,8 @@ def __init__(self, uri, transport=None, encoding=None, verbose=False, | |||
raise OSError("unsupported XML-RPC protocol") | |||
self.__host = p.netloc | |||
self.__handler = p.path or "/RPC2" | |||
if p.query: | |||
self.__handler += "?" + p.query | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not self.__handler: | |
self.__handler = "/RPC2" | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also the same side with @serhiy-storchaka about testing and NEWS.d
we need to add the test code for checking self.__handler
value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer PR #25057 which reuses urlunsplit() and adds tests.
Yep, I also like that PR. |
PR #25057 was merged instead. |
Reinstate pre-3.9 behaviour where query parameters would be carried through in the path component.
https://bugs.python.org/issue43433