The comment_wrapper.tpl.php file does not provide logic to prevent outputting an empty wrapping div when no comments actually exist:

<div id="comments">
  <?php print $content; ?>
</div>

I am filing this as a support request because there may be a valid use-case that needs to be documented. If there is a good reason to output an empty div, please change this issue to the documentation category so we can find a place to explain this behavior.

This template's 5.x predecessor, theme_comment_wrapper(), also lacked logic. Here's an issue from the 4.7.x days that appears similar: [60589].

CommentFileSizeAuthor
#26 comment.patch354 bytesjmaties
#21 comment-wrapper-is-empty-372414-21.patch865 bytesnetsensei
#21 Picture 3.png23.04 KBnetsensei
#18 comment-wrapper-is-empty-372414-18-d6.patch691 bytesJohnAlbin
#16 comment-wrapper-is-empty-372414-16.patch876 bytesJohnAlbin
#13 comment-wrapper-is-empty-372414-13.patch877 bytesJohnAlbin
#8 comment-wrapper_logic_02.patch614 bytesainigma32
#5 comment-wrapper_logic_reroll.patch391 bytesTodd Nienkerk
#1 comment-wrapper_logic.patch354 bytesTodd Nienkerk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Todd Nienkerk’s picture

Todd Nienkerk CreditAttribution: Todd Nienkerk commented
Title: comment_wrapper.tpl.php outputs empty div when no comments exist » comment-wrapper.tpl.php outputs empty div when no comments exist
FileSize
comment-wrapper_logic.patch354 bytes

Patch attached.

ainigma32’s picture

ainigma32 CreditAttribution: ainigma32 commented
Status: Active » Needs review

Makes sense to me. Will try applying the patch when I get home.

- Arie

ainigma32’s picture

ainigma32 CreditAttribution: ainigma32 commented
Status: Needs review » Needs work

Patch doesn't apply. You need to create the patch from the root of Drupal.

- Arie

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented

subscribe.

Todd Nienkerk’s picture

Todd Nienkerk CreditAttribution: Todd Nienkerk commented
Version: 6.9 » 6.10
FileSize
comment-wrapper_logic_reroll.patch391 bytes

Rerolled patch to apply to root.

ainigma32’s picture

ainigma32 CreditAttribution: ainigma32 commented

Well after this issue you're going to be start patch-maker :-)
Still does not apply. Why is there a mention of modules/comment/comment-wrapper.tpl.php.orig.php ?

Could you try one more time?

- Arie

Todd Nienkerk’s picture

Todd Nienkerk CreditAttribution: Todd Nienkerk commented

ainigma: Thanks for bearing with me here. I've been writing patches for modules for months, but I've rarely done anything with core.

The mention of modules/comment/comment-wrapper.tpl.php.orig.php is the original comment-wrapper.tpl file. (The extra .php extension is annoying. I'm trying to do this while attending Drupalcon sessions, so I just ignored it.)

I used diff -up original.php new.php > patch.patch to create the patch. Should I be doing something differently, like cvs diff from a checkout? I don't normally mind CVS, but I'd rather avoid it if possible because I've never checked out core.

Again, sorry for the elementary how-to stuff. I thought I was far beyond this.

ainigma32’s picture

ainigma32 CreditAttribution: ainigma32 commented
FileSize
comment-wrapper_logic_02.patch614 bytes

No problem.

Personally I use Eclipse to create patches. I checkout Drupal using CVS and just create a patch using the UI.
So basically I'm just a mouse monkey clicking away :-D
What is important is that you use the Unified format (command line -u I think) and that you choose the drupal root as the root of the patch root. Of course using the UI I can easily select which changes I want to add to the patch and which I want to ignore. I suppose that's a little trickier using the commandline.

Result attached.

- Arie

Hope Murphy's law won't kick in now ;-)

ainigma32’s picture

ainigma32 CreditAttribution: ainigma32 commented
Status: Needs work » Needs review

Hmmm thought I set this...

ainigma32’s picture

ainigma32 CreditAttribution: ainigma32 commented

@Tod Nienkerk: Since I rolled the last one maybe you can test and review (and set to RTBC if it checks out) ?

- Arie

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Version: 6.10 » 7.x-dev
Category: support » bug
Issue tags: +theme, +tpl-refresh

It looks RTBC, but we'll need to apply it to HEAD first. So let's have the testbot have a go at it. Then I'll mark it RTBC. :-)

[edit: Oh! And after that, we can apply it to DRUPAL-6, of course!]

Dries’s picture

Dries CreditAttribution: Dries commented

