[drupal-devel] [bug] Handle full URLs in url() and l()

adrian drupal-devel at drupal.org
Mon Jul 18 14:12:47 UTC 2005

Issue status update for 
Post a follow up: 

 Project:      Drupal
 Version:      cvs
 Component:    base system
 Category:     bug reports
 Priority:     normal
 Assigned to:  Bèr Kessels
 Reported by:  jhriggs
 Updated by:   adrian
 Status:       patch

spoke to dries about this patch, and apart from the incomplete patch I
uploaded, there are about a dozen instances where in-line html is used
were l() would suffice.

I will be cleaning them up and submitting a new patch soon.


Previous comments:

1095346884 : jhriggs

Attachment: http://drupal.org/files/issues/full_url.patch (1.97 KB)

This little patch allows full URLs to be used for the url() and l()
functions, giving modules a single interface for creating links. 
Currently, modules must use l() for drupal links and raw HTML for
external links.  This patch changes url() to handle -- basically pass
through -- full URLs while still doing the proper handling of internal
paths and works with clean URLs enabled or disabled.  It also shortens
(and hopefully simplifies) the url() code a bit.

Some examples of calls that now work:

l('a link', 'some/path');  // same as before
l('an external link', 'http://drupal.org/');
l('full with query', 'http://google.com/search?q=define:foo');
l('full with separate query', 'http://google.com/search', array(),
'q=define:foo'); // same link as above
l('full with fragment', 'http://drupal.org/node/5942#comment-12374');
l('full with separate fragment', 'http://drupal.org/node/5942',
array(), NULL, 'comment-12374'); // same link as above


1096542570 : moshe weitzman

+1. less HTML in code is a good thing for readability, if nothing else.


1106597186 : mathias

Attachment: http://drupal.org/files/issues/external_urls.patch (838 bytes)

Revisiting this issue one year later shows we're almost there. The only
thing that's left is the ability to handle external urls when
clean_urls is disabled.  Once implemented, menu.module can happily
accept external urls and not just Drupal paths, making the menu system
much more attractive to end users.

I'll update the menu.module docs if this patch (or an alternative
solution) is accepted.


1110753394 : killes at www.drop.org

Attachment: http://drupal.org/files/issues/external_urls_0.patch (909 bytes)

Updated from Drupal root directory.


1110758717 : gordon

+1 This is a good thing for people who can't using clean urls.


1112422990 : TDobes

This patch seems like a good idea, but does not apply anymore.  Also,
shouldn't the comments for l() and url() be updated to indicate that
the functions will work with both internal and external URL's?


1112426713 : asimmonds

Attachment: http://drupal.org/files/issues/external_urls_1.patch (897 bytes)

The &'s had been changed to & amp; , here's a valid patch


1115909827 : adrian

This is a  new patch against cvs that allows url() to create external
links, with the side-effect that menu can now be used to make external
Also closes http://drupal.org/node/10177

This will be even more important with the primary links menu
integration. (This patch already cleans up a special case in
phptemplate for that).

Killes benchmarked this, and it turns out the difference is negligible.
Requests per second:    1.33 [#/sec] (mean) = with patch
Requests per second:    1.31 [#/sec] (mean) = without patch.


1115909997 : killes at www.drop.org

I had benchmarked /node as anon user without cache.


1116996608 : Steven

+1 on the idea of this patch. It allows people to use l() for external

However, I don't like the implementation. Running that complicated
regexp for every url() call is simply wasteful. 95% or more of url()
calls have a fixed string as parameter and point to Drupal's own paths.

I suggest we create an url_external() wrapper around url() which allows
external URLs. Any path which we want to allow external URLs in can use
url_external() instead. The places where we use it will not be many


1116997555 : gordon

splitting into url() url_external is not a good idea. The main reason
for this patch that I see is for putting external urls into the
navigation menu using the menu.module, and if all goes to plan, the
primary and secondary menus.


1117000801 : Steven

gordon: I still think this is a bit excessive. Perhaps instead of
valid_url() we could use a simpler regexp !^[a-z]://!i or even


1117001736 : gordon

I am not says that it is or is not excessive. I was just saying that
this is where it is going to be used, so creating url() and
url_external() is not really an option


1117004273 : Bèr Kessels

we /do/ need an external_url() thing.

The links bundle  (handling weblinks and the likes) willprovide this
/and/ need this.
People will get all sorts of tools to do /stuff/ with external links,
like add permissions, track clicks etc. depending on the modules they
have and the settings. All outgoing weblinks, should somehow respect
those. Patches for core are coming, though.

On top of that, a external_l() finction is simple. And if people want
them in primary links they can do the regexping /before/ the function,
not /in/ the function. The calling code can decide whether to call l()
or external_l(), instead of putting logic in l() that is unneded 95% of
the times.

But i hereby promise to introduce such a function tbefore the end of
this week :)


1117005467 : gordon

The only thing I do not see in this plan is how with this work with the
menu module and the navigation menu.


1117015452 : adrian

Attachment: http://drupal.org/files/issues/url_external_links.patch (1.75 KB)

What happened to the patch i uploaded ?

It doesn't use a complex regexp, but uses strpos($url, '://') < 5.


1117088372 : Bèr Kessels

Allthough this works, is clean AND does what it should do, I still am in
favour of a better nitegrated solution. So please do not yet commit
this, untill I upload my patch. We can then discuss what solution to
take. It will not differ a big lot though.


1117096772 : adrian

is this the patch where you want to introduce an el() to allow for
counting of external links ?

That's a different patch altogether. This is primarily about stopping
url() from not handling external links when there is no good reason for
it to do so. Your el() has a very specific use in the weblinks api, and
should probably be included with it.


1117124311 : Bèr Kessels

Yes, that el()

But let me explain why i am anxious for my solution: el(), by default
will not do much, in fact it will just be an URL()-clone but one that
allows outgoing links. But it has a hook-caller. Again: by default it
does nothing much, for there will not be any hooks around in core. 

It will allow, for example, weblinks, to track clicks, or to look up a
permission and allow or disallow users to follow a link.

Now, the whole issue, IMO is, that we should *not* provide two ways for
handling external URLS, but rather a single one, that is weblink-safe.
If we would introduce URL() with outgoing links-capabilities, we can no
longer track any outgoing URLs trough them. We will then have two ways
for handling outgoing URLs, resulting in inconsistencies, and confused
users. (hey, my anonymous users can follow link foo, while i have
"follow outgig links" permission disabled for them).

(BTW weblink will soon be renamed into links, just for reference)


1120129571 : adrian

Attachment: http://drupal.org/files/issues/url_external_links_0.patch (824 bytes)

I've updated the patch.

More information about the drupal-devel mailing list