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

Added URL Encoding for form posting #748

Merged
merged 2 commits into from Aug 7, 2014

Conversation

Projects
None yet
2 participants
@etcimon
Contributor

etcimon commented Jul 28, 2014

I'm currently configuring a Paypal IPN module where the post data needs to be mirrored back for verification, and I couldn't find this feature anywhere.

usage: req.form.toFormEncodedString() or req.params.toURLEncodedString()

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 29, 2014

Member

I was actually surprised that this hasn't been there before, but you are right. The only thing that would be nice, would be to maybe name the function formEncode. That would require a template constraint, though, so that it overloads against the string version. Something like if (__traits(compiles, (){ foreach(k, v; map){} }()) should work.

BTW, there are some little stylistic nitpicks, like the format of the doc comment (also missing the trailing full stop), missing space between ) {. I'd also write the if-else as if (amp) ret.put('&');->else amp = true;, because reduced vertical space often improves readability for such trivial lines of code.

Finally, it would be nice to split the function into a version that takes a ref R dst as the first argument and directly writes to an output range and another version that uses Appender + the first version to build an actual string.

If you want, you can do any of these, or just let me do it after the merge.

Member

s-ludwig commented Jul 29, 2014

I was actually surprised that this hasn't been there before, but you are right. The only thing that would be nice, would be to maybe name the function formEncode. That would require a template constraint, though, so that it overloads against the string version. Something like if (__traits(compiles, (){ foreach(k, v; map){} }()) should work.

BTW, there are some little stylistic nitpicks, like the format of the doc comment (also missing the trailing full stop), missing space between ) {. I'd also write the if-else as if (amp) ret.put('&');->else amp = true;, because reduced vertical space often improves readability for such trivial lines of code.

Finally, it would be nice to split the function into a version that takes a ref R dst as the first argument and directly writes to an output range and another version that uses Appender + the first version to build an actual string.

If you want, you can do any of these, or just let me do it after the merge.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 29, 2014

Contributor

If you want, you can do any of these, or just let me do it after the merge.

I decided to change the separator to a string rather than a char to allow &;, I saw this in the parse but I'm unsure if it should be valid.

Contributor

etcimon commented Jul 29, 2014

If you want, you can do any of these, or just let me do it after the merge.

I decided to change the separator to a string rather than a char to allow &;, I saw this in the parse but I'm unsure if it should be valid.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 29, 2014

Contributor

I had to add parseURLEncodedString which fills up FormFields with a regular URL query string with spaces as %20. Do you think this is appropriate?

Contributor

etcimon commented Jul 29, 2014

I had to add parseURLEncodedString which fills up FormFields with a regular URL query string with spaces as %20. Do you think this is appropriate?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 29, 2014

Member

I decided to change the separator to a string rather than a char to allow &;, I saw this in the parse but I'm unsure if it should be valid.

That's just either '&' or ';' used as a field separator. indexOfAny just happens to use a string to specify the possible matches. ";" is recommended to be supported by the W3C, because "&" is a little cumbersome to specify within HTML.

I had to add parseURLEncodedString which fills up FormFields with a regular URL query string with spaces as %20. Do you think this is appropriate?

What's the difference to parseURLEncodedForm? Just the use of urlDecode vs. formDecode? formDecode is just like urlDecode, just that it additionally supports + as the space placeholder. Since + is explicitly specified as the placeholder for www-forms, this should always be the correct choice.

Member

s-ludwig commented Jul 29, 2014

I decided to change the separator to a string rather than a char to allow &;, I saw this in the parse but I'm unsure if it should be valid.

That's just either '&' or ';' used as a field separator. indexOfAny just happens to use a string to specify the possible matches. ";" is recommended to be supported by the W3C, because "&" is a little cumbersome to specify within HTML.

I had to add parseURLEncodedString which fills up FormFields with a regular URL query string with spaces as %20. Do you think this is appropriate?

What's the difference to parseURLEncodedForm? Just the use of urlDecode vs. formDecode? formDecode is just like urlDecode, just that it additionally supports + as the space placeholder. Since + is explicitly specified as the placeholder for www-forms, this should always be the correct choice.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 29, 2014

Contributor

Looks like the unittest are passing for me without parseURLEncodedString so I'll just go without

Contributor

etcimon commented Jul 29, 2014

Looks like the unittest are passing for me without parseURLEncodedString so I'll just go without

etcimon added some commits Jul 29, 2014

Added formEncode, urlEncode
Cleaned up a bit

Reorganized the file

More bug fixes, added unit tests

Changed separator to char

Use parseURLEncodedForm instead of _String

Rewrote documentation for public functions

Fix unit test
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 7, 2014

Member

Thanks, this looks good.

Member

s-ludwig commented Aug 7, 2014

Thanks, this looks good.

s-ludwig added a commit that referenced this pull request Aug 7, 2014

Merge pull request #748 from etcimon/url-encode-form
Added URL Encoding for form posting

@s-ludwig s-ludwig merged commit 450c2b6 into vibe-d:master Aug 7, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment