-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
bpo-42967: coerce bytes separator to string in urllib.parse_qs(l) #24818
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
Off topic: wow the bot is more alert than I thought. I never knew mentioning a bpo in the description would also cause it to link |
Would it make sense to disallow mixing types (e.g. string qs with bytes separator)? |
Yeap I thought about that for a while :). The problem is it will break existing code: # currently this works
urllib.parse_qsl(b"qwerty")
# but after patch:
urllib.parse_qsl(b"qwerty")
# TypeError: Cannot mix str and non-str arguments Because the default separator argument is a string |
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.
@Fidget-Spinner - This change looks good to me.
Ouch! - We missed this in the original PR. Using the same ticket or a new ticket is fine with with me.
Good thing this isn't a regression against prior versions (IMO, I may be wrong here). Because currently, the following still works, and all existing user code should look like this: urllib.parse_qsl("qwerty")
urllib.parse_qsl(b"qwerty") It's only triggered when Edit: Hmm considering this is bugfix. I'm guessing backport to 3.8 only? |
Misc/NEWS.d/next/Library/2021-03-11-00-31-41.bpo-42967.2PeQRw.rst
Outdated
Show resolved
Hide resolved
This PR is stale because it has been open for 30 days with no activity. |
A very gentle ping @orsenthil |
Oh, thanks @Fidget-Spinner. Sorry, I missed this. |
@orsenthil: Please replace |
Thanks @Fidget-Spinner for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9. |
Thanks @Fidget-Spinner for the PR, and @orsenthil for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
…thonGH-24818) * coerce bytes separator to string * Add news * Update Misc/NEWS.d/next/Library/2021-03-11-00-31-41.bpo-42967.2PeQRw.rst (cherry picked from commit b38601d) Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
GH-25344 is a backport of this pull request to the 3.9 branch. |
…thonGH-24818) * coerce bytes separator to string * Add news * Update Misc/NEWS.d/next/Library/2021-03-11-00-31-41.bpo-42967.2PeQRw.rst (cherry picked from commit b38601d) Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
GH-25345 is a backport of this pull request to the 3.8 branch. |
…-24818) (#25345) * coerce bytes separator to string * Add news * Update Misc/NEWS.d/next/Library/2021-03-11-00-31-41.bpo-42967.2PeQRw.rst (cherry picked from commit b38601d) Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com> Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
urllib.parse_qsl
previously (and currently) deals with bytes query string by coercing them to string internally. After bpo-42967 was fixed, a newseparator
argument was added. This patch follows the rest of parse_qsl by coercing theseparator
to string too.https://bugs.python.org/issue42967