It would be nice if l() could take into account some way of themeing links, presumably by taking into account the existence of a theme_link() implementation.

Application case: create site-global specific CSS depending on some property of the URL, without having to handle it at the module or theme level by passing additional parameters in the attributes parameter as is currently the case. This would allow, for instance, all links back to d.o. on a site to automagically inherit a druplicon before the link by default, or links to wikipedia, whichever module would be creating them, like it is done in various wikis for inter-wiki links.

Of course, this could take advantage of the new hook registration system introduced in D6. The problem would typically be efficiency, since l() tends to be heavily used. I expect that, for this specific case, precomputing the themeability on the first call and keeping it in a static within l() to avoid multiple checks would beat the use of theme('link', <computed link>);.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

This is a good idea! ++

pixelite’s picture

Makes sense to me.

joachim’s picture

+1
I have a designer who wants a SPAN inside every link to do funky things with it.

effulgentsia’s picture

Status: Active » Needs review
FileSize
1.35 KB

Still needs benchmarks and PHPDoc, but I want to know what testbot thinks.

Status: Needs review » Needs work

The last submitted patch failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.87 KB

With initial PHPDoc (probably needs refinement) and corrected test failure.

effulgentsia’s picture

Issue tags: +API clean-up

Tagging API cleanup, because everywhere HTML is generated, it should be done via theme(). Performance is critical though, so benchmarks, and if needed, optimization, coming soon.

effulgentsia’s picture

This one's as optimized as I could make it (or rather, I'm giving up on trying for better). For anyone wanting to play with it, I'm attaching a micro-benchmarking script. Rename benchmark_196908.txt to benchmark_196908.php, put it in your drupal root, and navigate to it. It will output the time it takes to call l() in microseconds (averaged over many iterations).

Here's the summary: it adds 10% overhead to calling l() (on my computer, from 69 microseconds to 75 microseconds).

But, l() typically comprises about 10% of the page request time, even for link intensive pages. For example, on my computer, going to /admin/content when there are more than 50 nodes in the database (and therefore, 50 being displayed) takes 100ms. Of that, 10% of that time is spent within l() in order to generate the 150 links (the node title, the edit link, and the author link). Given the 10% extra overhead this patch adds to l(), the total page request with this patch moves to 101ms, or an additional 1%. Similarly, a node with 300 comments takes about 700ms on my computer, and with this patch, 707ms (an additional 1%).

So, is it worth adding 10% to l(), and therefore 1% to total page request time for link-heavy pages in order to follow the well established Drupal rule that all HTML should be generated with a theme function, and therefore, support override-ability by modules and themes? My vote is yes.

effulgentsia’s picture

Issue tags: +Performance

tagging with "performance" for obvious reasons.

catch’s picture

Subscribing.

I have a feeling #523284: Optimize url() will cancel out any overhead added here, but I don't have time to test this today.

fgm’s picture

I wonder whether this could/should be integrated with custom_url_rewrite_outbound, since link rewrites are performed in both places (via url() in the case of l()), to avoid rewrite duplication.

effulgentsia’s picture

@fgm: I don't think so. custom_url_rewrite_outbound() happens later (after adjusting for locale and alias). A preprocess function or theme override would be able to adjust path before all that. However, given that custom_url_rewrite_outbound() exists, there might not be as compelling a use-case for preprocess functions to adjust the path. However, they can still be used to adjust the text and options. And a theme might want to do something weird like add a span inside every link (see #3). So the existence of custom_url_rewrite_outbound() doesn't remove the utility of this patch.

I'm intrigued by the possibility of #523284: Optimize url() "paying for" this patch. What say you, webchick?

Damien Tournoud’s picture

Status: Needs review » Closed (duplicate)

Erm? Reimplementing theme() just for l()?

See #318636-20: Make l() themable for a *way better* solution.