This patch creates theme_l() that can be overriden by themes. The original l() stays, because it is used for 'administrative' actions. The patch doesn't work yet, since for some reason theme('l'); doesn't actually call theme_l(). I need to look into this problem a little closer, but perhaps somebody can give me a nudge in the right direction.

Files: 
CommentFileSizeAuthor
#67 318636-themable-l-67.patch6.11 KBeffulgentsia
Passed: 14674 passes, 0 fails, 0 exceptions
[ View ]
#66 318636-themable-l-66.patch5.9 KBeffulgentsia
Passed: 14694 passes, 0 fails, 0 exceptions
[ View ]
#62 318636-themable-l-62.patch4.41 KBeffulgentsia
Failed: 14652 passes, 0 fails, 1 exception
[ View ]
#60 318636-themable-l-60.patch4.45 KBeffulgentsia
Failed: 14618 passes, 0 fails, 1 exception
[ View ]
#59 318636-themable-l-59.patch4.45 KBeffulgentsia
Failed: 14681 passes, 0 fails, 1 exception
[ View ]
#57 318636-themable-l-56.patch4.05 KBeffulgentsia
Failed: 14674 passes, 0 fails, 1 exception
[ View ]
#54 318636-themable-l-54.patch4.7 KBeffulgentsia
Passed: 14682 passes, 0 fails, 0 exceptions
[ View ]
#51 318636-themable-l-51.patch2.82 KBeffulgentsia
Passed: 14690 passes, 0 fails, 0 exceptions
[ View ]
#51 ab-318636-51-head.txt1.57 KBeffulgentsia
#51 ab-318636-51-patch.txt1.57 KBeffulgentsia
#45 318636-themable-l-45.patch2.58 KBeffulgentsia
Passed: 14708 passes, 0 fails, 0 exceptions
[ View ]
#44 318636-themable-l-44.patch2.57 KBeffulgentsia
Passed: 14703 passes, 0 fails, 0 exceptions
[ View ]
#37 benchmark_318636.txt1.56 KBeffulgentsia
#33 drupal.theme-link.33.patch3.75 KBsun
Failed: 13265 passes, 1 fail, 0 exceptions
[ View ]
#30 theme_link-318636-30.patch4.88 KBeffulgentsia
Passed: 14706 passes, 0 fails, 0 exceptions
[ View ]
#29 drupal.theme-link.29.patch4.34 KBsun
Passed: 14683 passes, 0 fails, 0 exceptions
[ View ]
#27 drupal.theme-link.patch4.29 KBsun
Failed: 13585 passes, 121 fails, 460 exceptions
[ View ]
#24 themable-l-318636-24.patch4.57 KBjrchamp
Failed: Failed to install HEAD.
[ View ]
#23 318636-themable-l-22.patch4.82 KBeffulgentsia
Passed: 14686 passes, 0 fails, 0 exceptions
[ View ]
#20 318636-themable-l.patch2.17 KBDamien Tournoud
Passed: 14689 passes, 0 fails, 0 exceptions
[ View ]
#18 318636-themable-l.patch1.99 KBDamien Tournoud
Failed: 13712 passes, 0 fails, 1 exception
[ View ]
#6 theme_l_01.patch1.88 KBXano
Failed: Failed to apply patch.
[ View ]
theme_l_00.patch1.48 KBXano
Failed: 6715 passes, 212 fails, 29 exceptions
[ View ]

Comments

Well, it makes sense and is a step toward a non-HTML output for Drupal. You will have to register 'theme_l' in a hook_theme().

I thought so too, but in what hook_theme() implementation? I cannot find where the theme functions inside theme.inc are registered.

Although I'd keep l() the same, I'd rather use theme_link to go along with the naming of theme_links.

I'd also rather pass in the straight $attributes array instead of the HTML for the attributes from drupal_attributes.

What's yaman about?

