Instead of only allowing one hard-coded function to rewrite outbound links, I propose we introduce a new hook (hook_url_alter) so that modules can alter the URLs processed by url() and l(). I had the need for this feature in one of my D6 modules where I wanted to rewrite the links to internal Drupal RSS feeds to a FeedBurner feed. As a part of this, I'll probably need to write a test or two, as well as do some performance testing. This could probably help move language_url_rewrite() from language.inc into locale.module.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 359965-hook-url-alter-D7.patch | 7 KB | dave reid |
| #3 | 359965-hook-url-alter-D7.patch | 8.19 KB | dave reid |
| #1 | 359965-hook-url-alter-D7.patch | 6.91 KB | dave reid |
Comments
Comment #1
dave reidInitial patch for review.
Comment #2
dave reidRevised patch that actually works. Here's the benchmarks running
ab -c 5 -n 1000 http://mysql.drupalhead.localwith 50 nodes, 5 comments per node, and the recent blog posts block enabled:About a +/- 1 ms difference with the patch. Maximum benefit is with the patch and locale module enabled.
Comment #3
dave reidRevised patch used in #2.
Comment #4
dave reidOk, another attempt to post the patch actually used in #2.
Comment #5
dave reidComment #6
dave reidComment #7
agentrickardDave, I had this issue in a module as well, and had a similar patch at one time. However, my understanding is that we need the hardcoded functions due to situations where calls to paths or translations are made early in the bootstrap process, before all modules are loaded.
Does the new code registry mean that hook_url_alter() implementations are always present when the hook is fired? I am trying to think of an example case; hopefully someone else will recall why custom_url_rewrite_*() originally had to be loaded in setttings.php.
Comment #8
dave reidWell, after my initial investigations, the url() and l() functions are in common.inc, which is only included on a full bootstrap. Therefore, with the registry loader in module_invoke_all(), any hook_url_alter() implementations will be available as well. They would be available no matter the bootstrap setting anyway thanks to the registry.
Comment #9
lilou commentedComment #10
dave reidComment #11
Dave Cohen commentedI would like to see the call to custom_url_rewrite_outbound() remain unchanged. That is, leave these lines intact:
My feeling is that if custom_url_rewrite_inbound() is still necessary (and it is!), and quite often the two custom_url_rewrite functions have essentially the opposite effect. So there's an easy to understand symmetry and logic to having the two functions as a pair.
What you're suggesting instead is to have custom_url_rewrite_inbound() defined in my settings.php file, and have a corresponding hook_url_alter found in some module somewhere else and using a different technique to change paths. That sounds harder to maintain to me, and potentially requires a whole module where previously just a function would suffice.
I am +1 in favor of introducing hook_url_alter. And I'm -1 opposed to getting rid of custom_url_rewrite_outbound.
Comment #12
dave reidWell...I can't claim this original idea anymore. Marking as a duplicate of #320331: Turn custom_url_rewrite_inbound and custom_url_rewrite_outbound into hooks.
Comment #13
dave reid@Dave Cohen - The current implementation is a hack. If we have a chance to change things and do them the "Drupal way" we should. It will improve performance and make things easier for more than one module/etc to alter the outbound urls. It doesn't take much effort to write one small little module that would implement hook_url_alter().