I wonder if this is the right approach. Why do we even bother calling the theme system when there is no comment. Sounds like we're checking this at the wrong level. Should this check live somewhere in comment.module?

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
FileSize
comment-wrapper-is-empty-372414-13.patch877 bytes

@Dries: very. good. point.

Here's a completely untested patch that will only call the theme function if there is content to wrap.

Todd Nienkerk’s picture

Todd Nienkerk CreditAttribution: Todd Nienkerk commented

@JohnAlbin: I'm about to hop on a plane, so I can't brainstorm this at the moment... But can you think of any use cases where you would want to call the wrapper even if there were no comments? I suspect there may be some legitimate reasons.

Status: Needs review » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Status: Needs work » Needs review
FileSize
comment-wrapper-is-empty-372414-16.patch876 bytes

Hmmm… with this patch applied, I was able to install HEAD and all the comments simple tests past. I have no clue why the patch "failed".

Re-rolled for no good reason.

Dries’s picture

Dries CreditAttribution: Dries commented
Version: 7.x-dev » 6.x-dev
Status: Needs review » Reviewed & tested by the community

I ran the tests on my machine and everything is fine. Committed to CVS HEAD, and setting the version to Drupal 6.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
FileSize
comment-wrapper-is-empty-372414-18-d6.patch691 bytes

Ok, I've re-rolled this patch for D6. Its a trivial re-roll, so I'm taking the liberty of leaving this as RTBC. :-D

I did verify the patch works on my D6 test system using Stark (for d6).

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to Drupal 6. Thanks! Marking this fix now.

Status: Fixed » Closed (fixed)

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

netsensei’s picture

netsensei CreditAttribution: netsensei commented
Version: 6.x-dev » 7.x-dev
Status: Closed (fixed) » Active
FileSize
Picture 3.png23.04 KB
comment-wrapper-is-empty-372414-21.patch865 bytes

Hm. My site is giving me this when there are no comments (screenshot attached)

'Reacties' is a h4 title inside comment-wrapper.tpl.php in my theme. Comments should be shown below. But there are no comments so it justs shows the h4 and goes on to render the comment form.

$content also contains the comment form box. So it can never really be empty (according to the patch). The code above the patch does that.

// If enabled, show new comment form if it's not already being displayed.
    $reply = arg(0) == 'comment' && arg(1) == 'reply';
    if (user_access('post comments') && node_comment_mode($nid) == COMMENT_NODE_READ_WRITE && (variable_get('comment_form_location_'. $node->type, COMMENT_FORM_SEPARATE_PAGE) == COMMENT_FORM_BELOW) && !$reply) {
      $output .= comment_form_box(array('nid' => $nid), t('Post new comment'));
    }

Instead of using $content, it would be better to use $num_rows since this variable is set to TRUE/FALSE depending on the presence of comments.

Attached to this comment my patch against HEAD.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Version: 7.x-dev » 6.x-dev
Status: Active » Closed (fixed)

@netsensei

Currently, the comment-wrapper.tpl.php doesn't have a title in it. It just has a single div which wraps the comments and the comment form. As such, the patch that was committed was sufficient to prevent an empty div from appearing on pages.

What you want is slightly different: 1. (possibly) have an H2 title in the comment-wrapper and 2.) not include that h2 title when there are no comments. I think that feature request actually dovetails nicely with this issue #517850: Update and polish default comment-wrapper.tpl.php and associated css, so you should head over there and leave this issue as closed.

dawansv’s picture

dawansv CreditAttribution: dawansv commented

Old story but just want to second Todd Nienkerk (#14) comment... I was actually wondering why comment-wrapper was not running when there is no comment... No I see that's because of this patch. Not a big deal but indeed they are legitimate situations where one might want to run comment-wrapper even if they are no comments...

jmaties’s picture

jmaties CreditAttribution: jmaties commented
Version: 6.x-dev » 7.x-dev
Status: Closed (fixed) » Active
FileSize
comment.patch354 bytes

Hack comment module ;)

  • Dries committed 562f0ae on 8.3.x 
    - Patch #372414 by Todd Nienkerk, JohnAlbin: don't output empty comments...

  • Dries committed 562f0ae on 8.3.x 
    - Patch #372414 by Todd Nienkerk, JohnAlbin: don't output empty comments...

  • Dries committed 562f0ae on 8.4.x 
    - Patch #372414 by Todd Nienkerk, JohnAlbin: don't output empty comments...

  • Dries committed 562f0ae on 8.4.x 
    - Patch #372414 by Todd Nienkerk, JohnAlbin: don't output empty comments...

  • Dries committed 562f0ae on 9.1.x 
    - Patch #372414 by Todd Nienkerk, JohnAlbin: don't output empty comments...