+function theme_l($path, $attributes, $text) {return '<strong>yaman</strong>';
+  return '<a href="' . $path . '"' . $attributes . '>' . $text . '</a>';
+}

Debugging code?

Err, yes, that was debugging code. Must have overlooked it for some reason :S

Status:Needs work» Needs review
StatusFileSize
new1.88 KB
Failed: Failed to apply patch.
[ View ]
  • This patch works. I registered theme_link() in drupal_common_theme().
  • Renamed theme_l() to theme_link().
  • Passed on attributes as an array.
  • Removed debugging code.

Status:Needs review» Needs work

The last submitted patch failed testing.

According to swentel This patch still applies without problem. We both cannot figure out why the PHP filter tests failed. Could somebody please a look at this problem?

I've looked at it and identified the l() in the following line in php.install as the bad guy:

<?php
drupal_set_message
(t('A %php-code input format has been created.', array('%php-code' => l('PHP code', 'admin/settings/filters/' . $format))));
?>

If the l() is replaced/removed, the all PHP Filter tests run without fails.

Seeing that only the simpletest environment and not the drupal installation itself shows any errors, leads me to think that the problem lies not with the patch, but within Simpletest. How and why, I am not sure, but I believe it has something to do with the cache/theme registry, because a single PHP.test case runs fine directly after clearing the cache.

Status:Needs work» Needs review

Requesting re-test.

Patch passes. I'd like to see some more reviews! :D

Status:Needs review» Needs work

The last submitted patch failed testing.

Requesting re-test. Testing server only says the tests failed, but all tests look OK.

Status:Needs work» Needs review

Requesting re-test. Testing server only says the tests failed, but all tests look OK.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

Come on, testing bot. Convince me.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.99 KB
Failed: 13712 passes, 0 fails, 1 exception
[ View ]

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.17 KB
Passed: 14689 passes, 0 fails, 0 exceptions
[ View ]

Better patch:

  • move theme_link() to common.inc (theme.inc is only loaded in BOOTSTRAP_FULL), and remove debug code,
  • make CommonURLUnitTest a full DrupalWebTestCase: because it now calls the theming layer, it cannot be a DrupalUnitTestCase anymore.

can't we just make theme('link') call theme_link() if we are not fully strapped?

@Moshe: I'm pondering that. url() and t() are really two special cases: they *need* to be available as soon as possible, before Drupal is fully bootstrapped. I fear that if we allow the theming layer to gracefully fallback even when incorrectly called before the end of the full bootstrap we expose ourselves to more subtle bugs. There is no coming back: once something has been themed using the incorrect theme (or the base theme function), it will be used and mixed with other, correctly themed elements.

Issue tags:+Performance, +API clean-up
StatusFileSize
new4.82 KB
Passed: 14686 passes, 0 fails, 0 exceptions
[ View ]

I like #20 in terms of its elegance. But it adds 25% more time to the execution of l(). Link-heavy pages (admin/content, node with 300 comments) have 10% of their time spent in l(), so this would add 2.5% to page request time for those pages. If webchick is ok with this, so am I. But if she's not, this patch does the same thing in spirit, but optimized to only add 10% overhead to l(), and therefore, only 1% overhead to page requests (http://drupal.org/node/196908#comment-2137168). Even this might be a bit of a challenge to get her on board. Yes, the code is less pretty, but sometimes that's the price to pay for optimization.

Since #196908: Add a theme_link() function was correctly marked a duplicate, I'm adding a link to http://drupal.org/node/196908#comment-2137328: #523284: Optimize url() has a greater performance gain than this patch has a loss. But, #523284: Optimize url() can be committed and this one not, so we still need some community support around the overhead of this patch being worth the removal of a long-standing Drupal WTF (why can my theme change just about every piece of html that drupal outputs, oh EXCEPT LINKS?).

StatusFileSize
new4.57 KB
Failed: Failed to install HEAD.
[ View ]

