Field Type Template Suggestions

Here testbot ... here boy

For reference, this came up working on Bartik discussing taxonomy links #777840: Taxonomy terms should display inline

From IRC:

[23:41]  <jensimmons> webchick: Is http://drupal.org/node/784792 something that could happen now? Or is it DOA?
[23:41]  <webchick> jensimmons, That looks reeeeeeeeeeally iffy.
[23:42]  <jensimmons> it'd just be nice to know, since figuring out what to do about http://drupal.org/node/777840 depends on it
[23:42]  <Druplicon> http://drupal.org/node/777840 => Taxonomy terms should display inline => Bartik, User interface, normal, active, 7 comments, 1 IRC mention
[23:42]  <webchick> Yeah. Why aren't we just changing the markup for taxonomy to <li>s?
[23:42]  <jensimmons> i don't know
[23:42]  <jensimmons> well  because it's not in the theme....
[23:42]  <webchick> To me that would feel more like fixing a regression, where this is clearly a new feature, and has performance implications.
[23:43]  <jensimmons> ok. would you say so on the issue?
[23:43]  <jensimmons> then we can kill the effort to go down that road
[23:43]  <webchick> Sure, but I'd prefer to hear first from some of the theme system maintainers and get their view.
[23:43]  <jensimmons> ok

This is one of the things that annoys me most about the current default core template files.

Suggestions went in kind of late, and it seems like a bug to me that it's not possible rather than a feature request, but I don't know what the performance implications are.

These terms should be output as a list by default, and generally there should be an easy way to do a type override that is inline with how the rest of templating works in Drupal. It's not just taxonomy terms we are talking about here... I can think of endless use cases where the ability to easily target a field type for theming makes sense. Different types of data usually need different markup/presentation.

So... +1.

Sorry, cross-posted... adding the Bartik tag back.

+1

We have a regression here: taxonomy terms should be displayed as an inline UL and currently they are not.

So, once again, why are we not just fixing the regression with taxonomy's output?

Edit: Er. I mean I get that this would be a nice thing to have, but it's unclear to me why a solution like this is needed to fix taxonomy. It seems like you're trying to hack around sub-optimal output, so let's just fix the output.

As far as performance stuff goes, probably people like catch, effulgentsia, etc. would be good folks to talk to. They did endless amounts of belly-dancing to get field templates to perform at an acceptable level.

This still smells featurey to me.

All I see is the following:

THEMENAME_field__body__article()
THEMENAME_field__article()
THEMENAME_field__body()
THEMENAME_field()

http://api.drupal.org/api/function/theme_field/7

In the above example, article is the content type, and body is the field.

Personally, I would be a lot less likely to theme fields by content type or name, over field type. This is especially true for taxonomy terms, where there are likely a few per site at minimum. Other examples would be fields like phone & fax numbers, dates, numbers/currency, where there are more than one actual field (different names), but which will require the same output. Core provides 2 taxonomy vocabs (tags and forums).

Without the ability to target the field type, I would be inclined to make a ridiculous theme_field() override just to avoid going after each taxonomy field by name to do the same thing with the markup. Why create 3 template files containing the same exact code? It's not very efficient. Still you need to understand theme functions to even consider this.

Even if it's decided that this is a "feature request" I would love to see how it's going to be done, because I don't see an easy way right now, so maybe I'm missing something too.

I agree 100%. I don't understand the objection either.

As far as performance stuff goes, probably people like catch, effulgentsia, etc. would be good folks to talk to. They did endless amounts of belly-dancing to get field templates to perform at an acceptable level.

We are not adding a template though. We are simply asking to add a suggestion for #field_type. I don't get it. I'll try to find catch or effulgentsia on IRC and ask them.

Ok, I couldn't get catch, but I asked merlinofchaos, and he showed me how to test it myself. I tested this on the homepage of my test site, which has 10 nodes on it. I hacked drupal_render() and did the following:

  // Call the element's #theme function if it is set. Then any children of the
  // element have to be rendered there.
  if (isset($elements['#theme'])) {
    // Start hack
    if ($elements['#theme'] == 'field') {
      timer_start('field_test');
      foreach (range(1, 1000) as $count) {
        $elements['#children'] = theme($elements['#theme'], $elements);
      }
      $time = timer_stop('field_test');
      dsm($time['time'] / 1000 . ' s');
    }
    else {
      $elements['#children'] = theme($elements['#theme'], $elements);
    }
    // End hack
  }

