-
Notifications
You must be signed in to change notification settings - Fork 331
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
encode space to '%20' as per url standard #928
base: main
Are you sure you want to change the base?
Conversation
@valenting let me know what you think! |
Previously the space character was exclusively encoded to '+'. This is wrong, as the URL Standard [0] specifies that the default is '%20'. Another function has been introduced as well, which replicates the old behavior and converts spaces to '+'. Notice that this breaks the default behavior and could lead to bugs. [0]: https://url.spec.whatwg.org/#string-percent-encode-after-encoding Fixes: servo#927 Fixes: servo#888 Signed-off-by: Gabriel Goller <[email protected]>
I am wary about breaking upstream users, especially the browser users that use this crate (who will likely care about nitty gritty url spec behavior) @valenting @mrobinson can you check whether the Gecko/Servo callsites are all ones that need spaceIsPlus by default? We can invert which behavior is the default here to solve this if needed. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #928 +/- ##
=======================================
Coverage ? 81.88%
=======================================
Files ? 20
Lines ? 3550
Branches ? 0
=======================================
Hits ? 2907
Misses ? 643
Partials ? 0 ☔ View full report in Codecov by Sentry. |
As far as I can tell servo doesn't use this method https://github.com/search?q=repo%3Aservo%2Fservo+form_urlencoded&type=code Nor does Gecko https://searchfox.org/mozilla-central/search?q=byte_serialize&path=&case=false®exp=false This will be a breaking change anyway. |
Since this crate is used a lot should we implement this in a way that is non breaking (with reversed defaults, strongly documented), and perhaps file an issue for a potential 3.0 where we start listing breaking changes? I suspect 99% of users will not care about this anyway. I feel like doing a 3.0 just for this is a bit much. |
Fixed link to url spec. Signed-off-by: Gabriel Goller <[email protected]>
This is a breaking change, although a very small one. As you said it will probably only affect those who care about the url spec, which in turn, should know this case already!
I have to disagree with the reversed defaults. It's better if we do a 3.0 release and keep it this way, otherwise we will not be 'url-spec compliant'. The url-spec clearly states that the default-behavior should be '%20' and not '+'. IMO a 3.0 release (or even a minor release if we're feeling adventurous :) ) is the way to go! |
It would be nice to have an opt-in way to do this at least (which wouldn't have to be a breaking change hopefully), that way users who need this encoding can still use this crate while plans for a breaking change are worked out. If nothing else this will probably break a bunch of tests. |
Previously the space character was exclusively encoded to '+'. This is wrong, as the URL Standard 0 specifies that the default is '%20'. Another function has been introduced as well, which replicates the old behavior and converts spaces to '+'.
Notice that this breaks the default behavior and could lead to bugs.
Fixes: #927
Fixes: #888