It seemed like a decent chunk of this could be reordered to reduce overhead.

It also may be worthwhile to evaluate whether foreaching over an empty array is slow or not. I wouldn't expect to to justify the overhead of being wrapped in a if(!empty()), but it may be worth considering.

@effulgentsia, if you could provide some performance numbers that this patch has on your test environment, I'd appreciate it. Also, if this version of the patch makes some assumptions that are incorrect (global variables and state changes are confusing at times), please let me know.

@jrchamp: same exact performance numbers as http://drupal.org/node/196908#comment-2137168, but I like your changes from a stylistic standpoint. What I would like to happen next is the following test: fresh install of HEAD, enable devel_generate, give anonymous user permission to "administer content" and "access user profiles", use devel_generate to generate 500 nodes, and then Apache Benchmark /admin/content (as in "ab -c1 -n100 http://localhost:8888/d7/admin/content"). And do this for:

1) HEAD
2) With just #523284: Optimize url()
3) With just this patch
4) With both patches

I'm hoping #4 is faster than #1 and will therefore help justify this patch. But all 4 numbers are needed. If someone would like to do that, it would be very helpful. Otherwise, I'll do it when I have a chance to spend some time on that.

Tip: cache needs to be emtied between each test, because this patch affects the theme registry. And I recommend running each test twice in succession (without clearing cache between the 2 runs of the same test), because usually the 2nd run produces tighter and therefore more meaningful numbers, because caches and APC get well primed.

Category:feature» task
Priority:Normal» Critical
Issue tags:+D7 API clean-up
StatusFileSize
new4.29 KB
Failed: 13585 passes, 121 fails, 460 exceptions
[ View ]

So let's focus on theme_link() in here and do the conversion of links in forms and renderable arrays in #602522: Links in renderable arrays and forms (e.g. "Operations") are not alterable.

The approach taken in the last patch looks fine to me.

I've tried to apply some further tricks to it though. ;) Let's see what the bot thinks.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.34 KB
Passed: 14683 passes, 0 fails, 0 exceptions
[ View ]

Sorry, variable name mismatch.

Category:task» feature
Priority:Critical» Normal
StatusFileSize
new4.88 KB
Passed: 14706 passes, 0 fails, 0 exceptions
[ View ]

I like the code simplification in #29, but here's a slightly more optimized version (for webchick to approve this issue, every microsecond counts). Will post benchmark data (ala #25) soon.

Category:feature» task
Priority:Normal» Critical

category/priority changes in #30 weren't intentional.

Reimplementing theme() in l() is a total no-go for me. I can live with the performance implications, but not with the code ugliness introduced here.

StatusFileSize
new3.75 KB
Failed: 13265 passes, 1 fail, 0 exceptions
[ View ]

Thought more about this. There is absolutely no point for preprocessing and processing here. Either it's the default theme_link(), or it's JSON module's json_link() or whatever.

Hence, we can certainly special-case this.

@Damien: You can live with the performance implications, but ain't no way in hell that webchick will. #33 is as performant as #30, which is as good as we've gotten with it. I can live with the loss of preprocess function support in #33 in order to have less ugly code. Can you live with #33 from a code-cleanliness standpoint?

Much nicer! I'm glad we were able to remove the processing functions to get #33. It does seem like theme() is a beast that should probably get some performance attention (it seems like most of the processing which occurs for the $info could be preserved in $hooks and any chunks which would repeat the modifications done to info could be skipped on subsequent passes). That's likely for a different issue though.

@effulgentsia: your benchmarks are apparently flawed. With your own benchmark script, I only measure a 5% overhead added by the patch in #20.

StatusFileSize
new1.56 KB

@Damien: fascinating. I'm re-uploading my benchmarking script. Same script, but no need to keep referencing that other issue. Here's my protocol: copy this to drupal root, rename .txt to .php. Open 3 browser tabs: 1) drupal home page, 2) phpmyadmin, 3) the url of the benchmarking script.

