When viewing a comment preview, the preview node content has wrong extra content. The node field and any other fields is displayed in a wrong way.
See attachment below.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

peximo’s picture

FileSize
653 bytes

comment_preview() attach the builded node to the comment form:

    $build = node_build($node);
  }

  $form['comment_output_below'] = $build;
  $form['comment_output_below']['#weight'] = 100;

The form_builder() function attach to all node fields some form properties (eg. #parents, #array_parents etc.) because comment_output_below is treated as common form elements. So drupal_render() function print this properties.
I don't know if is a drupal_render() bug, but one solution is to mark the $build form container as markup and assign to this element the rendered $build.

peximo’s picture

Status: Active » Needs review
plach’s picture

Priority: Normal » Critical

This seems a pretty severe bug.

yched’s picture

The fix seems correct, but we should probably do

$form['comment_output_below'] = array(
  '#markup' => drupal_render($build),
  '#weight' => 100,
);
moshe weitzman’s picture

Status: Needs review » Needs work

We worked hard to remove these premature drupal_render() calls. I would much prefer that this stuff gets rendered along with everything else during drupal_render_page().

yched’s picture

moshe: true, but the problem here is that the "render array inside a form array" gets massaged by form builder, which seems to produce odd results...

peximo’s picture

Status: Needs work » Needs review
FileSize
1.21 KB

To avoid "render array inside a form array" problem one solution is to introduce a new form element type, like 'build', that is not processed by the form_builder() function. But I don't know if this is a right approach and if we should have a generalized approach.
The following patch introduce this type and change the $form['comment_output_below'] element.

Damien Tournoud’s picture

Status: Needs review » Needs work

@plach found out that the culprit is theme('field'). field.tpl.php expects to receive a $items of sub-elements to display, but instead it is passed a renderable array (ie. something that can legitimately contain additional #properties).

We could fix that in two places:

- either by adding a element_children() to field.tpl.php
- or by filtering out the properties in template_preprocess_field().

I have not looked at this further, but it seems that the structure received by theme('field') is unnecessarily complicated:

array(
  '#object' => ,
  '#build_mode' => ,
  '#title' => ,
  '#label_display' => ,
  '#field_name' => ,
  'items' => array(
    // items.
  ),
);

Why not storing the items directly as children of the field?

yched’s picture

Component: comment.module » field system
Status: Needs work » Needs review
FileSize
1.29 KB

Nice catch. I guess this broke in #545662: Simplify field rendering, we didn't check comment preview (node previews are attached as a fully rendered #prefix).
Attached patch should fix this.

Why not storing the items directly as children of the field?

Wouldn't work for 'multiple formatters' (= 1 formatter call handles the display of all field values). In this case, $element['#theme'] = 'field', and $element['items']['#theme'] = 'my_formatter', so we need this intermediate layer.

yched’s picture

Testing this let me notice: #632188: Notices on comment preview when node has terms (happens with or without the patch in #9).

peximo’s picture

@yched: what about something like this?

<?php if ($items) : ?>
  <div class="field <?php print $classes; ?> clearfix"<?php print $attributes; ?>>
    <?php if (!$label_hidden) : ?>
      <div class="field-label"<?php print $title_attributes; ?>><?php print $label ?>:&nbsp;</div>
    <?php endif; ?>
    <div class="field-items"<?php print $content_attributes; ?>>
      <?php print render($items); ?>
    </div>
  </div>
<?php endif; ?>

And then have a separate template for the single item?

yched’s picture

@peximo: because looping over $items in the template rather than by relying on drupal_render() lets themers easily change the default "successive divs" display of multiple values, in favor of ul, ol, comma-separated etc... That would be fairly difficult with the code you propose.

plach’s picture

Status: Needs review » Needs work

Excepted a few typos the patch looks good to me.

+++ modules/field/field.module	13 Nov 2009 18:05:06 -0000
@@ -680,9 +680,17 @@ function template_preprocess_field(&$var
+    // Filter out non-children properies that might have been added if the
+    // render sttructure has been through form_builder().

"Filter out non-children properties that might have been added if the renderable structure has been through form_builder()."

I'm on crack. Are you, too?

yched’s picture

Status: Needs work » Needs review
FileSize
1.29 KB

Indeed. Updated patch fixes that.

plach’s picture

+++ modules/field/field.module	13 Nov 2009 18:05:06 -0000
@@ -680,9 +680,17 @@ function template_preprocess_field(&$var
+    // Filter out non-children properies that might have been added if the

"properies" :(

I'm on crack. Are you, too?

yched’s picture

FileSize
1.29 KB

Gosh :-).

plach’s picture

Status: Needs review » Reviewed & tested by the community

I suppose the bot should be happy too.

webchick’s picture

Is it possible to write tests for this?

yched’s picture

@webchick: not really IMO. We'd need to test that nothing is displayed besides the expected value. Awkward.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yeah, that's kinda what I thought too.

Committed to HEAD.

Status: Fixed » Closed (fixed)

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