External URL support for menus

Perhaps I'm missing the point... or perhaps this has been fixed elsewhere since this issue was posted. But when I enter "http://drupal.org" as the path for a menu item it links to the Drupal site as expected.

I'm closing this issue... feel free to re-open it if I've misunderstood what this patch does.

You're right. This doesn't work properly.

Can you confirm that the attached patch fixes your problem?

Rather than create a new function for linking to an external URL, this patch makes url() automatically handle external URLs correctly.

Cheers, that approach is the right one - fix the problem at the source.

I had patched my own code with something simliar a while ago, when I started getting those silly q=http results on a test server.
+1 vote in theory

xss_bad_protocol is a new one on me, good catch.

( Heh, I wrote a little review criticizing the use of // if $path contains ':'.
but then decided it was good enough - a random path containing ':' deserves to be misunderstood - my experiences were based on turning random strings into URIs)

All good from code review> but not test.

.dan.

A valid URL won't contain a colon anywhere except at the end of the scheme.

If you want to pass a URL as an argument you need to escape the colon.
For example:
http://www.example.com/?url=http://www.example.com/
is invalid. It should be encoded as:
http://www.example.com/?url=http%3a//www.example.com/

What happens if i want to add a link to an fpt site using a URI such as ftp://user:pass@ftp.whatever.com/

This has two colons in it.

What happens? ftp is allowed protocol and there filter_xss_bad_protocol finishes happily. filter_xss_bad_protocol filters bad protocols and stops when a good one is found.

Rcihard, budda there is no need to escape the second colon.

The filter_xss_bad_protocol function sees the allowed scheme at the start of the URL and allows the string through unmangled.

This is also the case with URLs like http://www.example.com:8000/ ... they work fine.

@chx: Yes, I realise now that filter_xss_bad_protocol is smarter than I first gave it credit for. And now it's faster too so my misunderstanding isn't all bad :)

Richard, do you have any specific reason why you cleared the ready to commit status I set?

Gerhard's tip is possible correct -- we just crossed comments ie. Richard started his earlier than mine.

Yes, crossed comments. Sorry, I should have noticed the change of status when I followed up.

Rerolled.

This is essential functionality to allow Primary Links to work as expected... because if someone had an external URL in their 4.6 installation they need this patch for those links to work in 4.7.

+1. Now that http://drupal.org/node/41744 has been comitted, this bug affects sites with clean URLs too.

Now that that other patch has been committed, this is a huUUge +1 for me too. I *relied* on the previous bug to Do What I Mean (ie., support full URLs), and yes, that's bad, but I'd still like DWIM. And that means this patch is required.

I've been running this patch live on disobey.com and it's working fine for me.

rerolled

A bit better doxygen.

Passthrough for external URLs.

Google says:
Results 1 - 10 of about 16,900,000 for "a url".
Results 1 - 10 of about 2,330,000 for "an url".

In this basis I used "a URL" in the first line of the doxygen, so I've made them consistent.

If other parameters are ignored for URLs this block of code can move up for a very modest performance improvement.

This patch is still a bit weird, because url() is a function to generate an URL and do all the related tasks about it. Generating from an existing URL is a bit weird, but an acceptable border-case I suppose.

What bothers me is that the behaviour for pass-through is half-baked. It will add a query string from $query, but assumes there is no existing query. And this will only work if the original URL did not have a #fragment. Similarly, adding a fragment only works if there is no existing fragment.

If we're going to add passthrough URL support to url(), lets do it properly. Me and chx changed the code so it properly appends the query string with & rather than ? if there already was one, does not disturb existing #fragments, and replaces an existing #fragment if there was already one.

Note that all of this code is triggered by strpos(':'), so it does not affect the calls to url() where a normal Drupal path is passed. Also, the extra code is based around strpos(#) and isset(), so it's not that heavy either. It makes url() truly intuitive to use, which I suppose is what people want.

For completeness: what happens if a dynamic menu path (e.g. search/node/$keywords) contains a ':'? It will be seen as an external URL, but it will not pass the 'valid protocol' check and will still be considered a regular menu path. We can assume that each dynamic path has at least one non-dynamic component at the start.

Committed to HEAD in modified form.

This should have been benchmarked! Please benchmark or I will roll back this change.

Benchmarking.

I am not able to show anything. I am running ab and the uncertainity is by far above a few dozen strpos calls on a ~20 char string...

Um, all of this is triggered by a single strpos call on a short string, looking for a single character. Aside from this test, nothing changes for the vast, vast, vast majority of url() calls. Is benchmarking this really necessary?

I benchmarked this as part of my testing for the <front> feature.

With the other optimisations I've made, adding these two features (Front and absolute URLs) is performance-neutral.

Anyhow, being able to place absolute URLs in menus is absolutely critical functionality. Even if there was a performance hit, this feature has to be implemented.

I'm assuming silence is golden.