Test 1: Just after a HEAD install, refresh the benchmarking script: my results: 69.5098114014 +/- 0.276735992734
Test 2: Apply patch #20, use phpmyadmin to empty cache table, refresh drupal home page, refresh benchmarking script: my result: 100.095853806 +/- 0.293068306058
Test 3: Revert patch #20, apply patch #33, use phpmyadmin to empty cache table, refresh drupal home page, refresh benchmarking script: my result: 75.8947300911 +/- 0.358750393776

I'm running PHP 5.2.6 (using MAMP) with APC on.

What are your numbers?

Clean HEAD:

0.132071971893 +/- 0.00379160882552 59.4390368462 +/- 0.524119472308

HEAD + #20:

0.132327079773 +/- 0.0100945694353 62.9671502113 +/- 0.587171209051

HEAD + #33:

0.119791030884 +/- 0.0241602692632 62.4159598351 +/- 0.370888953288

The improvement between #20 and #33 is very close to the error margin...

I'm running PHP 5.2.6-3ubuntu4.2, with APC enabled. Would you be, by any chance, running your benchmarks with xdebug enabled?

Assigned:Xano» Unassigned

This is a community effort at this point.

Assigned:Unassigned» Xano

I use Zend debugger, but I get the same results as #37 with it not loaded. You're on Linux, I'm on Mac. I don't know why that would make this kind of difference though. Can anyone else following this issue post their numbers?

Assigned:Xano» Unassigned

cross-post. sorry.

If Damien's numbers are more reflective of reality than mine, I agree that #20 is the better patch.

StatusFileSize
new2.57 KB
Passed: 14703 passes, 0 fails, 0 exceptions
[ View ]

This one's a small change to #20 - just changing variable names for theme_link(), and also caching bootstrap phase. I'm submitting this so that just the theme() overhead can be compared relative to #33, not the combination of theme() and drupal_get_bootstrap_phase().

With this patch, I get 84.0980029106 +/- 0.211646928941. Still curious what other people get as benchmarks for HEAD, #20, #33, and #44.

StatusFileSize
new2.58 KB
Passed: 14708 passes, 0 fails, 0 exceptions
[ View ]

Hey, I think I found a solution that I hope can make everyone happy. Check it out. This has next to 0 overhead compared to HEAD (2 extra if statements on a boolean variable) when no module or theme registers a 'link' theme hook. However, it leaves open the possibility for a module or theme to register a theme hook and choose to take the performance overhead in order to accomplish something special.

Examples of things you can't currently do in HEAD that this would enable a module or theme to do:
1) Add a span tag inside every link, or selectively based on some criteria
2) Add a custom class for every link, or selectively based on some criteria
3) Add a target="_blank" attribute for every link that points to an external website

Note: while I think the work in #602522: Links in renderable arrays and forms (e.g. "Operations") are not alterable is quite nice and should be committed because of what it enables with respect to operations links, it doesn't fully make this issue obsolete unless we replace EVERY call to l() within core with a conversion to a renderable element.

What do y'all think?

Status:Needs review» Reviewed & tested by the community

Seems like a good compromise to me.

Just checking, but did you mean to lose the following chunk between the two revisions?

<?php
+    'link' => array(
+     
'arguments' => array('text' => NULL, 'href' => NULL, 'attributes' => array()),
+    ),
?>

Edit: I guess you did. That's so you fail the isset when it hasn't been overridden, correct?

@jrchamp: yes. a contrib module or theme can add a 'link' theme hook if they want one. the point is for core to not have it and therefore not have the performance penalty by default. this will need documentation, so webchick, if you commit this before i have time to write the doc for it, please set it to "needs work" and give it the "needs documentation" tag.

Issue tags:-Performance

Removing performance tag since this is more or less a no-op.

Hm. I am not sure about this patch.

