Fix Drupal 6 url() function to call custom_url_rewrite_outbound for external links too

I'm sure all of this brings good news to Drupal Heaven, where the gods are using Drupal 7. But for the rest of us who need this functionality in Drupal 6, is there a patch in the works?

Thanks Dave.

I may be preaching to the choir, but this API change is probably a small one. That is, 3 lines added to common.inc starting at line 1408 to make a call to custom_url_rewrite_outbound():

<?php
  // common.inc line 1408
  if (function_exists('custom_url_rewrite_outbound')) {
    // Modules may alter outbound links by reference.
    custom_url_rewrite_outbound($path, $options, $original_path);
  }
?>

Quite frankly, these are the exact same 3 lines from the same url() function that, for some reason, does not my module to rewrite URLs for external links as is with internal links... unless I hack or patch Drupal.

If the lords at Drupal can commit this change, many libraries would be able to easily rewrite links from their metadata records pointing to resources held at external databases, such as ProQuest and LexisNexis, routing these links through an EZProxy or other proxy referral server.

As noted in #2, this is fixed in Drupal 7, but as noted in #3, Drupal 6 will remain supported (and therefore, one would imagine, used) until Drupal 8 is released, which may be 2 years or so from now. However, this isn't a critical bug, and an API change (even a small one) to fix a non-critical bug introduces more harm than good by potentially breaking compatibility with released modules. What if some module out there implements custom_url_rewrite_outbound() and expects it to only ever be called with internal paths? Someone using that module would be justifiably angry if their site breaks during an upgrade from Drupal 6.14 to Drupal 6.15. Introducing a new function, custom_url_rewrite_outbound_external() would probably be safer, but still not safe enough, because we shouldn't assume that a module doesn't implement that function for some other purpose. The problem is we have no reserved namespace in Drupal in which we can safely add functions between minor versions (unless we can justify "drupal_" as that name space, in which case adding a drupal_custom_url_rewrite_outbound_external() might be considered safe, but ugly).

This is a long-shot, but here's a proposed patch. It introduces a new configuration variable in settings.php that can be used to allow non-critical API improvements between minor Drupal versions. These should always ship disabled, so an upgrade remains safe. But, it allows for important improvements to be rolled out in a controlled way. And IMO, it would be better for module authors like mtwesley to ship an install.txt file that says to enable that change within settings.php rather than ship an install.txt file that says to apply a patch that hacks core.

As I said, it's a long-shot, because it changes a long-established Drupal convention of no non-critical API changes during minor upgrades. But I'm curious what the community thinks of this. If not for D6, maybe we could get this convention into D7, so that we don't have the same situation there of a 4-year life span with no room to fix things that people want fixed.

Seems like a great compromise to me. +1.

I like custom_url_rewrite_outbound_external(). There should not be any namespace conflict unless someone has a custom.module and creates a hook_external() or something similar.

This suggestion is great, but I am only interested in solving the problem at hand. Trying to make that big of a change to settings.php seems much too unrealistic, whereas I still think the fact that Drupal cannot rewrite external URLs is a significant enough bug that could be fixed in an upcoming minor release.

For now, I created a patch to deal with the problem of rewriting external URLs in Drupal 6 since I only need those three lines. I have included this with my module and implemented a way to rewrite the URLs using JavaScript, so webmasters can choose which option they prefer.

I tested this patch a few times and it works for me.

The size of a change isn't just how many lines of code are in the patch file, but what the repercussions will be. I'm willing to be overruled by community wishes on this one, but I'm against calling a custom_url_rewrite_outbound_external() function. I don't think it's safe to assume that some module or site out there doesn't implement that function as a helper function to custom_url_rewrite_outbound(), but for a different purpose and with a different signature. In my opinion, introducing a non-optional API change to an already released major Drupal version is a bigger change than introducing a new configuration variable that starts with "drupal_". Measured by level of backwards-compatibility, I can think of no smaller change than the one in #6.

Oh, and mtwesley, I fully support you including patch #8 with your module. It's up to your module's users to decide if they want to apply your patch, and if it breaks backwards compatibility, then that's strictly between them and you. I'm just against applying that patch to Drupal 6 core and releasing it with the next minor Drupal version, because that would affect all Drupal users. My attempt in #6 was to create something that has a greater chance of being committed to core, and would allow you to create a module that does not require shipping a patch file along with it.

OK. No reason to argue.

Thanks. Re-uploading #6 to make it clear that's the currently suggested solution. We have 3 people for (me, mtwesley, and moshe) and none against. @Dave Reid: what do you think?

Ok, I'm trying to get more core developer eyes on this. I'd like to use this comment, however, to change the use-case from what's posted at the top of this issue to the one that was sent by mtwesley in an email to the development list. The following is the use-case from that email, with some minor fluff added:

1. Different modules let a content editor or administrator add external links to the Drupal site. For example, the menu module lets you add menu items with external links as paths. The link module lets you add links as CCK fields. There's probably a module somewhere in contrib that implements a filter that routes anchor tags entered into textareas through the l() function. There's a bunch of other modules that let you enter lists of urls from which links are rendered by calling l().
2. Library websites often use these modules to display various "resources" that someone can visit. As an example, consider a "resource" / external url such as http://www.books24x7.com/marc.asp?bookid=30164.
3. Access to these URLs is controlled by the corresponding vendor. From within the library (within a certain IP address range), directly navigating to the URL will work. Outside that range, access to that external url is restricted and needs to be proxied for authentication. For example, for these users, the website wants to rewrite the link to point to http://ezproxy.example-library.edu/login?url=http://www.books24x7.com/ma... instead.

Without custom_url_rewrite_outbound() (or something) being invoked by url(), the only other way to implement the above functionality is to try to intercept on a theme function by theme function basis. For example, one could try intercepting theme_menu_item_link(), the theme function for each formatter provided by link.module, create a custom filter for links inside of textareas, and so on and so on for every possible place that can call l() with an external url. Or one could create a javascript-based solution, or hack core. But, all of those options suck.

This is a very common use-case for libraries and universities, and I imagine similar use-cases exist for other types of websites, and while this is all nicely fixed for Drupal 7, it would be nice to let people still using Drupal 6 to get over this hurdle.

I vote in favour of whatever core solution to have custom_url_rewrite_outbound functionality for external links.

We have a security situation that a client does not want HTML referrer information leaked through external links. We would need to rewrite all external links to go through a proxy that strips the HTML referrer. Please make this possible for D6 because it will still take a long time before all the modules we use will be available for D7.

Subscribing. The patch from #12 worked for me against 6.19.