-
Notifications
You must be signed in to change notification settings - Fork 48
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
Allow non-RFC 3986-compliant URLs #45
Conversation
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.
Thanks. I'm not confident that the parser can be this loose, so I'd like to change it to a bit more conservative implementation with which the specs you provided will pass.
|
||
REGEXP = { | ||
ABS_PATH: /\A[^?#]*\z/ | ||
} |
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 constant does not seem to be used in any way.
if uri.is_a?(URI::Generic) | ||
uri | ||
elsif uri = String.try_convert(uri) | ||
parse(uri) |
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.
I think we should try the default URI.parse() first and then our custom parser only if it fails.
require 'singleton' | ||
|
||
class HTTP::Cookie::URIParser | ||
include Singleton |
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 don't need a class or instance for providing these methods. Make it a module and provide module functions.
require 'singleton' | |
class HTTP::Cookie::URIParser | |
include Singleton | |
module HTTP::Cookie::URIParser | |
module_function |
} | ||
|
||
def parse(uri) | ||
m = / |
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.
Using %r{…}
will eliminate the need for escaping the slash character.
Yes, I believe it does. Thanks! |
@knu This solution solved some of the errors I saw but also some invalid URIs with curly braces in path segments. Would you be open to adding curly braces to the |
This PR has objective as #44 but takes a different approach.
It avoids polymorphism. It still represents URLs as
URI
instances but uses a very liberal URL parser.