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.

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

Parameter value is not url encoded by default #124

Closed
balamaci opened this issue May 26, 2015 · 5 comments
Closed

Parameter value is not url encoded by default #124

balamaci opened this issue May 26, 2015 · 5 comments

Comments

@balamaci
Copy link
Collaborator

I don't know if it's intentional or not, it seems in the sources there is a comment.
https://github.com/decebals/pippo/blob/cbb8863bb0dbee1f4154a90774144a3e2dbdeeb6/pippo-core/src/main/java/ro/pippo/core/route/DefaultRouter.java#L407

Maybe I'm used to wicket but I was expecting the value to be automatically encoded.

decebals added a commit that referenced this issue May 26, 2015
decebals added a commit that referenced this issue May 26, 2015
@gitblit
Copy link
Collaborator

gitblit commented May 26, 2015

@decebals Why not just use URLEncoder instead of roll-your-own?

@decebals
Copy link
Member

@gitblit My first intention was to use URLEncoder but I read some unfavorable comments (for example http://stackoverflow.com/questions/10786042/java-url-encoding-of-query-string-parameters).
Sure I have no problem to use URLEncoder.
I remarked some differences. For example "Decebal Suiu" is encoded "Decebal+Suiu" with URLEncoder and "Decebal%20Suiu" with my function.
Please tell me if you want to use URLEncoder to make the change.

decebals added a commit that referenced this issue May 26, 2015
@decebals
Copy link
Member

@gitblit
Copy link
Collaborator

gitblit commented May 26, 2015

Totally agree. I'm familiar with that article which is why I am wary of roll-your-own solutions for encoding. I think the best solution is to add a proven Apache dependency with proper encoding utils but that is a slippery slope and breaks our nice claim that Pippo-Core has a single dependency - logging - and that's it.

@decebals
Copy link
Member

Added a notice in Reverse routing section on the documentation site.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants