fix(isURL): allow URLs with colon and no port#1751
fix(isURL): allow URLs with colon and no port#1751profnandaa merged 1 commit intovalidatorjs:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1751 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 102 102
Lines 2029 2029
Branches 457 457
=========================================
Hits 2029 2029
Continue to review full report at Codecov.
|
|
Hi @tux-tn Can I have your feedback about this? 🙏 |
tux-tn
left a comment
There was a problem hiding this comment.
Hello @MatteoPierro and thank you for your PR!
Is an URL with a colon and no port allowed in IETF standards? if yes can you provide a source?
According to the two RFCs concerning URLs and URls,http://example.com: shouldn't be valid
RFC 3986 ( Uniform Resource Identifier (URI): Generic Syntax)
URI producers and normalizers should omit the port component and its ":" delimiter if port is empty or if its value would be the same as that of the scheme's default
RFC 1738 ( Uniform Resource Locators (URL))
port
The port number to connect to. Most schemes designate protocols that have a default port number. Another port number may optionally be supplied, in decimal, separated from the host by a colon. If the port is omitted, the colon is as well.
|
Hi @tux-tn If I understand correctly RFC 2396 revises and replaces the RFC 1738.
About the RFC 3986. I understand that defines only the behavior of producers and normalizers. It's not about the validity of URIs.
So |
There was a problem hiding this comment.
If I understand correctly RFC 2396 revises and replaces the RFC 1738.
In deed, you are correct. I missed RFC 2396 and allowing colon without port should be a valid behaviour.
Approving your PR! Thank you for your work and for addressing my comment 🎉
|
Great 🎉 Since it's my first contribution to this project, do I need something else for having this merged? |
No, you just need to wait for @profnandaa to do another review if needed and merge it if everything is okay. BTW, forgot to ask but did you pick the issue for hacktoberfest? |
|
Yes, for the hacktoberfest 😅 |
|
@MatteoPierro thank you, added the hacktoberfest label ! |
profnandaa
left a comment
There was a problem hiding this comment.
LGTM, thanks for your contrib!
fix(isURL): allow URLs with column and no port
This solves the issue #1584
Checklist