These were the results:

Before

0.12404 s
0.47532 s
0.98916 s
1.12415 s
1.64731 s
1.78298 s
1.92098 s
2.05935 s
2.19844 s
2.3373 s
2.47671 s
2.61559 s
2.7545 s

After

0.12473 s
0.4762 s
0.99204 s
1.12848 s
1.6549 s
1.79142 s
1.92889 s
2.06738 s
2.20873 s
2.34799 s
2.48726 s
2.62643 s
2.76581 s

There is a performance impact, but it's very small and I think we really need this. The current suggestions are really not very useful, or consistent, and I'd be willing to bet that what themes end up doing to get around this will be considerably worse in the long run.

Here's the patch again, since something was wrong with the format of the other one.

The patch looks good, and based on Jacine's AB test result I dont think there is a significant performance impact here. So.... RTBC

I still need to see sign-off on this from one of the people who worked on the original field templates patch. This is also very late, but the case has been made to at least consider this in D7.

Those numbers do indeed look like there's a fairly small performance impact, although I can't quite tell if they're accurate in a more typical "real world" scenario where we're showing a list of 10 nodes with ~10 fields each.

Won't the one extra filename to search for only have an impact when the theme registry is being refreshed?

I think we are trying to optimize based on a false assumption that most themes will not attempt to modify theme_field() or actually use these suggestions in some way shape or form. The taxonomy term example (needing to be a list) is just one example. theme_field()'s markup doesn't make sense in many situations, and themers WILL change it. The suggestions as they are now promote idiotic theming behavior. It's like saying, we're not going to allow you to theme nodes by node--type, but instead you need to do node--id for every node you want to style which is insane.

Since template files are rampant in Drupal these days, many themers don't think twice about adding them to themes, let alone have any idea there is any performance impact at all. By not adding the type suggestion (which is the only sane way to theme fields IMO), we are telling themes that it's okay to create templates for every single field name, and it's really not. If it wasn't a big deal, then we would even be talking about performance right now.

Also, @joachim brings up a very good question. That's what I thought, so I would like some clarification on this too:

Won't the one extra filename to search for only have an impact when the theme registry is being refreshed?

But, webchick is right, at this point, what we need catch or effulgentsia to weigh in on this.

There is miniscule performance impact for this patch. It can be up to 100 more file_exists() calls on a page with many fields displaying but that file_exists() is cached by the OS anyway. I think it can proceed. Would still be good to hear from catch or alex.

There is a general truism that templates are more expensive than theme functions but thats unrelated.

This patch just makes sense. There is no real performance impact.

That said, if we'd really care for performance, then the theme system should cache theme hook suggestions results. That would only make sense, because everything else in the theme system is equally cached already. I wouldn't have remotely assumed that you can simply put a template file somewhere, anywhere, and expect that to work immediately. Likewise, you need to flush the theme registry cache(s), after changing a theme's .info file to add a CSS file or do whatnot.

What I'm trying to say is: If we're concerned about performance, then we should attempt the real, underlying performance issue seriously, instead of adding burden on themers.

Template discovery performance was massively improved already due to #678714: Unify use of theme hook / template suggestions, fix clobbering problems, and improve suggestion discovery performance which removed all the runtime file_exists() calls, so I don't have any problem with this patch, didn't profile it but I think it'd more or less just make a couple of foreach() loops a bit longer, and that's nothing compared to the rest of what's going on here.

If people want to open D8 issues for removing the bundle suggestion that seems fine though - no point doing work for no reason.

Cool, thanks catch. I couldn't remember the details, I just remember there being a lot of teeth gnashing and crying and whatnot. I'm glad Jacine and Jeff Burnz also posted benchmarks to back this up, thanks.

I'm going to allow this in. It's technically an API change (more properly, API addition) after beta, but I think the argument has been well-made that the original field templates that were designed by the field team didn't take into account the reality of what themers would need from the system. This gives a lot more flexibility with one line of code and minimal performance impact.

So, committed to HEAD. :)

[edit] Whoops, posted in the wrong issue. Sorry friends.