Simplify field rendering

No time for a proper review right now, but more eyes on this part of the code are highly welcome, thanks moshe !

This being said, I'm a little worried that adding one template call per *value* is going to be fairly expensive. That's originally why we kept formatters as theme functions instead of templates (although back then we could have used the preprocessing for the 'sanitize' step)

Also: FieldAttachTestCase explodes on my local setup too. When I need to debug something in there I rename a couple tests to focus on the ones I want. Tedious :-(. For the current patch I'd suggest testFieldAttachViewAndPreprocess()...

+1 on "would be great if pifr gave more detail about failures". We should probably also split FieldAttachTestCase, too...

Rerolled after #361839: Help needed for some field_attach function docs + added back field-item.tpl.php, missing from #7.

Not able to help on the tests tonight, my WAMP stack refuses to boot...

The cleanup in template_preprocess_field() is awesome. I'm not sure how 'multiple value' formatters work with the new code - maybe those are the test failures ? There's currently no example of a multiple formatter in core, aside from 'field_test_multiple' in field_test.module, used in testFieldAttachViewAndPreprocess().

I'm still worried about the additional template, and the fact that it possibly needs to support the same template suggestions than field.tpl.php (-fieldname.tpl.php, -bundle.tpl.php, -fieldname-bundle.tpl.php).

Then I thought : the ugliest part in field.tpl.php right now is the way we support 'label inline':

Label: Value 1
       Value 2
As opposed to 'label above':
Label: 
Value 1
Value 2

HTML for 'label' inline is the following, and thus bleeds in the 'display items' part:

<div class="field">
  <div class="field-items">
    <div class="field-item">
      <div class="field-label-inline-first">Label:&nbsp;</div>Value 1
    </div>
    <div class="field-item">
      <div class="field-label-inline" **hidden by CSS, just provides vertical alignment of values**>Label:&nbsp;</div>Value 2
    </div>
  </div>
</div>

There is probably a cleaner way to provide the vertical alignment of values at the right of the label, using floated divs or whatever. This would allow us to keep a common structure for the markup in both 'label display' modes, leaving the layout differences to CSS:

<div class="field">
  <div class="field-label">Label:&nbsp;</div>
  <div class="field-items">
    <div class="field-item">Value 1</div>
    <div class="field-item">Value 2</div>
  </div>
</div>

If so, field-item.tpl.php doesn't have to mess with label at all, just display the items alone. It becomes so simple it can just be a regular theme function. No additional template overhead.

I think this even opens the way to dead-simple 'multiple-values display modes' : as divs (like currently), as ul, ol, as comma separated - just override the theme function... w00t !
Breaks many D6 custom themes and CSS, probably - but very much worth it IMO.

Hm. I guess we should ask someone with CSS mojo :-)

Question is: is there a way to have this markup:

<div class="field">
  <div class="field-label">Some string:&nbsp;</div>
  <div class="field-items">
    <div class="field-item">Value 1</div>
    <div class="field-item">Value 2</div>
    <div class="field-item">Value 3</div>
  </div>
</div>

styled as:

Some string: Value 1
             Value 2
             Value 3

with vertical alignment of "Value x" whatever the length of "Some string"

A quick try with firebug shows this seems to work :

<div class="field clear-block">
  <div style="float: left;" class="field-label">Some string:&nbsp;</div>
  <div style="float: left;" class="field-items">
    <div class="field-item">Value 1</div>
    <div class="field-item">Value 2</div>
    <div class="field-item">Value 3</div>
  </div>
</div>

Probably needs to be cross-browser tested, though. We still officially support IE6 ?

crosspost, tags are welcome

My CSS is a bit rusty, but:

I do not see where css class clear-block is defined. Assuming clear-block does nothing (which I doubt), the markup above will leave the outer

Yes, that class was clear-block in D6 and has been renamed clearfix in D7. It uses :after for modern browsers and (I think) inline-block for IE6, so I *think* it works. Could welcome confirmation, though.

Tested snippet #14 to work on IE6, IE8 (didn't try IE7 but I'm not really worried), Safari, Chrome, Opera (but I was not worried about those).
[edit: and Firefox, of course]

So it looks like the path is clear :-)

Work-in progress patch.

Simplified to the HTML in #14, still TODO: add the CSS, turn field-item.tpl.php into a theme function.

Er, without the commented out tests. The patch should also fix test failures for 'multiple' formatter.
CNR just to check that.

I'll be mainly AFK for the next 3-4 days, so anyone is welcome to take this further meanwhile ;-)

