Merge hook_field_formatter_info and hook_theme()? [#567092]

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),
    ),      
  );
}

Comment	File	Size	Author
#10	field-formatter-hook-theme-567092-10.patch	7.75 KB	bjaspan
#10
#6	field_formatter_hook_theme-567092-6.patch	6.58 KB	yched
#6
#5	field_formatter_theming-1.patch	5.57 KB	rcross
#5
#3	field_formatter_theming.patch	1.28 KB	rcross
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

yched CreditAttribution: yched commented 5 September 2009 at 10:08

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.

Comment #2

rcross CreditAttribution: rcross commented 5 September 2009 at 14:34

Assigned:

Unassigned

» rcross

i'm on it!

Comment #3

rcross CreditAttribution: rcross commented 5 September 2009 at 15:11

Status:

Active

» Needs review

File	Size
field_formatter_theming.patch	1.28 KB

Here is a start. I hope this works.

Comment #4

5 September 2009 at 15:31

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #5

rcross CreditAttribution: rcross commented 5 September 2009 at 16:09

Status:

Needs work

» Needs review

File	Size
field_formatter_theming-1.patch	5.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.

Comment #6

yched CreditAttribution: yched commented 6 September 2009 at 15:23

Status:

Needs review

» Reviewed & tested by the community

File	Size
field_formatter_hook_theme-567092-6.patch	6.58 KB

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

Comment #7

bjaspan CreditAttribution: bjaspan commented 7 September 2009 at 15:53

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.

Comment #8

bjaspan CreditAttribution: bjaspan commented 7 September 2009 at 15:53

Status:

Reviewed & tested by the community

» Needs review

CNR until the question in #7 is answered.

Comment #9

yched CreditAttribution: yched commented 7 September 2009 at 16:24

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.