Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#16 | field_render_in_form.patch | 1.29 KB | yched |
#14 | field_render_in_form.patch | 1.29 KB | yched |
#9 | field_render_in_form.patch | 1.29 KB | yched |
#7 | comment_preview-623314-2.patch | 1.21 KB | peximo |
#1 | comment_preview-623314-1.patch | 653 bytes | peximo |
Comments
Comment #1
peximo CreditAttribution: peximo commentedcomment_preview() attach the builded node to the comment form:
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.
Comment #2
peximo CreditAttribution: peximo commentedComment #3
plachThis seems a pretty severe bug.
Comment #4
yched CreditAttribution: yched commentedThe fix seems correct, but we should probably do
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commentedWe 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().
Comment #6
yched CreditAttribution: yched commentedmoshe: 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...
Comment #7
peximo CreditAttribution: peximo commentedTo 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.
Comment #8
Damien Tournoud CreditAttribution: Damien Tournoud commented@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:
Why not storing the items directly as children of the field?
Comment #9
yched CreditAttribution: yched commentedNice 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.
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.Comment #10
yched CreditAttribution: yched commentedTesting this let me notice: #632188: Notices on comment preview when node has terms (happens with or without the patch in #9).
Comment #11
peximo CreditAttribution: peximo commented@yched: what about something like this?
And then have a separate template for the single item?
Comment #12
yched CreditAttribution: yched commented@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.
Comment #13
plachExcepted a few typos the patch looks good to me.
"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?
Comment #14
yched CreditAttribution: yched commentedIndeed. Updated patch fixes that.
Comment #15
plach"properies" :(
I'm on crack. Are you, too?
Comment #16
yched CreditAttribution: yched commentedGosh :-).
Comment #17
plachI suppose the bot should be happy too.
Comment #18
webchickIs it possible to write tests for this?
Comment #19
yched CreditAttribution: yched commented@webchick: not really IMO. We'd need to test that nothing is displayed besides the expected value. Awkward.
Comment #20
webchickYeah, that's kinda what I thought too.
Committed to HEAD.