Added the CSS, turned field-item.tpl into a theme function.

Forgot one documentation TODO

And forgot to remove field-item.tpl.php...

Let's wait a little, I'm mulling over alternatives around this TODO.

The idea is :

- the current theme_field_item() (or template in earlier iterations) is not really interesting at this level. Theming the whole list of items would be much more powerful to allow 'output values as divs, ul, ol, comma-separated...', and would not add much complexity in the theme func (mainly a foreach).
theme_field_itemS() becomes the #theme of the field array, and field.tpl.php is the #theme_wrapper.

- I don't think we want to go through the hassle of formalizing the notion of 'multiple values display mode (div, ul, ol, char-separated... like we have for 'label display (inline, above...)' (which we keep for legacy reasons), and reflect that in the UI for display settings. This can be purely theme-override stuff.
After the simplification of the overall html in field.tpl.php, I'm wondering if this 'display items' thing deserves a separate theme call at all. Could all happen in field.tpl.php. You want values for field_foo as comma separated ? Easy, override field-field_foo.tpl.php and do it over there.

So, patch #26 works, I'll try to provide an alternative for the above later today.

Bumping down to needs work.

So, alternate proposal, as explained in #27.

Gets back to everything inside field.tpl.php. Not a complete roundtrip, in the process:
- the HTML got much simpler to override to do stuff like 'display the values of field_foo as a ul, ol or custom-char-separated list'.
Before:

<?php if (!$field_empty) : ?>
<div class="field field-type-<?php print $field_type_css ?> field-<?php print $field_name_css ?>">
  <?php if ($label_display == 'above') : ?>
    <div class="field-label"><?php print t($label) ?>:&nbsp;</div>
  <?php endif;?>
  <div class="field-items">
    <?php $count = 1;
    foreach ($items as $delta => $item) :
      if (!$item['empty']) : ?>
        <div class="field-item <?php print ($count % 2 ? 'odd' : 'even') ?>">
          <?php if ($label_display == 'inline') { ?>
            <div class="field-label-inline<?php print($delta ? '' : '-first')?>">
              <?php print t($label) ?>:&nbsp;</div>
          <?php } ?>
          <?php print $item['view'] ?>
        </div>
      <?php $count++;
      endif;
    endforeach;?>
  </div>
</div>
<?php endif; ?>

After:

<?php if ($items) : ?>
  <div class="field <?php print $classes; ?> clearfix">
    <?php if (!$label_hidden) : ?>
      <div class="field-label"><?php print $label ?>:&nbsp;</div>
    <?php endif; ?>
    <div class="field-items">
      <?php foreach ($items as $delta => $item) : ?>
        <div class="field-item <?php print $delta % 2 ? 'odd' : 'even'; ?>"><?php print render($item); ?></div>
      <?php endforeach; ?>
    </div>
  </div>
<?php endif; ?>

- template_preprocess_field() got terribly streamlined.

There could be additional cleanup/reorganizing in the field.tpl.php documentation, I'll do that if this approach is agreed.

Done additional docs cleanup, leaving RTBC.

Reroll after Translatable fields (after having to be rerolled so many times, TF patch gets his revenge by forcing every other Field patch to reroll...)
CNR for the bot, then RTBC.

re-roll...

Indeed, forgot one template variable in the reroll. That one should be better.

Sneaking an additional cleanup : field.css now attached in #attached_css on forms and displayed fields (instead of in field_init(), yuck)

I don't get this. 'Multiple formatters' are screwed, and there's now way this is related to the #attached_css change in #40.
Actually, I also get the failure locally with #39, which came back green. Reposting #39 for a re-test.

Really strange. I can't see anything in the recent commits that would cause #39 to raise failures while it came back green 2 hours ago.
Anyway. That's all for today :-/

Gah ! Testbot didn't run 'Field attach' tests on #39: http://testing.drupal.org/pifr/file/1/fieldrender-545662-39.patch

Anyone is able to submit a patch that badly breaks Field API and get it committed :-p.
Very possibly FieldAttachTestCase being too crowded and exploding some slaves... Should be split up, but not by me tonight :-/

moshe, there's no menu_rebuild() in field.module anymore : #164 / #171 in #516138: Move CCK HEAD in core.
Back to needs work, BTW.

This should unbreak 'multiple formatters'.

If the formatter is multiple (typical example is: dots on a computed png), the field template only sees one 'item', which will be rendered into the 'all values' output.

Stupid typo.

Green at last. Back to RTBC.

Committed to CVS HEAD. Thanks.