Reusing code is a good thing and many theme functions are generalized implementations so that the they can be re-used in several different scenarios. For example, theme_links() is used in 22 different places in Drupal core.

However, this reuse of code means that it is difficult, and often impossible, to alter the implementation of one specific theme call without affecting all other calls to that same function.

Again, for example, theme_links is used for page.tpl's main menu links, the node links, the comment links, and forum links (to name a few). And if you want to change how node links are displayed, you’re SOL. From inside an overridden THEMENAME_links() implementation there’s almost no way to know which specific implementation is being called at the time your function is called.

So, do we change all 22 instances of theme_links to use 22 different theme functions? Hell, no!

Fortunately, theme() already has a built-in solution for this problem; we just need to implement it. Let me quote the theme_links docs ( http://api.drupal.org/api/function/theme/7 ):

Parameters
$hook The name of the theme function to call. May be an array, in which case the first hook that actually has an implementation registered will be used. This can be used to choose 'fallback' theme implementations, so that if the specific theme hook isn't implemented anywhere, a more generic one will be used. This can allow themes to create specific theme implementations for named objects.

So if we add an optional “named implementation” along with the “fallback implementation” that we already have, we would make it possible for themers (and module developers) to create a named theme implementation that overrides the fallback implementation, but only for that specific call to theme().

For example, let’s change node links’ call to theme from theme('links', etc.) to theme(array('node_links', 'links'), etc.). Then we could optionally override theme_links with THEMENAME_node_links(), which would leave all our other calls to the fallback theme_links untouched.

Awesome!

Note: We’ll need to verify that the render api works the way theme() works. In IRC, Moshe said that if it doesn’t, it should.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

For anything in a preprocess function or normal function, sweeeet. My concern would be that it's not particularly friendly for templates to have to put arrays in them, if you're calling theme('links') there. I don't know if we still are, though.

Also, if I'm reading the code at http://api.drupal.org/api/function/drupal_render/7 correctly, drupal_render() (and therefore render()) should be fine. If set, #theme is passed on to the theme() function directly which means if you make it an array it will Just Work(tm) as expected. Nice.

I don't know if we can sneak this in for D7, but I am in favor of it if so.

Zarabadoo’s picture

I like this move. Another bit of flexibility using existing functionality is a good thing.

yoroy’s picture

This would be a verynicetohave indeed. Improved DX and more freedom to create not-typical-Drupal-looking sites.

chx’s picture

benchmarks? esp for comment links. the theme(array() is not used much imo so we do not know the speed implications.

Crell’s picture

Passing an array is no more expensive than passing a string. In theme(), making $hook an array involves a foreach() and an isset() per item in the array. I can't see it being an appreciable difference in performance either way.

mfer’s picture

/subscribe

mortendk’s picture

im getting a warm and fuzzy felling through my body :)

This could remove one of the key problems in theming!
We are today bound into the idea that when a module calls a theme function we cant do anything about it.

So if module A calls the theme(link) then inside link() i have no idea where the call came from. i i dont misunderstands this - we would now force all theme calls to identify them self, and that would be a giant leap for the themer.

JohnAlbin’s picture

Status: Active » Needs review
FileSize
24.88 KB

I realized that trying to make named theme implementations for every single call to theme() would make this an impossible-to-review patch. There are 591 instances of theme(), #theme or #theme_wrappers in core. :-p

So this patch is specifically to fix the worst offender, theme_links(). We should use this patch as a template for any follow-up patches (if there's time). If this is the only named theme implementation to make it into D7, themers will still rejoice.

This patch is 95% docblock comments. The only code modifications it makes is changing every instance of my_theme_links, with theme(array('SOMETHING_links', 'links'), and my_theme_links with '#theme' => array('SOMETHING_links', 'links'). Dead simple.

By itself, core will function exactly the same; the fallback theme_links implementation will continue to be used.

And, if a contrib theme or module registers and defines one of those named implementations, theme() will use the newly registered theme function instead. Themes will still be able to override a module-defined named implementation just like any normal theme function.

Status: Needs review » Needs work

The last submitted patch failed testing.

RobLoach’s picture

Although I like the approach, it seems like a very hard coded solution. The good thing is that it gives us a different theme function for comment links, node links, book links, etc, but at the same time, I think it could all just be abstracted to one singular "link" type. If we battle #602522: Links in renderable arrays and forms (e.g. "Operations") are not alterable first, we might be able to put in some additional attributes into the links in this issue to give some more context to the links themselves for the theme to override. Another related issue is #318636: Make l() themable.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
24.92 KB

@Rob Loach, yes, this is a hard-coded approach. That's how D6/7's "named theme implementation" feature works.

I think the confusion comes because the current implementation of theme_links does 2 separate things in one go (which I hate, but am not trying to change in this patch): 1.) it renders each individual link and 2.) wraps the collected links in an unordered list.

This patch is an attempt to make #2 easier to change depending on which grouping of links you have; it doesn't change theme_links or any API at all, it's just using an existing API. The 2 issues you mention are attempting to change #1 and they look great! But they are definitely separate and complementary problems to solve.

Wow, I just realized the patch in #8 was completely corrupted by me when I accidentally hit search-and-replace instead of search in my text editor. whoops. No wonder it didn't apply.

JohnAlbin’s picture

When this patch is applied this will be the documentation for theme_links:


/**
* Return a themed set of links.
*
* This is the "fallback" theme implementation for theming links. For specific
* function calls of theme_links(), this can be overridden by registering and
* defining more-specific "named" theme implementations. The possible named
* theme implementations of theme_links() in core are:
* - theme_blog_node_links()
* - theme_book_node_links()
* - theme_comment_links()
* - theme_comment_node_links()
* - theme_forum_links()
* - theme_locale_block_links()
* - theme_node_admin_links()
* - theme_node_links()
* - theme_poll_results_links()
* - theme_statistics_node_links()
* - theme_main_menu_links()
* - theme_secondary_menu_links()
* - theme_toolbar_menu_links()
* - theme_toolbar_user_links()
* - theme_toolbar_shortcuts_links()
* - theme_translation_node_links()
* - theme_update_version_links()
* - theme_upload_node_links()
*/
?>

That, hopefully, in a nutshell explains what this patch does.

JohnAlbin’s picture

Version: 7.x-dev » 8.x-dev

*sigh* No reviews and code slush is officially over. :-(

Annoyingly, we can't even fix this in contrib for D7, since the only way to extend the API in this way is to hack core.

I wish I'd thought of this solution earlier. bah!

sun’s picture

Version: 8.x-dev » 7.x-dev
Category: task » bug

Issue title: Impossible... - this is a bug.

Let's kick away all of those granular API documentation additions and just keep the list in the docs for theme_links.

This is just leveraging the existing capability of theme function suggestions in the theme system. This is no API change, no functional change, or whatsoever change. It's a bugfix that could even be backported.

See also #634472: Menus cannot be themed differently

sun’s picture

However, we may want to consider a standard for those suggestions. For example, Views leverages those suggestions since a long time: (shortened)

function _views_theme_functions($hook, $view, $display = NULL) {
  $themes = array();
  $themes[] = $hook . '__' . $view->name;
  $themes[] = $hook;
  return $themes;
}

which boils down to using the original theme hook as prefix, followed by two underscores, and followed by any additional identifiers, i.e.

  theme(array('HOOK__IDENTIFIER', 'HOOK'), ...);

Hence, a few examples:

theme_links__node()
theme_links__node_admin()
theme_links__node_blog()
theme_links__node_book()
theme_links__node_comment()
theme_links__node_forum()
theme_links__comment()
theme_links__locale_block()
sun’s picture

FileSize
13.91 KB

Of course, we want to prefix the identifier in suggestions with the module name to prevent name clashes.

sun’s picture

Title: DX: impossible to override specific theme implementations when using generalized theme functions » theme_links() is not really themeable
Assigned: Unassigned » sun

What are we waiting for?

sun requested that failed test be re-tested.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
13.96 KB

Re-rolled against HEAD.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Agree that this is has no API impact and elegantly fixes the bug.

mfer’s picture

ooooo, I like this pattern.

effulgentsia’s picture

Issue tags: +Performance

I love this patch. But chx asked about benchmarks in #4, and I'm getting very weird results that I'm hoping someone can refute or shed light on.

Fresh install of HEAD with default profile, disable overlay module (it's newly committed, and I want to remove it as a variable from this test) and rdf module (adds processing time to comments that's irrelevant to this issue, and I want to remove it as a variable), and enable devel_generate. Give anonymous role permission to "view comments" and "post comments with approval" (in addition to what's included by the default profile). Generate a node with 50 comments. Access that node as anonymous.

This results in 54 calls to theme_links(), which makes sense (50 comments + 4 other ones).

When I micro-benchmark the difference between theme('links', array('links' => array()) and theme(array('links__comment_node', 'links'), array('links' => array()), on my computer, I see a 2us difference (10us vs 12us). If instead of calling theme(), I set up a renderable element, and call drupal_render(), the total time increases, but the difference is still 2us. If I pass actual links to theme in the $links variable, again, total time increases, but difference is still 2us.

Ok, so 2us * 54 invocations would suggest that it should produce a 0.1ms difference in page request time. But, when I Apache benchmark HEAD vs the patch for that node, I see 1.0ms+ difference, which is more than 0.5% of page request time (169ms vs 170ms). I've run this over many iterations and this is much larger than any statistical noise. I cannot figure out why the performance impact of this patch is 10x larger than what would be predicted by micro-benchmarks.

The 1ms difference is clearly wrong. There is no way that passing an array of hooks instead of a scalar hook contributes an extra 20us per invocation. Can someone please generate Apache benchmarks for this that shows the difference to be on the order of 0.1% of page request time or less? Or, if you're also seeing 0.5% or more, any thoughts on why?

sun’s picture

Thanks!

But, um, can we please compare 1ms or <1ms to a monster theming WTF? ;)

JohnAlbin’s picture

I'd never realized thats what Views was doing with its double-underscore stuff. So +1 to making the named theme implementation using a pattern that is already in use. theme_links__NAME makes sense to me and makes it clear that it is related to theme_links, whereas theme_NAME_links (as in my original patch) didn't.

sun’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -Performance +DrupalWTF

This needs a quick re-roll after #626286: Make contextual links a module has landed.

Since Overlay went in and a lot happened in the meantime, it wouldn't hurt to grep once again through core for new instances.

effulgentsia’s picture

I agree that even if it does add 0.5% - 1% to node with 50 comments page, it would be worth it. But it doesn't make sense for the impact to be that large, so either something is f'd up on my computer (and I'd like to know if that's the case), or we need to figure out why the impact is that large and optimize it, and since this is the first time we're adding usage of passing an array of hooks to theme() in core, this gives us a chance to understand its performance characteristics better, which we can use in deciding where else it's okay to use it. But, I'm fine with that being dug into after this patch lands; no need for this awesome patch to be held up.

If you're re-rolling anyway, please add a 'pattern' => 'links__' entry in drupal_common_theme() for it.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
12.7 KB

Re-rolled for HEAD changes, including removal of upload.module and addition of contextual.module. Also added the 'pattern' entry in drupal_common_theme(). Grepped and didn't find any other instances of 'links' that needed an identifier. There is this call in theme_system_themes_page():

$output .= theme('links', array('links' => $theme->operations, ...

Elsewhere, operations links are rendered in a table rather than via theme('links'), so we have no convention for this one.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.2 KB

Setting this to RTBC as it was in #25. I've been benchmarking the node with 50 comments page most of the day over on #538164: Comment body as field, so I wanted to re-try benchmarking this issue (leaving overlay and rdf modules enabled, as they are in a default HEAD install), and this time I got results that made sense:

HEAD: 209.46ms
Patch 28: 209.48ms

Micro-benchmarks still show a 2us difference, so for 54 calls in the page request, I'd expect to have seen a 0.1ms difference. But noise from other factors may be obscuring a 0.1ms difference if it actually exists.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thank you! :)

Frankly, I didn't even know that this 'pattern' key exists. But it's even documented: http://api.drupal.org/api/function/hook_theme/7 :)

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

Re-test of drupal.theme-links.28.patch from comment #28 was requested by webchick.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

as per #29

webchick’s picture

Status: Reviewed & tested by the community » Needs work
effulgentsia’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
11.78 KB

re-rolled.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

In retrospect, it's probably a bit late for all three of these patches, but since I committed the first in the series, I guess we might as well finish the set. At least it doesn't break anything. :) (famous last words)

effulgentsia's performance benchmarks look ok to me, given the themer experience gains we get from this patch.

Committed to HEAD. Needs documenting in the theme upgrade guide.

yoroy’s picture

Thank. You. Very. Much.

effulgentsia’s picture

I added http://drupal.org/update/modules/6/7#theme_links_with_context for module developers, but that's not enough: documentation is also needed in the theme upgrade guide. However, it seems like that documentation should also include a mention of what the specific identifiers are in core (e.g., '__node', '__comment_node', etc.). But, does that list belong in the theme upgrade guide or in the PHPDoc for theme_links()? What ever happened to #12?

effulgentsia’s picture

Assigned: sun » Unassigned
Status: Needs work » Needs review
FileSize
3.32 KB

As per #38. Then, the theme upgrade guide can simply link to http://api.drupal.org/api/function/theme_links/7.

retester2010’s picture

#39: drupal.theme-links.39.patch queued for re-testing.

sun’s picture

My problem with this list is, regardless of where we put it, that it will be outdated very soon, and in general, is hard to maintain. We have (or hopefully "had"?) a similar list of all cache tables that Drupal core uses... still containing 4.7.x table names, since no one ever cared for that list buried in code.

Therefore, I'd rather suggest to create a regular d.o handbook page that can be edited and updated at any time by anyone. People could even add further items for the most popular modules, possibly.

Personally, however, I'd be much more interested in a clear + precise documentation about who is responsible for registering these sub-pattern-theme functions? I.e., if my module uses "links__mymodule_foo", do I have to register "links__mymodule_foo" in hook_theme()? If not, how can themers override this theme function? Do they have to register it + implement the master theme_links__mymodule_foo() function instead of THEME_links__mymodule_foo()? Or does the system register and/or look up that function automatically?

I've already played around with variations of this, but didn't come to a final conclusion for how exactly these sub-theme functions are actually working, and who's responsible for doing what. :)

sun’s picture

So AFAICT, a theme needs to implement hook_theme_registry_alter() in order to copy the original 'functionname' into 'functionname__FOO__BAR' along with the original properties, and manually replace the 'function' with aforementioned pattern function name, to be able to use it. That's quite a WTF, and I wonder whether it was the intention, and whether we could make this work a bit smarter. (Please correct me if I'm wrong!)

matglas86’s picture

Subscribe +

I'm looking forward to see this completely in action. Like @sun said it would be very necessary to have the documentation on this way of theming. I second the remark about function references in code. With good documentation is would speak for itself where to find them and in the book on theming it could be expended to help themers get around the concepts.

I still have a question. I came here when I was searching for a fix to problem I have but I'm not sure it this is what will fix it. What I mis is a way to have preprocessing on theme functions and not only on the theme templates. The templates have a preprocess function that can be called but I can't find a way to do a preprocess on a theme function. If I do a override it would require me to copy the whole function from it's source and change the things that are importent to my theme. Or is there a different way?

Thanks and I love D7 I'm gone love it as a themer.

jhodgdon’s picture

tagging for upgrade guide (see #38)

jhodgdon’s picture

Status: Needs review » Needs work
Issue tags: -Needs documentation updates

I added information to the theme update guide:
http://drupal.org/update/themes/6/7#theme-links-specifics

Leaving the issue open for generic "Needs Documentation", since it doesn't appear a decision has been made on how to maintain the doc for this change going forward.

Note: If we need to add to the drupal.org documentation somewhere, the thing to do is for someone either to do it directly or to file an issue in the Documentation issue queue.

David_Rothstein’s picture

This documentation and the examples above are out of date, since #878092: Regression from D7 alpha: themes are unable to render one group of node links separately from another (and maybe other issues too) changed the pattern for node links sometime after this issue was finished.

We now have 'links__node__blog' rather than 'links__blog_node' in the case of node links. I tried updating both the module and theme documentation to address this:

http://drupal.org/node/224333/revisions/view/1534368/1535596
http://drupal.org/node/254940/revisions/view/1534498/1535584

It is worth a review.

For the theme update guide, I left 'links__node__MODULE' out of the example list entirely, as it's a significantly more advanced use case (that theme override function won't even get called unless you specifically render the module's links separately from the others in node.tpl.php), so I just used 'links__node' instead which encompasses all the node links. But I did try to mention both in the module update guide, since module authors all need to follow this pattern.

jhodgdon’s picture

The changes look reasonable to me. This issue is still open for regular documentation (as opposed to update documentation).

netsensei’s picture

Title: theme_links() is not really themeable » m is not really themeable
FileSize
486 bytes

So I use D7.9, tried to override theme_links__locale_block in my own template.php. But it doesn't work, although the theme suggestion is implemented in the locale_block_view and the documentation on d.o. says it should work.

Went through the patches in this thread and compared them with the D7.9 and D7.x codebases. I've noticed that 'pattern' => 'links__' went AWOL somewhere along the line. As soon as I added it in common.inc, the suggestions started to work.

Couldn't find another thread where someone matches this, I've attached a patch.
I couldn't find an specific test for this piece of functionality either. Neither in theme.test or common.test. Maybe that's why it slipped through?

Can someone confirm this?

netsensei’s picture

Title: m is not really themeable » theme_links() is not really themeable

Accidental typo in the title of the issue. :(

  • webchick committed 85c680a on 8.3.x
    #588148 by JohnAlbin, sun, and effulgentsia: Make theme_links() actually...

  • webchick committed 85c680a on 8.3.x
    #588148 by JohnAlbin, sun, and effulgentsia: Make theme_links() actually...

  • webchick committed 85c680a on 8.4.x
    #588148 by JohnAlbin, sun, and effulgentsia: Make theme_links() actually...

  • webchick committed 85c680a on 8.4.x
    #588148 by JohnAlbin, sun, and effulgentsia: Make theme_links() actually...

  • webchick committed 85c680a on 9.1.x
    #588148 by JohnAlbin, sun, and effulgentsia: Make theme_links() actually...