Skip to content
This repository was archived by the owner on Oct 17, 2024. It is now read-only.

Stop percent-encoding of numbers.#4

Merged
nex3 merged 1 commit intodart-archive:masterfrom
DanTup:master
Aug 8, 2016
Merged

Stop percent-encoding of numbers.#4
nex3 merged 1 commit intodart-archive:masterfrom
DanTup:master

Conversation

@DanTup
Copy link
Copy Markdown
Contributor

@DanTup DanTup commented Jul 24, 2016

RFC 3986 states that numbers are unreserved characters:

Characters that are allowed in a URI but do not have a reserved purpose are called unreserved. These include uppercase and lowercase letters, decimal digits, hyphen, period, underscore, and tilde.

unreserved  = ALPHA / DIGIT / "-" / "." / "_" / "~"

[...]

For consistency, percent-encoded octets in the ranges of ALPHA (%41-%5A
and %61-%7A), DIGIT (%30-%39), hyphen (%2D), period (%2E), underscore
(%5F), or tilde (%7E) should not be created by URI producers

There were no tests or code specifically handling numbers so I believe this is just an oversight. This change adds numbers to the test of reserved characters as well as a couple of the additional tests that had alpha characters in.

This fixes dart-lang/core#254.

I've signed the CLA but I'm not sure how to submit this for review (the contributing file doesn't explain much); is that done here in the PR or should do I need to do something to add this to codereview.chromium.org system?

RFC 3986 states that numbers are unreserved characters:

Characters that are allowed in a URI but do not have a reserved purpose
are called unreserved. These include uppercase and lowercase letters,
decimal digits, hyphen, period, underscore, and tilde.

unreserved  = ALPHA / DIGIT / "-" / "." / "_" / "~"
[...]

For consistency, percent-encoded octets in the ranges of ALPHA (%41-%5A
and %61-%7A), DIGIT (%30-%39), hyphen (%2D), period (%2E), underscore
(%5F), or tilde (%7E) should not be created by URI producers
@devoncarew
Copy link
Copy Markdown
Contributor

/cc @nex3 @kevmoo @lrhn

@lrhn
Copy link
Copy Markdown
Contributor

lrhn commented Aug 8, 2016

Sorry for the delay, I'm back from vacation now. I have also made a similar patch CL at https://codereview.chromium.org/2225763002/ (although your CL seems to have better testing!)

@DanTup
Copy link
Copy Markdown
Contributor Author

DanTup commented Aug 8, 2016

@lrhn Cool; you're free to just trash this PR if you've already pushed code, as long as it's fixed I'm happy :-)

@nex3
Copy link
Copy Markdown
Contributor

nex3 commented Aug 8, 2016

I'm going to merge in this one, since it has tests and I think the test is a little clearer.

@nex3 nex3 merged commit 44b5c66 into dart-archive:master Aug 8, 2016
@DanTup
Copy link
Copy Markdown
Contributor Author

DanTup commented Aug 9, 2016

Great; thanks for pushing this out in a release already! :-)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Development

Successfully merging this pull request may close these issues.

Percent encode unexpectedly encodes numbers

5 participants