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.
Massive updating of the spaghetti mess that is comment rendering. Got rid of horrid nested theme functions with logic and instead modelled the rendering on node_build_multiple(). I got rid of folded view for comment thread. I never saw anyone use it, and it isn't clear how to impement it in the new render system (a new build mode). Anyway, I think it can go. We still support flat and threaded of course.
TODO
- comment form does not work
- fix tests
- add 'first new' marker back
Comment | File | Size | Author |
---|---|---|---|
#40 | comment_render-523950-40.patch | 37.89 KB | yched |
#35 | comment_render-523950-35.patch | 37.68 KB | yched |
#34 | crender5.diff | 36.45 KB | moshe weitzman |
#32 | crender5.diff | 36.46 KB | moshe weitzman |
#30 | comment_render-523950-30.patch | 36.52 KB | yched |
Comments
Comment #2
kika CreditAttribution: kika commentedfyi, folded view is already dead #506218: Remove collapsed comments and tidy up settings, #520486: Remove comment-folded.tpl.php
Comment #3
kika CreditAttribution: kika commentedVery 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 ?
Comment #4
yched CreditAttribution: yched commentedBumping to critical, this is a blocker for #504666: Make comments fieldable
Comment #5
catchNice work Moshe.
diffstat 2 files changed, 285 insertions(+), 313 deletions(-) always good to see.
Comment #6
yched CreditAttribution: yched commented- In comment_build_content():
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) ?
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedRemoved that field attach line. Was in there since I copied from node_build().
Yes, granular themeing will egt in here soon.
Attached patch is better but still some real hassles with the comment form. It uses a pretty odd $edit array as input which I am still inspecting. Help is welcome. In any case, I'll keep on this over the next couple of days.
Comment #8
yched CreditAttribution: yched commentedNote 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.
Comment #9
moshe weitzman CreditAttribution: moshe weitzman commentedI *think* the form patch and this patch can go independantly. The second one to land will have to adapt a bit.
Most test failures fixed. I could use some help on the remaining ones. Setting to CNW just so everyone can see the remaining few failures.
Comment #11
yched CreditAttribution: yched commented- 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"/>
anchorWith 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.
Comment #12
scor CreditAttribution: scor commentedsubscribing, will need to refactor the RDF patches once this gets fixed
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedFixed indentation and a few test failures. A few more remain - help wanted. One is a NOTICE about $output_below that was introduced in the comment preview patch. That variable is either undefined or empty when i look the code. The other failures are those page 1/page 2 failures. I am tempted to remove that part of the test. It largely duplicates pager's own tests ... CNR so folks know more or less where the failures are.
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedfixed paging test and $output_bottom notices. also reworked preview so that it uses a render() array in $page.
could use help with remaining failures.
Comment #17
moshe weitzman CreditAttribution: moshe weitzman commentedI think this one fixes remaining failures.
Comment #18
catchLooks very, very nice.
Capital 'T', should probably be
$divs
instead of'divs
Do we do this in core? Most functions that alter by ref just do it in the phpdoc.
no extra line break before @return
Looks like this can go?
Comment #19
yched CreditAttribution: yched commentedYay. 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
With current patch we only have:
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.With current patch, comment links are within the
<div class="content">
: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.
Comment #20
yched CreditAttribution: yched commentedAttached 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.
Comment #21
yched CreditAttribution: yched commentedBetter PHPdoc for comment_node_page_additions().
Comment #22
yched CreditAttribution: yched commentedThe
<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...
Comment #23
moshe weitzman CreditAttribution: moshe weitzman commentedLooking good. Attached patch fixes 2 minor nits
* removed code comment at end:
+ $comment->first_new = TRUE; // "<a id=\"new\"></a>\n";
* template_preprocess_comment_folded() is now removed
Comment #24
kika CreditAttribution: kika commentedI 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)?
Comment #25
scor CreditAttribution: scor commented(note the 3 'm' in commment_parent)
why did this patch pass all tests, is there no test for this line?
Comment #26
yched CreditAttribution: yched commented@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 :-)
Comment #27
kaakuu CreditAttribution: kaakuu commentedMany 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
Comment #28
catch@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.
Comment #29
Frando CreditAttribution: Frando commentedGreat 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:
Missing t().
Comment #30
yched CreditAttribution: yched commentedAdded 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 :-)
Comment #31
Damien Tournoud CreditAttribution: Damien Tournoud commentedVery nice.
^ Looks like the comment might need to be updated?
Any particular reason to keep this dumb function?
^ (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?
^ 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?Comment #32
moshe weitzman CreditAttribution: moshe weitzman commentedAddresses Damien's issues
Comment #33
Damien Tournoud CreditAttribution: Damien Tournoud commentedThere is a small defect in the last patch:
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".Comment #34
moshe weitzman CreditAttribution: moshe weitzman commentedremoved extra semi colon
Comment #35
yched CreditAttribution: yched commentedPrevious 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)
Comment #36
catchBenchmarked #35 so we know what we're getting into. No significant change.
HEAD:
Patch:
Took several passes over this patch the past couple of days and it all looks good, RTBC.
Comment #37
catchComment #38
moshe weitzman CreditAttribution: moshe weitzman commentedI'm happy with yched's latest change and the green bot and the green benchmarks. Let's commit before we go back to neglecting ole comment module :)
Comment #39
moshe weitzman CreditAttribution: moshe weitzman commentedx-post. fix status.
Comment #40
yched CreditAttribution: yched commentedExpanded code comment about $page_node in comment_node_view(), as requested by Dries on IRC.
Comment #41
catchThis is a slightly modified version of the addition I proposed in irc, looks good and fixes the bit I wasn't sure about :)
Comment #42
Dries CreditAttribution: Dries commentedReviewed it once more (had reviewed it on IRC last night), and committed it to CVS HEAD.
Comment #43
yched CreditAttribution: yched commentedre Damien, Moshe #31, #32:
Patch in #537062: Remove unneeded comment_edit() page callback
Comment #45
grendzy CreditAttribution: grendzy commentedThanks 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).
Comment #46
grendzy CreditAttribution: grendzy commentedfixing tag
Comment #47
catch#653068: Update documentation is incomplete
Comment #48
moshe weitzman CreditAttribution: moshe weitzman commentedAdded upgrade docs. feel free to improve them. http://drupal.org/update/modules/6/7#comment_render