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

is.url does not accept valid characters (?, +, %) and allow white spaces #106

Closed
marqu3z opened this issue Mar 30, 2015 · 6 comments
Closed

Comments

@marqu3z
Copy link

marqu3z commented Mar 30, 2015

is.url("foo.com bar") // true
is.url("foo.com/something.php?foo=bar") // false
is.url("foo.com/something%20else.php") // false
is.url("foo.com/page.php?foo=one+two") // false

Is this a desired behaviour?

@fslone
Copy link

fslone commented Mar 30, 2015

I'd like to see this fixed as well. I personally use the regex from @dperini for URL's but that returns false for any URL not prefixed with http/https/ftp so it would change the behavior of the library slightly. You can see that regex here:

https://gist.github.com/dperini/729294

If everyone would be in favor of using that regex I can make the change and write the test cases. Just let me know.

@marqu3z
Copy link
Author

marqu3z commented Mar 30, 2015

I was looking at the same gist.
The author made a comparison table here: https://mathiasbynens.be/demo/url-regex

@fslone
Copy link

fslone commented Mar 30, 2015

yeah that's where I got it from :P

@dperini
Copy link

dperini commented Mar 30, 2015

@fslone,
if there is no need to validate the protocol scheme (http/https/ftp) remove the following part:

// protocol identifier
"(?:(?:https?|ftp)://)" +

if there is no need to validate user/password (user:pass) remove the following part:

// user:pass authentication
"(?:\\S+(?::\\S*)?@)?" +

the entire IPV4 part could also be removed to shorten the regex and make it more efficient.

Thumbs up !

@dperini
Copy link

dperini commented Mar 30, 2015

@fslone,
the protocol part could also be made optional instead of removing it:

// protocol identifier
"(?:(?:https?|ftp)://)?" +

by just adding a question mark after the grouping parenthesis.

@fslone
Copy link

fslone commented Mar 30, 2015

@dperini thanks, you're the man. I'm going to make a few changes to the regex like Diego has recommended and I'll submit a pull request for this change shortly.

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

No branches or pull requests

4 participants