Handle full URLs in url() and l()

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

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.

Updated from Drupal root directory.

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

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?

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

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 links.
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.

I had benchmarked /node as anon user without cache.

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

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 anyway.

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.

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 strpos('://').

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

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 :)

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

What happened to the patch i uploaded ?

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

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.

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.

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)

I've updated the 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.

hopefully adrian or someone else will chase down the remaining urls which should use this function.

I've come to this party late but would like to see a patch soon... I've used the patch from Adrian (#15) but this did not work correctly in my setup (IIS 5.0 / no clean urls) I applied it against 4.6.2 and cvs.

Where is the latest patch that I can test ... I'll throw some time at this because I need it for a module used within our Intranet.

I'm not sure what the final plans are for this in the CVS so I have developed a patch for use on 4.6.2

I decided to add a parameter named $external that is passed to url() function. The default is FALSE so this should not break any existing calls to either l() or url(). I have also refactored url() so that it is much easier to understand and extend --- I'm thinking of Bèr Kessels intention to

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

Since the parameter is passed to activate the external url there is no need for regex or strpos, this should be handled by the calling function and then set the $external parameter to TRUE

For example I have patched the theme_menu_item_link() function in menu.inc to accept external urls:

function theme_menu_item_link($item, $link_item) {
  // this next line was added so that a menu item could point to the default home
  $link_item['path'] = preg_replace('/^\/$/','',$link_item['path']);
  // this next line was added so that a menu item could point to an external url
  $external = (preg_match('<^[a-z]+://>', $link_item['path'])) ? TRUE : FALSE ;
  return l($item['title'], $link_item['path'], array_key_exists('description', $item) ? array('title' => $items['description']) : array(),NULL,NULL,FALSE,FALSE,$external);
}

I hope that this patch will spur some solutions - I'm using this now in production but have not tested extensively across every scenario.

thanks
jim sloan

... are there any thoughts on this? What is the status of the final patch?

The problem with this patch is that we still need to fix all the inline occurrences of the <a href tag.

I don't think we should confuse the issue and implement a hook at this point, as url() breaking with external links is a bug, pure and simple.

Adrian, your second patch for theme.inc looks wrong to me. Don't you still need the line
$settings[$type .'_links'][] = l($text, $link, $attributes); ?

I also got rid of unset($attributes); Where was that supposed to come from that it needed unsetting?

Here is a unified patch that I updated to fix a bug in Adrian's common.inc code (everything was being handled as absolute), plus the update to theme.inc that I questioned in the previous post.

In my initial testing, this works perfectly. I can put absolute URLs in for the paths in menu items and it handles them fine, and changes no other behavior. I searched core for all uses of url() and http:// and found no more violating code such as that in theme.inc.

Net change in core with this patch: 0 lines of code.

Please review quickly, I'm writing in the upcoming book about this functionality as if it is in. =)

Correction: net change to core -1 line of code.

What needs to be removed from core are not uses of url() and l(), but all occurences of the anchor tag.

The anchor tag was mostly used because l() didn't do external links.
Also, in the case of phptemplate , that is the line that creates the array that is passed to the template.

Good - that needs to be removed too, but is there anything that doesn't work right if we don't remove them? Lots of the $output = '<a href....' instances in the code use url() for href and these are not broken. They can't be seen as a reason not to commit this particular patch. If anyone point out things that break with the patch I made, I'll fix them. I can't go through and clean up anchor tags on idealistic grounds at the moment (as much as I would otherwise like to).

I think that using strpos($path, '://') to trigger the external url is a bad idea.

My patch implements a new parameter to url() and l() that will let the calling function decided if it is an external url. And it is backward compatible with all existing calls.

I also rewrote the logic of builing the url to make it simple.

Adrian, I do not agree that it was a bug; url() never intended to handle external url´s and also, the clean up of the inline anchor tags are not dependent on this feature.

Can this be considered for the solution to this feature?

... the patch I am referring to is above at # 23

I think that using strpos($path, '://') to trigger the external url is a bad idea.

Please explain why. The only thing that your regexp does that is different is check to see if the part of the string before the :// is within the range a-z, and for that you use a more expensive PHP function. Please indicate the cases where strpos($path, '://') won't work correctly.

My patch implements a new parameter to url() and l() that will let the calling function decided if it is an external url. And it is backward compatible with all existing calls.

This means that instead of doing one check in a centralized place, programmers now have to write checks anytime they want to call this function with an either/or case. I don't see why this is better.

I also rewrote the logic of builing the url to make it simple.

This is good.

Adrian, I do not agree that it was a bug; url() never intended to handle external url´s and also, the clean up of the inline anchor tags are not dependent on this feature.

I'm still unclear on why anything else in the code base needs to be changed before this functionality can be accepted. I stick with my patch as the preferred solution.

Please explain why

My post #23 has confused you. The patch itself does not add a strpos() or regexp() ... the code snippet I posted is an example of the calling function´s responsibility of determining the use of the external link and then passing that parameter on to the l() or url() function.

I believe it is a bad idea to use string parsing at this low level to determine functionality because it will impose this behavior on all calls. There could be a case in which this would be a problem; such as a url that could contain another url as a parameter.
index.php?q=module&param=http://example.com

... and there are some who would balk at the processing overhead of pattern matching at his level.

This means that instead of doing one check in a centralized place, programmers now have to write checks anytime they want to call this function with an either/or case. I don't see why this is better.

This is better because the module author is the proper person to determine whether the link is external. Note that in the case of all internal links there is no change of coding practice at all. It is when the module function must handle external links that module´s author can include the various context and string pattern tests to set the $external parameter for the l() or url() call. This is what I was demonstrating in the code example of post #23.

I'm still unclear on why anything else in the code base needs to be changed before this functionality can be accepted. I stick with my patch as the preferred solution.

I think that we agree on this point. Nothing needs to be changed in the code base – my patch is backward compatible to all existing calls and the clean up work on the in-line anchor tags can (and should) take place regardless of this patch.

Thanks for the clarification.

1) does your patch still apply well?
2) could you please add the update to the theme menu item to the main patch so that menu items can be external?

I had forgotten about urls containing urls. That's clearly a showstopper.

-Robert

You might update the bit in theme.inc as well and include that in the patch.

Here is the patch for CVS;

It contains a rewrite of the url() function and the $external parameter is added to the l() function. I have tested on Apache2/Linux and IIS5/WIndws 2000 and with clean url's on Apache2/Linux.

The patch fails on version 4.6.3 so that will be posted seperate.

A little later I will submit a patch for menu.inc to enable external url's.

Here is the patch for 4.6.3;

It contains a rewrite of the url() function and the $external parameter is added to the l() function. I have tested on Apache2/Linux and IIS5/WIndws 2000 and with clean url's on Apache2/Linux.

A little later I will submit a patch for menu.inc to enable external url's.

A patch for menu.inc that enables external urls.

You MUST apply jsloan's patch, in comment #38, for this patch to work.

+1 awesome work guys, i hope this gets committed soon!

I just had to revert to the distributed includes/common.inc. jsloan's patch killed path aliases by not generating the alias for links.

-1 on jsloans patch.

I don't want to have to require the calling function to do anything to be able to alias to an external url.

That's my one requirement for this.

Another example that came up was being able to redirect the user/login page to https://mysite.com/user/login , to do https auth.

That would mean finding and editing all the places anyone links to user/login (in core.. keep in mind we want to avoid editing core whenever possible), to be able to do something like that.

This day of all days, finally this problem is put to rest.