Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upAvoid double encoding of query params when using FoundationClient #244
Conversation
steffendsommer
requested review from
tanner0101
and
Joannis
Mar 8, 2018
tanner0101
approved these changes
Mar 12, 2018
tanner0101
added this to the 2.2.4 milestone
Mar 12, 2018
tanner0101
added
the
bug
label
Mar 12, 2018
tanner0101
self-assigned this
Mar 12, 2018
tanner0101
merged commit 0ecc50f
into
vapor-2
Mar 12, 2018
tanner0101
deleted the
bugfix/avoid-double-encoding-with-foundation-client
branch
Mar 12, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
steffendsommer commentedMar 8, 2018
•
edited
Currently when using
FoundationClientquery params will be encoded twice since Vapor has already percent-encoded the query at the same it's set toURLComponents. Since the value is expected to not be percent-encoded at the time of assignment I suggest a naive solution where we simple decode it before setting it. I do not have an overview of any potential implications which is why I suggested this on Slack where @tanner0101 seemed fine with the approach.Please note that I created a
vapor-2(based on the 2.2.3 release) branch in order to be able to open this PR. Let me know if you had another branching strategy in mind.