In part this is because I'm sensitive to how slow we're making just regular links on the page, given this patch and #320331: Turn custom_url_rewrite_inbound and custom_url_rewrite_outbound into hooks and #602522: Links in renderable arrays and forms (e.g. "Operations") are not alterable are all vying to slow down links down by "just a little bit."

But while the other two have real use cases, I am trying to figure out what the use case is for a theme function this generic. Would you realistically want to wrap spans around EVERY link on the site? No. So you would need further special casing in your theme override function, which would just slow this code down even more. :\

While the patch as-is has minimal performance impact, which is good, it's going to create inconsistent results since it will behave in up to three different ways, depending on the current 'bootstrappiness' of the site.

The XHTML v HTML 5 case is about the only one that makes sense to me. But this seems like an overly heavy solution to that problem when we could probably just store the doctype somewhere and switch on that. It gets into a whole kettle of fish though about how to let Drupal's output react to different output formats which we never figured out before code freeze.

I'm leaning towards bumping this to D8 to figure all of this out. Where would that leave us?

StatusFileSize
new1.57 KB
new1.57 KB
new2.82 KB
Passed: 14690 passes, 0 fails, 0 exceptions
[ View ]

I think it makes sense to evaluate each issue on its own merits. #320331: Turn custom_url_rewrite_inbound and custom_url_rewrite_outbound into hooks introduced an alter hook for manipulating urls, which is awesome, and I hope that further work on #523284: Optimize url() can mitigate any performance impact created by that new hook. #602522: Links in renderable arrays and forms (e.g. "Operations") are not alterable makes sense because the architecture of FAPI is that the more well-structured a form is, the more can be adujsted inside of hook_form_alter() and hook_form_FORM_ID_alter(), and that makes it easier for developers to adjust forms without hunting down other hooks to use or jumping through reg-ex hoops. However, that issue needs to be evaluated relative to its performance cost, and I'm still waiting on a response to http://drupal.org/node/602522#comment-2194944 to know how aggressively to optimize that patch considering that it only affects performance of admin pages.

Those two issues don't make this one irrelevant, but they do make it more targeted. Because of #320331: Turn custom_url_rewrite_inbound and custom_url_rewrite_outbound into hooks, one would not need to use the 'link' theme hook provided by this issue if all that is wanted is URL adjustment. If #602522: Links in renderable arrays and forms (e.g. "Operations") are not alterable lands, one would not need to use the 'link' theme hook provided by this issue if all that is wanted is to adjust operations links of forms. However, there remains the use-case of wanting to adjust the markup of links generically. There are 10,000+ users of the http://drupal.org/project/extlink module, indicating that adding icons to links and/or setting the target attribute of a link is a popular use-case. That module uses jQuery, and for optimal server performance, that solution remains a good one, but why is it impossible within Drupal to create the equivalent functionality without resorting to javascript? This issue would make it possible to create a non-js version of that module, and people could make their own choice as to whether they want the non-js version (resulting in slightly increased server load) or the js version (in order to not increase server load). That's not the only use-case, but it does indicate that links are markup like everything else, and as such should be themeable. If we provide generic theme_image() and theme_table(), why not theme_link()?

Now, absolutely, we do not want to decrease performance for everyone in order to make something possible for relatively few people. That's why I'm attaching an even more optimized patch and benchmarks. These benchmarks indicate that there is no measurable performance impact of this patch if there is no 'link' theme hook registered. For the admin/content page showing 50 of 500 nodes, served to an anonymous user with "administer nodes" permission, HEAD = 112.035 and Patch = 112.014. The standard deviation of the sample = 0.7 / sqrt(1000) = 0.022, so the two numbers are within statistical equivalence (the patch does not really improve performance).

