The comment in url_is_external() is wrong:

function url_is_external($path) {
  $colonpos = strpos($path, ':');
  // Only call the slow drupal_strip_dangerous_protocols() if $path contains a
  // ':' before any / ? or #.
  return $colonpos !== FALSE && !preg_match('![/?#]!', substr($path, 0, $colonpos)) && drupal_strip_dangerous_protocols($path) == $path;
}

In fact, we only call drupal_strip_dangerous_protocols() if $path does not contain a / ? or # before the first ':'.

A good explanation is in drupal_strip_dangerous_protocols()

      // If a colon is preceded by a slash, question mark or hash, it cannot
      // possibly be part of the URL scheme. This must be a relative URL, which
      // inherits the (safe) protocol of the base document.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bfroehle’s picture

Priority: Normal » Minor

Setting the priority to minor, as it's just a bug in the comments.

franz’s picture

Status: Active » Needs review
FileSize
756 bytes

I'm giving a shot at it.

christefano’s picture

Status: Needs review » Reviewed & tested by the community

This is ready to go. Thanks, franz!

This review was done at DrupalCamp LA 2011 during a live session. You're famous!

franz’s picture

cool. =)

klausi’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/includes/common.inc
@@ -2197,8 +2197,9 @@ function url($path = NULL, array $options = array()) {
+  // Avoid calling drupal_strip_dangerous_protocols() if there is any ¶

trailing white space

20 days to next Drupal core point release.

franz’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
755 bytes

Status: Reviewed & tested by the community » Needs work
Issue tags: -Documentation, -Novice

The last submitted patch, 1113702-url_is_external_comment.patch, failed testing.

franz’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1113702-url_is_external_comment.patch, failed testing.

franz’s picture

Status: Needs work » Needs review
Issue tags: +Documentation, +Novice
marcingy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

White space issue has been fixed.

Dries’s picture

Status: Reviewed & tested by the community » Needs review

Shouldn't it be 'a URL' instead of 'an URL'?

franz’s picture

It seems to depend on how you pronnounce it: http://grammar.ccc.commnet.edu/grammar/abbreviations.htm (Using articles with abbreviations and acronyms) and http://grammar.ccc.commnet.edu/grammar/grammarlogs1/grammarlogs254.htm (Ctrl + F to searh for "an URL")

In my case I should've used "a URL" but I don't know about the standards adopted - if any - for this.

franz’s picture

However, my sources could be outdated, I checked a free online dictionary and pronunciation is like "yoo-arr-ell" (and not something ugly as "earl") so it should be "a URL".

franz’s picture

Status: Needs review » Reviewed & tested by the community

Simple change, doesn't need testing again.

catch’s picture

Version: 8.x-dev » 7.x-dev

Committed to 8.x, back to 7.x for webchick.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Documentation, -Novice, -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.