While writing my Color Picker sample field module, I found myself wondering what value hook_field_formatter_info() provides that could not also be provided by additional data in hook_theme().

/**
 * Implement hook_field_formatter_info().
 */
function colorpicker_field_formatter_info() {
  return array(
    'colorpicker_helloworld' => array(
      'label' => t('"Hello, World" in color'),
      'field types' => array('colorpicker_rgb'),
    ),
  );
}

/**
 * Implement hook_theme().
 */
function colorpicker_theme() {
  return array(
    'field_formatter_colorpicker_helloworld' => array(
      'arguments' => array('element' => NULL),
    ),      
  );
}
CommentFileSizeAuthor
#10 field-formatter-hook-theme-567092-10.patch7.75 KBbjaspan
#6 field_formatter_hook_theme-567092-6.patch6.58 KByched
#5 field_formatter_theming-1.patch5.57 KBrcross
#3 field_formatter_theming.patch1.28 KBrcross

Comments

yched’s picture

yched commented

The list of formatter settings doesn't really belong in hook_theme(), I guess ?

I'd tend to think the other way around :
given hook_field_formatter_info(), field.module should be able to take care of exposing the corresponding hook_theme() information in field_theme(), saving the need for field type modules to do it themselves.

rcross’s picture

rcross commented
Assigned: Unassigned » rcross

i'm on it!

rcross’s picture

rcross commented
Status: Active » Needs review
StatusFileSize
new field_formatter_theming.patch1.28 KB

Here is a start. I hope this works.

Status: Needs review » Needs work

The last submitted patch failed testing.

rcross’s picture

rcross commented
Status: Needs work » Needs review
StatusFileSize
new field_formatter_theming-1.patch5.57 KB

after a few discussions, this should improve things but seems to have a small error on my dev machine that i can't pinpoint.

yched’s picture

yched commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new field_formatter_hook_theme-567092-6.patch6.58 KB

Updated patch just adds a few comments. Other than that, RTBC.

bjaspan’s picture

bjaspan commented

I'm fine with the approach of having hook_field_formatter_info() generate hook_theme() results instead of the other way around, so long as we do not have to specify the same info twice.

Question: I'm not a hook_theme guru. Is there any possible Field API formatter that might want to generate its own hook_theme output instead of what Field API provides for it? If so, then we should probably add an optional #tag that hook_field_formatter_info() can return for any given formatter to say "don't export hook_theme() for this one, I'll take care of it." A DEFAULT/CUSTOM behavior switch like we already have for other things, basically.

bjaspan’s picture

bjaspan commented
Status: Reviewed & tested by the community » Needs review

CNR until the question in #7 is answered.

yched’s picture

yched commented

bjaspan: number.module has this use case of needing to alter the 'default' theme info (2 formatters use the same theme function), and the patch does this using hook_theme_registry_alter().
That stays inline with the existing tools and hooks around the theme registry, I'm not sure we need to add a new way to specify theme informations, specific to formatters.

bjaspan’s picture

bjaspan commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new field-formatter-hook-theme-567092-10.patch7.75 KB

I added some PHPdoc I thought was important; no code changes.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Reviewed. Looks good, so committed.

Status: Fixed » Closed (fixed)

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