Also, I hope this patch addresses your concern about "bootstrappiness". It's quite simple really, if it's possible to call theme('link'), do so. If it's not, don't. The only inconsistency is if a module or theme does implement a 'link' theme hook, but the theme system hasn't been bootstrapped yet. But, I don't know if we actually have a use-case of l() being called before the theme system has been bootstrapped, and if we do, then what can we do? If this use-case exists, then it involves calling a function that returns HTML before the theme system is operational, which is either a bug in and of itself, or else having the function return non-themed default markup is the expected behavior.

Hm. This method isn't ideal, because there is no theme_link() function to look up on api.drupal.org, nothing in the theme registry for Devel Themer to identify as a possible candidate when hovering over the link (actually, worse; it would show up as a candidate function override on some sites [that have ext links module installed] but not others...), etc. Therefore, the only way someone who was not involved in this issue could possibly know that links are themable is to read the source code of l().

Furthermore, if we take your very valid use case of extlinks module, it will need to register theme_link() in order to provide a little external link icon on all outgoing links. But what if we also install social_network_links module, which also registers theme_link() in order to add stupid Digg, Twitter, etc. icons next to any links going to those sites? Suddenly KABOOM, we have a function name collision and PHP dies until you go into your database and fiddle with the system.status column.

If core were to provide the registry for theme_link() itself, then both of these modules would use preprocess functions and we wouldn't need to worry about function name collisions. But then we're right back to the performance hit of earlier patches unless we move the special casing to... I guess theme()? Bleh.

Thoughts?

Some possibilities:

1) We add a module to core (I can't think of a great name for it right now, but let's say, "themeable_links" until a better one comes along), and all this module does is implement a hook_theme() and a theme_link(). Disabled by default. Modules/themes wanting to take advantage of it can list it as a dependency. Pros: simple. Cons: the #smallcore folks would probably cringe at adding another module to core.

2) We add a settings.php setting (perhaps even a checkbox on admin/config/development/performance for those weird folks who like GUIs), and make system.module register theme_link() in hook_theme() only if that setting is enabled. Pros: still pretty simple, no extra module in core. Cons: modules/themes wanting to take advantage of it would need to force the setting or implement their own hook_theme() that registers the theme_link() function regardless of the setting or include in their readme.txt instructions that the site builder needs to have that setting enabled to receive that part of the module's/theme's benefits.

3) Add a theme_link() function in theme.inc, but not have system.module register it. Instead modules would need to implement hook_theme() to register it, but could do so without having their own copy of the function itself. There's no negative to multiple modules implementing the same 'link' entry in hook_theme(). Pros: no extra module in core, no extra setting. Cons: similar to #2, but without the module author having a choice of which of the 3 WTFs to use.

I'm more or less okay with any of these. Let me know if one of them strikes your fancy, or if we need to keep brainstorming. I'm done for the day, but can pop in on IRC tomorrow if that's a better medium for further brainstorming. Thanks for still being open to this. I hope we can come up with something you like.

StatusFileSize
new4.7 KB
Passed: 14682 passes, 0 fails, 0 exceptions
[ View ]

@webchick: does this work for you?

Status:Reviewed & tested by the community» Needs work

The concept is solid! I am shocked someone came up with a performant solution. However, I would definitely drop the else { } from the end as the if before it returns. Also add a variable_get to make it possible to totally kill the theme init and the registry get in case someone is hell bent they do not want this fantastic feature. Thanks!

Also note that AJAX pages might want to skip theme system and yet use a link -- that's a use case for the variable aside from the performance-bent idiot sites of mine :)

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new4.05 KB
Failed: 14674 passes, 0 fails, 1 exception
[ View ]

This patch implements chx's feedback from #55 and IRC. Specifically:

  • A variable_get() killswitch to allow certain edge cases.
  • Dropping an unnecessary else {} clause.
  • Removing the change to CommonURLUnitTest. The change was introduced in #20, but is not necessary with this implementation of theme system testing.

