-
Notifications
You must be signed in to change notification settings - Fork 475
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
Port is not validated the same when constructing URI as when using uri.port() method #344
Comments
nice catch! I don't think the constructor currently throws any errors so this would be the first. It would be possible to consider anything that not a valid port as part of the path in order to maintain the liberal parsing behavior. That would allow the following normalizations:
|
Ah I see, well it seems reasonable not to introduce unique behavior just for this scenario if the constructor doesn't throw for anything else. To me the behavior you're proposing makes sense! I do think most scenarios with a faulty port in the constructor URL will actually be wanting to make that port part of the pathname. It's still making a pretty big assumption, but if that's what people come to URI.js for, then I say 👍 |
Are you up for a PR to fix this? |
@rodneyrehm I am not familiar with the code base, but I can have a look and see what I can do! Might be a little while before I have time to submit it, though |
Are you saying that
should not throw? Or would you agree that the validation logic that is part of the setters should be extracted and called in the constructor/factory as well and if any error is found it should be thrown so that users can take appropriate action without ending up with broken urls? Or would you defer the validation logic to methods like toString(), validate(), etc. ? |
@rodneyrehm why would you reject "port" as port in port(), but allow it in the constructor/factory? |
»I don't think the constructor currently throws any errors so this would be the first.« is the only "reason". The parser is pretty permissive with everything else. But I do agree that it makes little sense. I'm fine with the parser throwing errors on invalid URLs that simply make no sense. |
@rodneyrehm Thanks for clarifying 👍 Here's my first attempt at fixing this. Something similar happens when the hostname is missing, such as
I might create a separate issue/pr or append changes needed. What do you think? |
Apparently I was wrong, as
go ahead and append them. |
fixed in v1.18.11 |
Could we look into adding the same validation of the URI port when constructing URI instances from strings as we have when calling the
uri.port
method?The text was updated successfully, but these errors were encountered: