[drupal-devel] [bug] Handle full URLs in url() and l()
robertDouglass
drupal-devel at drupal.org
Sun Sep 11 07:27:18 UTC 2005
Issue status update for
http://drupal.org/node/10888
Post a follow up:
http://drupal.org/project/comments/add/10888
Project: Drupal
Version: cvs
Component: base system
Category: bug reports
Priority: normal
Assigned to: Bèr Kessels
Reported by: jhriggs
Updated by: robertDouglass
Status: patch (code needs work)
"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.
robertDouglass
Previous comments:
------------------------------------------------------------------------
Thu, 16 Sep 2004 15:01:24 +0000 : 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:
<?php
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
?>
------------------------------------------------------------------------
Thu, 30 Sep 2004 11:09:30 +0000 : moshe weitzman
+1. less HTML in code is a good thing for readability, if nothing else.
------------------------------------------------------------------------
Mon, 24 Jan 2005 20:06:26 +0000 : 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.
------------------------------------------------------------------------
Sun, 13 Mar 2005 22:36:34 +0000 : killes at www.drop.org
Attachment: http://drupal.org/files/issues/external_urls_0.patch (909 bytes)
Updated from Drupal root directory.
------------------------------------------------------------------------
Mon, 14 Mar 2005 00:05:17 +0000 : gordon
+1 This is a good thing for people who can't using clean urls.
------------------------------------------------------------------------
Sat, 02 Apr 2005 06:23:10 +0000 : 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?
------------------------------------------------------------------------
Sat, 02 Apr 2005 07:25:13 +0000 : 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
------------------------------------------------------------------------
Thu, 12 May 2005 14:57:07 +0000 : 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
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.
------------------------------------------------------------------------
Thu, 12 May 2005 14:59:57 +0000 : killes at www.drop.org
I had benchmarked /node as anon user without cache.
------------------------------------------------------------------------
Wed, 25 May 2005 04:50:08 +0000 : Steven
+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.
------------------------------------------------------------------------
Wed, 25 May 2005 05:05:55 +0000 : 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.
------------------------------------------------------------------------
Wed, 25 May 2005 06:00:01 +0000 : 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
strpos('://').
------------------------------------------------------------------------
Wed, 25 May 2005 06:15:36 +0000 : 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
------------------------------------------------------------------------
Wed, 25 May 2005 06:57:53 +0000 : 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 :)
------------------------------------------------------------------------
Wed, 25 May 2005 07:17:47 +0000 : gordon
The only thing I do not see in this plan is how with this work with the
menu module and the navigation menu.
------------------------------------------------------------------------
Wed, 25 May 2005 10:04:12 +0000 : 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.
------------------------------------------------------------------------
Thu, 26 May 2005 06:19:32 +0000 : 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.
------------------------------------------------------------------------
Thu, 26 May 2005 08:39:32 +0000 : 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.
------------------------------------------------------------------------
Thu, 26 May 2005 16:18:31 +0000 : 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)
------------------------------------------------------------------------
Thu, 30 Jun 2005 11:06:11 +0000 : adrian
Attachment: http://drupal.org/files/issues/url_external_links_0.patch (824 bytes)
I've updated the patch.
------------------------------------------------------------------------
Mon, 18 Jul 2005 14:12:36 +0000 : adrian
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.
------------------------------------------------------------------------
Fri, 05 Aug 2005 18:44:03 +0000 : moshe weitzman
hopefully adrian or someone else will chase down the remaining urls
which should use this function.
------------------------------------------------------------------------
Fri, 05 Aug 2005 19:25:08 +0000 : jsloan
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.
------------------------------------------------------------------------
Mon, 08 Aug 2005 20:06:03 +0000 : jsloan
Attachment: http://drupal.org/files/issues/external_urls_2.patch (3.67 KB)
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:
<?php
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
------------------------------------------------------------------------
Fri, 19 Aug 2005 12:10:50 +0000 : jsloan
... are there any thoughts on this? What is the status of the final
patch?
------------------------------------------------------------------------
Sun, 28 Aug 2005 15:48:08 +0000 : adrian
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.
------------------------------------------------------------------------
Thu, 08 Sep 2005 15:24:16 +0000 : robertDouglass
Attachment: http://drupal.org/files/issues/theme.inc.patch.txt (1.16 KB)
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?
------------------------------------------------------------------------
Thu, 08 Sep 2005 15:48:02 +0000 : robertDouglass
Attachment: http://drupal.org/files/issues/theme.common.inc.patch.txt (1.83 KB)
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. =)
------------------------------------------------------------------------
Thu, 08 Sep 2005 15:49:43 +0000 : robertDouglass
Correction: net change to core -1 line of code.
------------------------------------------------------------------------
Thu, 08 Sep 2005 18:58:01 +0000 : adrian
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.
------------------------------------------------------------------------
Thu, 08 Sep 2005 20:58:13 +0000 : robertDouglass
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).
------------------------------------------------------------------------
Sat, 10 Sep 2005 20:11:08 +0000 : jsloan
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?
------------------------------------------------------------------------
Sun, 11 Sep 2005 02:14:10 +0000 : jsloan
... the patch I am referring to is above at # 23
More information about the drupal-devel
mailing list