In IRC, Damien and Moshe weighed in, and between them, chx, and me, I think we reached the following areas of agreement and disagreement (anyone, please correct me if I'm misrepresenting your positions):

  • We agree that the ability to have link markup be exposed to the theme system for people who want it is a useful feature.
  • We either agree, or at least can accept, that calling theme('link') in all cases would incur too much overhead and run into the bootstrap problem, so some level of switching between theme('link') and an alternate implementation is appropriate.
  • We disagree on where code ugliness introduced to satisfy optimization needs is justified and where it isn't. Chx would like to see more micro-optimization, such as replacing the if($must_theme) with if(empty($skip_theme)), because the latter is theoretically faster, but I haven't been able to produce benchmarks that demonstrate this in a statistically significant way. On the flip side, Damien and Moshe would like to see cleaner code, such as replacing the inline HTML with a direct call to the theme_link() function, because a literal function call (unlike theme()) is pretty cheap (although chx points out that while cheap, it's not free).

So, there's general agreement that the API change introduced in this patch is good, but there will be a follow-up issue opened to debate non-API-altering code cleanliness vs. micro-optimization. A challenge in having that debate be productive will be what to use as a benchmarking scenario, since the performance differences involved are tiny and machine dependent. No one indicated a desire to hold up this patch until full consensus on every implementation detail is reached.

Status:Reviewed & tested by the community» Needs review

+++ includes/common.inc 3 Nov 2009 22:17:30 -0000
@@ -2702,6 +2703,34 @@ function l($text, $path, array $options
+    if (variable_get('themeable_links', TRUE)) {

This variable should be called "theme_links".

+++ includes/common.inc 3 Nov 2009 22:17:30 -0000
@@ -2702,6 +2703,34 @@ function l($text, $path, array $options
+      $must_theme = (
+        !empty($registry['link']['preprocess functions']) ||
+        !empty($registry['link']['process functions']) ||
+        !empty($registry['link']['includes']) ||
+        !isset($registry['link']['function']) ||
+        ($registry['link']['function'] != 'theme_link')
+      );

Either we write all of this on one line, or we use this pattern:

$must_theme = !empty($registry['link']['preprocess functions']);
$must_theme = $must_theme || !empty($registry['link']['process functions']);
...

+++ includes/common.inc 3 Nov 2009 22:17:30 -0000
@@ -2702,6 +2703,34 @@ function l($text, $path, array $options
+        !empty($registry['link']['includes']) ||

Why is a include file a trigger?

+++ includes/theme.inc 3 Nov 2009 22:17:30 -0000
@@ -1367,6 +1367,34 @@ function theme_status_messages($variable
+function theme_link($variables) {
+  $text = $variables['text'];
+  $path = $variables['path'];
+  $options = $variables['options'];
+  return '<a href="' . check_plain(url($path, $options)) . '"' . drupal_attributes($options['attributes']) . '>' . ($options['html'] ? $text : check_plain($text)) . '</a>';

I don't see the need to extract these array values into separate variables before usage. We did this as a temporary measure for the global theme function change, but that doesn't mean we should do it in all new functions equally.

This review is powered by Dreditor.

StatusFileSize
new4.45 KB
Failed: 14681 passes, 0 fails, 1 exception
[ View ]

With #58 feedback. Please RTBC if ok.

StatusFileSize
new4.45 KB
Failed: 14618 passes, 0 fails, 1 exception
[ View ]

Oops. With a typo fix.

+++ includes/common.inc 4 Nov 2009 00:24:40 -0000
@@ -2683,6 +2683,7 @@ function drupal_attributes(array $attrib
+  static $must_theme = NULL;

Reading it for the second time - what about $use_theme ?

+++ includes/common.inc 4 Nov 2009 00:24:40 -0000
@@ -2702,6 +2703,36 @@ function l($text, $path, array $options
+    if (variable_get('theme_links', TRUE)) {

Sorry, my fault - singular, of course. "theme_link" is the function, and we should use the same variable name here to avoid confusion.

+++ includes/common.inc 4 Nov 2009 00:24:40 -0000
@@ -2702,6 +2703,36 @@ function l($text, $path, array $options
+      $must_theme = !empty($registry['link']['preprocess functions']) || !empty($registry['link']['process functions']);
+      $must_theme = $must_theme || !empty($registry['link']['includes']);
+      $must_theme = $must_theme || !isset($registry['link']['function']) || ($registry['link']['function'] != 'theme_link');

Shouldn't we start with the most trivial test, which is currently the last? - i.e. a registered theme_links() function?

+++ includes/common.inc 4 Nov 2009 00:24:40 -0000
@@ -5335,6 +5366,9 @@ function drupal_common_theme() {
+    'link' => array(
+      'variables' => array('text' => NULL, 'path' => NULL, 'options' => array()),
+    ),

Wait. So we define this by default, and it's used whenever the theme system is read and the "theme_link" variable isn't FALSE?

So we are introducing a performance decrease to all sites even when they have no module installed that uses this feature?

I'm on crack. Are you, too?

StatusFileSize
new4.41 KB
Failed: 14652 passes, 0 fails, 1 exception
[ View ]

With #61.

Wait. So we define this by default, and it's used whenever the theme system is read and the "theme_link" variable isn't FALSE?

So we are introducing a performance decrease to all sites even when they have no module installed that uses this feature?

See #52. We want the theme_link() function to show up in API docs, and be used for allowing modules to implement preprocess functions without needing to implement a theme function as well. The trigger for $use_theme isn't whether the hook exists, but whether anything other than the default is wanted, so no performance degradation for the sites that don't do anything custom.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

I'm in the middle of something right now, but I'll fix the test failure (not sure what changed between #57 and this one) within the next hour or two.

Status:Needs work» Needs review
StatusFileSize
new5.9 KB
Passed: 14694 passes, 0 fails, 0 exceptions
[ View ]

Prior to #57, we were changing CommonURLUnitTest to inherit from DrupalWebTestCase instead of DrupalUnitTestCase. I thought we could get rid of that, but it turns out we can't, because in unit tests, we're fully bootstrapped but can't query the db, so we can't call drupal_theme_initialize(). I'd like to get feedback on this patch's way of solving that. Please set to RTBC or tell me to revert to making CommonURLUnitTest inherit from DrupalWebTestCase.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new6.11 KB
Passed: 14674 passes, 0 fails, 0 exceptions
[ View ]

In IRC, sun suggested better comments explaining the cruft in the common.test hunk, but otherwise agreed that this is better than making it a web test case.

webchick: just in case you're trying to pick up from where you last left off with this, I suggest reading from #55 on down, and especially #57.

Status:Reviewed & tested by the community» Needs work
Issue tags:-D7 API clean-up+Needs Documentation

This patch still gives me an 'ooky' feeling. However, every possible attempt has been made to make it performant for the default use case, and it has the thumbs up from chx, DamZ, moshe, catch, and others in the performance crew. It does fix the last remaining part of our markup that is not themeable, and consensus seems to be that this is important for allowing Drupal 7 to utilize XHTML 2, HTML 5, and other next-generation markup that comes out during its lifecycle.

This patch is very borderline, though. While it was originally marked RTBC by 10/15, the resulting patch on 11/3 shows very little in common. However, with much gnashing of teeth, I have decided to commit this to HEAD anyway.

This change will need to be reflected in the theme upgrade guide.

Status:Needs work» Fixed

Yay! Thanks, webchick, for committing this despite your valid reservations.

Documentation: http://drupal.org/update/theme/6/7#theme-link

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

This patch still gives me an 'ooky' feeling.

Your intuition was sharp: #1213536: Non-resettable theme_get_registry() cache causes problems for non-interactive installations. I still think the patch was a net positive though.

Leaving this issue closed, but cross-linking another follow-up: #1187032: theme_link needs to be clearer about saying not to call it directly