Update comment rendering

fyi, folded view is already dead #506218: Remove collapsed comments and tidy up settings, #520486: Remove comment-folded.tpl.php

Very likely a followup issue, but what about moving comment links into $content as it was done back in #339929: Move node links into $node->content ?

Bumping to critical, this is a blocker for #504666: Make comments fieldable

Nice work Moshe.
diffstat 2 files changed, 285 insertions(+), 313 deletions(-) always good to see.

- In comment_build_content():

+  // Build fields content.
+  $comment->content += field_attach_view('comment', $comment, $build_mode);

I guess you forgot to remove that when splitting out from #504666: Make comments fieldable ?

- Shouldn't we also move comment.tpl.php to use render() ? When comments are fieldable, they'll need the same granular themeing features than nodes...

- Side note, not for this patch: is it me, or current HEAD does *nothing* with the 'homepage' comment input (except checking it's valid) ?

Note that I do quite a few changes in comment_form() in #524652: Overhaul comment form and preview - didn't try to get rid of the $edit array, though.
Maybe we could keep the form changes over there ? I don't how much 'update comment rendering' ties with the comment form, though.

- Rerolled after #524652: Overhaul comment form and preview

- fixed 'Anonymous comments' failure.
The HTML for the opening <div id="comment" div has slightly changed (now has a class too), the test did not catch the new markup. Fixed the test.

- 'Comment interface' failures are caused by a change in the place where the <div class="indented"> tag is added.
HEAD: before the <a id="comment-1"/> anchor
With patch: after the anchor (which looks wrong IMO)
=> the regexp built by CommentHelperCase::commentExists() fails when $reply = TRUE.
Did not investigate a fix.

Also note that with the patch, the indentation divs are not closed before the 'post new comment' form, which thus gets the same indentation than the last comment displayed.

- The 'Comment paging' failures are also a matter of CommentHelperCase::commentExists() regexp, although I couldn't find why it fails, the comments there are not indented.
I stopped there, it's quite late here :-)

Other remarks:

- It seems that with the patch, the 'add new comment' form is not displayed if the node doesn't already have comments.

- comment_build()
+ // $comment = (object)$comment;
seems like this is on the way out, but we don't want to forget it.

subscribing, will need to refactor the RDF patches once this gets fixed

Looks very, very nice.

+    // the 'divs element instructs #prefix whether to add an indent div or
+    // close existing divs (a negative value).

Capital 'T', should probably be $divs instead of 'divs

+ * @return void
+ *   An array of comment objects, enriched with indentation information.

Do we do this in core? Most functions that alter by ref just do it in the phpdoc.

+ * @param $build_mode
+ *   Build mode, e.g. 'full', 'teaser'...
+ *
+ * @return
+ *   A structured array containing the individual elements
+ *   of the comment's content.

no extra line break before @return

+  // $comment = db_query('SELECT c.*, u.uid, u.name AS registered_name, u.data FROM {comment} c INNER JOIN {users} u ON c.uid = u.uid WHERE c.cid = :cid', array(':cid' => $cid))->fetchObject();
+  //   // TODO: move this to comment_load_multiple().
+  //   $comment = drupal_unpack($comment);
+  //   $comment->name = $comment->uid ? $comment->registered_name : $comment->name;

Looks like this can go?

Yay. I was planning to help on the tests tonight, but you beat me to it. My bad for the $output_bottom notices, reroll error :-(

Minor remarks in comment_preview():
- $comment_build = array(); is not needed, the variable is now only used in the if() branch where it gets defined.
- double linespacing towards the beginning of the function.
- I don't think $node->in_preview is right: the node displayed here is a real node from real db values, not a preview from form values.

There are still a couple HTML changes:

- On a post with no comments, current HEAD displays

...(end of the node)...
<div class="links">(node links)</div>
<div id="comments">
  <h2 class="comments">Comments</h2>
  <h2 class="title">Post new comment</h2>
  <div>
    <form ...>

With current patch we only have:

...(end of the node)...
<div class="links">(node links)</div>
<h2 class="title">Post new comment</h2>
<div>
  <form ...>

Which means we lack the 'blue gradient' delimitation between the node and the comment section, since it's styled on h2.comments.

- In HEAD, comment links are directly below the <div class="comment"> wrapping the whole comment.

<div class="comment comment-by-node-author comment-by-viewer odd">
  <div class="clearfix">
    <span class="submitted">...</span>
    <h3>Subject</h3>
    <div class="content">...</div>
  </div>
  <div class="links">...</div>
</div>

With current patch, comment links are within the <div class="content">:

<div class="comment comment-by-node-author comment-by-viewer odd">
  <div class="clearfix">
    <span class="submitted">...</span>
    <h3>Subject</h3>
    <div class="content">
      ...
      <ul class="links inline">...</ul>
    </div>
  </div>
</div>

Visually, this just translates into slightly less vertical space between comment text and links, no biggie.
Probably not intended though, since comment.tpl.php still corresponds to the 1st version.

Attached patch should take care of all remarks in #18 and #19.

About <h2 class="comments">Comments</h2>
I merged comment_node_view_thread() and comment_node_view_form() into comment_node_page_additions(), that also takes care of adding the heading if either comments or the comment form are displayed. Moshe, you might want to check if that sounds OK to you. node/%nid was the only place where those two functions were called.

About the location of links:
Solved this by introducing render() in comment.tpl.php. As said earlier, will be needed for fieldable comments anyway.

Better PHPdoc for comment_node_page_additions().

The <h2 class="comments">Comments</h2> was displayed at the bottom of the page, because of the #sorted property on comments threads.
Reworked the $node->content['comments'] generated by comment_node_page_additions() into three distinct sections:
$node->content['comments']['comments'] (the threaded comments)
$node->content['comments']['comment_form']
$node->content['comments']['prelude'] (the h2 heading)

Which brings us just a step away from a more flexible comment-wrapper.tpl.php, BTW...

I wonder how does these changes align with #517864: Update and polish default comment.tpl.php and associated css and #517850: Update and polish default comment-wrapper.tpl.php and associated css (especially the latter)?

+          $build['commment_parent'] = comment_build($comment);

(note the 3 'm' in commment_parent)
why did this patch pass all tests, is there no test for this line?

@kika: the patch doesn't really interfere. When one patch lands, the other will possibly need a slight reroll, but no fundamental change.
As I noted in #22, it should actually make #517850: Update and polish default comment-wrapper.tpl.php and associated css a little easier when coupled with render().

@scor: Good catch. This typo did not break any feature though, it's just a key in a render array, it's meaningless per se, and no other parts of the code referred to it specifically.
Fixed in the attached patch.

So, it seems we just need someone to RTBC this :-)

Many Drupal webmasters won't be reading this regarding abolishment of collapsed comments
particularly those who are happily using 4.7x or 5x or 6x and may need to upgrade only in some distant future. It will be appropriate if the compatibility is NOT broken as many sites do use collapsed comments or give the option to the USERs. This is also helpful where there are huge number of comments like 200, 300 where collapsed view helps to expand and read only the new ones.

Many sites use collapsed comments for example
http://willy.boerland.com/myblog/adobes_flex_site_using_drupal
http://wikis.sun.com/display/Drupal/Home
and plenty more on googling.
And if early signs are correct social networking sites and wordpress blog will soon have collapsed comments making them funky and genx rather than outdated seldom used stuffs.

If standard features that were there have to be removed it is best to do survey rather than thinking many do not use. It is a PLUS point of Drupal that it offers such a good choce of comment view option to its users

@kaakuu, collapsed comments were removed in #506218: Remove collapsed comments and tidy up settings, anything being removed here is just leftovers which didn't go out with that patch.

Great patch. The more we standardize everything that deals with entities, the easier it will get to merge some of their APIs at one point of time. So huge +1.
Only 1 nitpick I found while reading the patch top to bottom:

+    $node->content['comments']['prelude'] = array('#markup' => '<h2 class="comments">Comments</h2>', '#weight' => -1)

Missing t().

Added the t(), and changed that bit to $node->content['comments']['#prefix'] instead of a standalone $node->content['comments']['prelude']['#type'] = 'markup'

Would be extra cool to have this in before webchick leaves for vacation, so that we can move forward on #504666: Make comments fieldable meanwhile :-)

Very nice.

-      $regex .= '<\/div>/s'; // Dot matches newlines and ensure that match doesn't bleed outside comment div.
+      $regex .= '/s'; // Dot matches newlines and ensure that match doesn't bleed outside comment div.

^ Looks like the comment might need to be updated?

 /**
+ * A menu callback; build a comment editing form.
  */
+function comment_edit($comment) {
+  return drupal_get_form('comment_form', (array)$comment);
 }

Any particular reason to keep this dumb function?

-    // Append the list of comments to $node->content for node detail pages.
-    if ($node->comment && (bool)menu_get_object() && empty($node->in_preview)) {
-      $node->content['comments'] = array(
-        '#markup' => comment_render($node),
-        '#sorted' => TRUE,
-      );
+    // Only append comments when we are building a node on its own node detail page.
+    $page_node = menu_get_object();
+    if ($node->comment && isset($page_node->nid) && $page_node->nid == $node->nid && empty($node->in_preview) && user_access('access comments')) {
+      comment_node_page_additions($node);
+    }
+  }

^ (bool)menu_get_object() is really bad, but this new check is no better. Why not moving the comment rendering to hook_page_alter(), where it would make more sense?

 function template_preprocess_comment_wrapper(&$variables) {
+  $variables['node'] = menu_get_object();
+  $variables['content'] = $variables['elements']['#children'];

^ This effectively prevent rendering the comments of a node on a page that is not node/%node/.... Why not passing the $node object to the variables, in the theme() call?

There is a small defect in the last patch:

+  $variables['node'] = $variables['elements']['#node'];;

Except from that, the patch looks very good. I agree we can tackle the other issues in follow up patches, but we really need to do so. In particular, I believe that removing the infamous $page argument without any decent alternative was a mistake. The solution is probably somewhere in the "build mode".

Previous patches hardcoded the <h2> headings ('Comments', 'Post new comment') in code and out of the theme layer, that's a regression.

Updated patch moves them to comment-wrapper.tpl.php, thus introducing render() into that template - which de facto solves #517850: Update and polish default comment-wrapper.tpl.php and associated css (and #295895: Garland overrides overrides template with theme function, at least for D7)

Benchmarked #35 so we know what we're getting into. No significant change.

HEAD:

Concurrency Level:      1
Time taken for tests:   100.101 seconds
Complete requests:      500
Failed requests:        0
Write errors:           0
Total transferred:      38114000 bytes
HTML transferred:       37881500 bytes
Requests per second:    4.99 [#/sec] (mean)
Time per request:       200.201 [ms] (mean)
Time per request:       200.201 [ms] (mean, across all concurrent requests)
Transfer rate:          371.83 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   184  200  13.2    197     295
Waiting:      180  195  12.5    192     291
Total:        184  200  13.2    197     295

Patch:

Time taken for tests:   101.834 seconds
Complete requests:      500
Failed requests:        0
Write errors:           0
Total transferred:      37767000 bytes
HTML transferred:       37534500 bytes
Requests per second:    4.91 [#/sec] (mean)
Time per request:       203.668 [ms] (mean)
Time per request:       203.668 [ms] (mean, across all concurrent requests)
Transfer rate:          362.18 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   192  204   9.4    201     248
Waiting:      187  198   9.2    195     241
Total:        192  204   9.4    201     248

Took several passes over this patch the past couple of days and it all looks good, RTBC.

Expanded code comment about $page_node in comment_node_view(), as requested by Dries on IRC.

This is a slightly modified version of the addition I proposed in irc, looks good and fixes the bit I wasn't sure about :)

Reviewed it once more (had reviewed it on IRC last night), and committed it to CVS HEAD.

re Damien, Moshe #31, #32:

- Damien:

/**
+ * A menu callback; build a comment editing form.
  */
+function comment_edit($comment) {
+  return drupal_get_form('comment_form', (array)$comment);
}

Any particular reason to keep this dumb function?
- moshe: That function is there because of the cast to array. I tried to remove the cast but it added some ugly code to comment_form() as it does array concatenation in a couple places. This could be done in a followup patch. It is pretty unrelated to comment rendering

Patch in #537062: Remove unneeded comment_edit() page callback

Thanks for this awesome refactor, figuring out how to move comments in D6 was just about driving me nuts. :-)

Since comment_render() has been removed from the API, this should be documented on http://drupal.org/update/modules/6/7 . (I would help, but I don't really understand all the changes introduced here. comment_node_page_additions() seems like only part of the picture, since it returns array data instead of rendered markup).

fixing tag

#653068: Update documentation is incomplete