Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Optionally replace + in parseUrlencoded #62

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Optionally replace + in parseUrlencoded #62

wants to merge 2 commits into from

Conversation

paluh
Copy link
Collaborator

@paluh paluh commented Jun 1, 2018 •
edited
Loading

This PR provides a way to optionally replace + by space in query values before passing them to uri decoding. (I want to reemphasize importance of this change, so I'm coping here my arguments from purescript-uri issue).

Without this option hyper user is not able to distinguish + from %2B in original query as both are decoded to + and finally is not able parse text input values containing spaces send by all tested by me modern browsers.

I've tested on two versions of Firefox and Chromium (latest Firefox nightly 59.0a1 (2017-11-20), Chromium 62.0.3202.89 and Firefox 56.0.2) that this form:

<!DOCTYPE html>
<html>
<body>
  <form action=".">
    <input name="name with spaces" value="value with spaces" />
    <input type="submit" value="send" />
  </form>
</body>
</html>

results in + in queries (so we get "value+with+spaces"). The same behavior is observed when I add explicit enctype=application/x-www-form-urlencoded.

I know that this is not a real argument for discussion, but other libs/tools/frameworks do seem to decode + as space in query strings:

@paluh paluh force-pushed the parseUrlencoded+replacePlus branch from 9bb4477 to 76e2d9f Compare June 1, 2018 11:07
@paluh paluh force-pushed the parseUrlencoded+replacePlus branch from 76e2d9f to 4e50610 Compare June 1, 2018 11:09
@paluh
Copy link
Collaborator Author

paluh commented Jun 1, 2018

In this commit I've also dropped FromForm and ToForm type classes as I've not found any use of them (in any hyper related repository on github).
I know that this is probably drastic change and this API approach shift should be discussed more extensively... In general I think that it will be easier to replace some of hyper type classes with simple record of functions.

@owickstrom:
What do you think about these changes?

Should I open separate issue regarding refactoring of some type classes into plain records (so I think we will have more flexible and extensible API and simpler type inference)?

@paluh
Copy link
Collaborator Author

paluh commented Jun 1, 2018

Travis detected that examples from docs use ToForm and FromForm type classes so I should somehow fix them... I'm not sure if I want to bring these classes back (and add additional parameter to FromForm method with Urlencode.Options). Any comments?

@paluh
Copy link
Collaborator Author

paluh commented Jun 1, 2018

I've tried to fix example for form handling, but I'm not sure if I should finish this fix or drop it all together.

I don't know if we should provide an API or make any assumptions about API for validation (which assumes for example error type to be String and not for example some Variant (e | row)).

As an author of validation libraries (https://github.com/paluh/purescript-polyform) I think that such a validation glue code could be easily extracted into separate library.

@paluh paluh changed the title Parse urlencoded+replace plus Optionally replace + in parseUrlencoded Jun 1, 2018
@paluh
Copy link
Collaborator Author

paluh commented Jul 15, 2018

Can I merge this (dropping FromForm and ToForm type classes)?

@paluh
Copy link
Collaborator Author

paluh commented Jul 15, 2018

@owickstrom ping...

@owickstrom
Copy link
Collaborator

Sorry for unresponsiveness! Yeah, I think ultimately these form-related things shouldn't go in the core Hyper library. I did the initial versions a bit to feature-heavy just to try out things. I'm happy you remove them and remove the corresponding docs. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@paluh @owickstrom