Expand/augment UrlHelper::isValid() to improve correctness and handle more URL types

created test for valid_url() used your proposed function for testing, function has some errors.

Here are my test results and the patch for the test is included.

test patch will not work with this function, made alterations after creating the patch to work with mentioned function, will roll different patch that works with the following function tomorrow.

** Note this message does not affect the test results show.

Invalid URL checking is not a feature - it's a bug. My users have reported that carets (^) do not work. Indeed they are not checked for in 5.x, 6.x, or 7.x. I have trouble reading "preg" patterns so I can't tell if your proposed patch fixes this.

Since this is causing "valid_url()" to fail, I'm changing this to a bug and hope it gets not only committed to 7, but all the way back to 5.

Here's one to add to your test:

http://thoth.lib.ucalgary.ca/uhtbin/cgisirsi/0/X/0/57/5/3?searchdata1=^C2671933&searchfield1=GENERAL^SUBJECT^GENERAL^^&user_id=WEBSERVER 

I just checked, your patch does not include the caret.

  if (preg_match('`[^a-z0-9\-._~%^!$&\'()*+,;=/?:@[\]]`i', $$part, $reg)) {

The patch (with the caret fix) is now incorporated into the Web Links module and seems to work fine.

May I suggest some way of taking a boolean also for the second param so that it doesn't change the current API?

@NancyDru: RFC1738 tells us that:

Thus, only alphanumerics, the special characters "$-_.+!*'(),", and
reserved characters used for their reserved purposes may be used
unencoded within a URL.

The caret symbol is not one of them and shouldn't appear in an URL unencoded.

The example I gave above is from an actual issue in the Web Links queue. It is created from a large and active web site. Do you wish to inform them that their web site is invalid?

BTW, valid_url, in its present form, also fails when then caret is encoded.

So, I went back and removed the caret that I added and tested, and this patch appears to have a debugging lone left in, because it showed above the heading a var_dump type line: array(1) { [0]=> string(1) "^" }

When I add a drupal_urlencode() before valid_url, I get

http%3A/%252Fthoth.lib.ucalgary.ca/uhtbin/cgisirsi/0/X/0/57/5/3%3Fsearchdata1%3D%5EC2671933%2526searchfield1%3DGENERAL%5ESUBJECT%5EGENERAL%5E%5E%2526user_id%3DWEBSERVER

which still fails to validate.

Just to let you know that function valid_url($url, array $options = array()) { causes a WSOD in PHP 5.2.6.

For those who see the bug side of this, you may want to watch: http://drupal.org/node/124492 . mfer has come up with a fix that is getting worked through the system towards D7, but also with an eye to back-port to D5 and D6.

For here, other urls to test and extend for: tel:// (for phone numbers) and xmpp:// (for jabber IM)
These may be useful in a social networking scenario, among others. Also, moving towards the mobile browsers that are also phones, these could become more desired.

The XMPP URI stuff is something I had a very small part in a long time ago, but you can find more information here: http://wiki.jabber.org/index.php/XMPP_URIs and the RFC here: http://tools.ietf.org/html/rfc4622.

The tel URI is explained in http://www.ietf.org/rfc/rfc3966.txt and http://www.ietf.org/rfc/rfc2806.txt

Here are some examples of URL's to test against provided by http://msdn.microsoft.com/en-us/library/ms709071(VS.85).aspx and http://xmpp.org/extensions/xep-0147.html.

callto:192.168.103.77+type=ipcallto:someone@example.com+type=directory
callto:msils/someone@example.com+type=directory
callto:msils:1002/someone@example.com+type=directory
callto:12345+type=phone
callto:12345+gateway=fusion+type=phone
callto:someone@example.com
callto:12345+type=phone

xmpp:romeo@montague.net?message
xmpp:romeo@montague.net?message;subject=Test%20Message;body=Here%27s%20a%20test%20message

I propose a 4 part proposal for moving forward with better validation. Instead of expanding valid_url to do everything we add some additional validation functions.

• We expand valid_url for additional schemes. This would allow us to have urls that start with ical:// and others.
• We expand valid_url for to allow international characters and international domains. Then we could have urls like http://例え.テスト/メインページ. These really aren't urls so much as they are irls. The url provided here works and is routable.
• Provide a more generic IRI validation function that can handle the cases in #10.
• Make field validation configurable. So, you can choose which validation function to use. (This may already be happening as part of the fields in core effort).

The proposed spec for IRIs is at http://tools.ietf.org/html/rfc3987. Thoughts?

I like the sound of mfer's solution. Configurability means flexibility without compromise to security. So would we provide for custom validation of the URL, meaning the user puts in their own regex? I'm thinking something like the date/time config that allows custom time config.

My guts says "really bad idea", but might as well toss it on the table so that can get slaughtered logically instead of just by my gut reaction. :-)

@flickerfly for url/uri validation I don't think we should allow the users to input their own regex. As part of the fields in core effort there are validators for fields. Developers can assign those. I want to see less configuration options. Maybe an option to set the allow schemes that has now web interface.

Only to notify you all: #368472: valid_url() marks correct IDN domains as invalid

About extending the function like discussed here, I'd like to please you to make sure module developers are able to define what protocols are valid via params. For e.g. I'd like to reuse the function in my modules, but make sure only http/https is allowed. I need this because I' like to validate the URL prior to using it much later in drupal_http_request().

Only to notify you all: #389278: Create IDN encoding and decoding functions

Subscribe

Let's start with a re-roll of this patch against D8. Any takers?

In Drupal 8 we should probably be using filter_var($url, FILTER_VALIDATE_URL) as a more robust solution.

actually, we can do that in 7 too, is there a reason we aren't? Anyone know some history here?

In Drupal 8 we should probably be using filter_var($url, FILTER_VALIDATE_URL) as a more robust solution.

we would still need to check the protocol, and use idn_to_ascii() to handle IDN domain names

http://mathiasbynens.be/demo/url-regex
this seems like a good list of urls to test against

@alexanderpas FILTER_VALIDATE_URL isn't a real validation option. It passes the work to parse_url which notes it can't be used for validation. URLs that are invalid pass it. If you look at the implementation it doesn't work.

Also, idn_to_ascii() is part of an add on for PHP and not part of PHP proper. Using that would create a dependency on functionality not there is many environments. Is the idea to add another dependency to Drupal?

@Pasqualle I had no idea about that page but it's great. Y'all should consider implementing the one from diegoperini if it passes all the tests.

I would love to see idn support and there are some other tickets for that floating around.

idn_to_ascii() is merely an implementation of the punycode. Urls could be converted to their punycode equivalents to check for validity.

More info here:
https://drupal.org/node/389278

Agreed, we have the same problem with filter_var(). I would suspect we need to look into Symfony's UrlValidator class as it uses a regex approach, and may work better than our current standard. Or should be improved via pull request to add better support. I'd rather us re-use an existing code like that rather than implement something custom.

Core uses a (modified since this issue was opened) regular expression, not filter_var(), so the issue title here is wrong.

The two bugs in the issue summary were fixed in the meantime, I think we need an updated issue summary with which valid URLs are not allowed by UrlHelper::isValid() and which valid URLs are rejected. These could then be moved into test coverage and we could go from there. The idea in #24 seems fine but is an API addition rather than a bugfix.