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

fix utf8 in url #1143

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix utf8 in url #1143

wants to merge 1 commit into from

Conversation

alexsorinpop
Copy link

fixed utf8 in urls and added a test for it

@veryrusty
Copy link
Member

Thanks @alexsorinpop ! Looks ok and matches rfc3986.

This also decodes route params to characters (which is desirable). We should add tests for this to make it explicit. I'm (slowly) working through some other corner cases (eg. when there are undecoded path elements, such as when behind a proxy).

@xsawyerx
Copy link
Member

👍

This reminds me that we should attempt to use Unicode::UTF8 if available. Path::Tiny does that1, 2, 3. Does anyone want to open a ticket for this enhancement?

@xsawyerx xsawyerx added the Bug label Mar 28, 2016
@alexsorinpop
Copy link
Author

So after more digging around it seems that as with every utf8 issue you can't just add that and fix utf8. Go figure.

I found that the STDERR is not opened as utf8 so it might crash the whole app when a utf8 url errors with some syntax in template toolkit and what I found to be really bizarre is that the session is not always remembered... sometimes it will remember your session and the next url that you open will not work anymore. I tested this with ascii urls and I never get that, but I cannot say that I found a way to reproduce this behavior all the time with utf8 urls. I will still dig around

@xsawyerx
Copy link
Member

Any updates about this?

@cromedome
Copy link
Contributor

@alexsorinpop It's been a while :) Any updates to share? Thanks!

@xsawyerx
Copy link
Member

xsawyerx commented Apr 1, 2022

I suggest we close this and open a ticket that refers to this implementation and discussion to fix the matter.

@cromedome
Copy link
Contributor

cromedome commented Jul 26, 2023

@xsawyerx seconded. Are you volunteering to open the new ticket? ;)

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

Successfully merging this pull request may close these issues.

4 participants