Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fixed utf-8 pattern in router option
- Loading branch information
Showing
2 changed files
with
25 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
d5283af
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This modification breaks routes that include a urlencoded '/' in a parameter.
d5283af
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is unfortunate, I would think utf8 characters are more likely to be found in path segments over encoded / 's.
d5283af
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lemon-lime For more reviews, please tell me an example of a url that failed
d5283af
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markstory do you have an example of an url that includes utf8 characters? I'm not quite sure what you mean by this. I spent an entire afternoon trying to solve why my urlencoded parameter wouldn't get through correctly, so I'm now kinda curious to understand why it has to behave like this.
d5283af
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JelmerD URLs with non-ascii characters, like in the test. If this code is causing a problem please open an issue. Commit comments are easy to lose track of and find again.
d5283af
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markstory Edited:
I see. So I kinda understand why you're now doing this, but I think it this is not the behaviour we want because in my opinion it doesn't make sense.
I will open a ticket for discussion. Because a base64-encoded string might contain a
/
. So if you parse a string like this, which should be perfectly fine, it fails the request, because it now splits this one parameter into two.**
kind of works, as long as this base64 encoded parameter is at the end. But what if I parse 2 base64 encoded params? Or a base64 encoded param followed by another parameter. That's now impossible.d5283af
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can find many examples on this website.
d5283af
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created a bug report #15269