Refactor & Clean-up comment.module theme functions

Updated issue summary.

Updated issue summary.

Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions rather than templates. See #1898054: comment.module - Convert PHPTemplate templates to Twig for template conversion.

#2: 1987396-2.patch queued for re-testing.

Assigned to myself for DC PDX sprint.

Reroll (since it didn't apply anymore) - hope I did that right, first time ever :)

Unassigning.

Manual review looks good! Rerolled patch is applying properly. Still profiling to do!

Here's my profiling info. Hopefully its correct.

=== 8.x..8.x compared (519fe25479d73..519fe7d418358):

ct  : 41,147|41,147|0|0.0%
wt  : 562,871|564,768|1,897|0.3%
cpu : 548,035|540,034|-8,001|-1.5%
mu  : 49,437,568|49,437,568|0|0.0%
pmu : 49,583,368|49,583,368|0|0.0%

<a href="http://127.0.0.1/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run1=519fe25479d73&run2=519fe7d418358&extra=8.x..8.x">Profiler output</a>

=== 8.x..1987396-profile compared (519fe25479d73..519fe87227310):

ct  : 41,147|41,147|0|0.0%
wt  : 562,871|563,618|747|0.1%
cpu : 548,035|552,035|4,000|0.7%
mu  : 49,437,568|49,435,856|-1,712|-0.0%
pmu : 49,583,368|49,581,856|-1,512|-0.0%

<a href="http://127.0.0.1/xhprof-kit/xhprof/xhprof_html/index.php?source=drupal-perf&url=%2F&run1=519fe25479d73&run2=519fe87227310&extra=8.x..1987396-profile">Profiler output</a>

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

re-tagging needs profiling, because same number of function calls in both runs:

=== 8.x..8.x compared (519fe25479d73..519fe7d418358):

ct  : 41,147|41,147|0|0.0%
...

=== 8.x..1987396-profile compared (519fe25479d73..519fe87227310):

ct  : 41,147|41,147|0|0.0%
...

#5: 1987396-5.patch queued for re-testing.

Add some comments, and place the recent comments block.

Not sure why, but I get a different number of function calls when comparing 8.x...8.x. I ran it a second time when I noticed this but got that again. Not sure why...

=== 8.x..8.x compared (51a278c0bf3d5..51a2798a2b504):

ct  : 50,689|50,708|19|0.0%
wt  : 439,808|436,186|-3,622|-0.8%
cpu : 432,027|432,028|1|0.0%
mu  : 8,910,380|8,910,388|8|0.0%
pmu : 8,989,068|8,989,076|8|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a278c0bf3d5&...

=== 8.x..comment-1987396-5 compared (51a278c0bf3d5..51a279f421da5):

ct  : 50,689|50,800|111|0.2%
wt  : 439,808|434,625|-5,183|-1.2%
cpu : 432,027|428,027|-4,000|-0.9%
mu  : 8,910,380|8,929,768|19,388|0.2%
pmu : 8,989,068|9,008,328|19,260|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a278c0bf3d5&...

Updating from previous issue

add atttribution

Updated issue summary.

#5: 1987396-5.patch queued for re-testing.

manual testing is a novice task

Needs Re-Roll.

rerolled.

doh, status.

doh, tags.

syntax fixed

andypost mentioned in IRC that the "comment-block.html.twig" template was lost from #21 -> #25.

Marked as duplicate #1914000: Convert l() call in template_preprocess_comment_block() into a render array to be handled by Twig. and closely related #1938062: Convert the recent_comments block to a view

Filed separate issue to make block more sane #2054993: Remove (untested, unused, broken) comment_get_recent()

rerolled. Passing tests now :)

@Cottser re #31 I don't think we should remove that change you listed in 1. regarding the change from drupal_render to the '#theme' as that is the way those should be going so they aren't rendered too early. Though if you want we can move it over to another issue? Maybe the one we have for drupal_render() removals?

Attached are re-roll and item 2.

follow up at #2073957: Remove useless "early render" in Comment Module

#33 taken care of.

Ok this should be ready for manual testing and profiling. Test bot start!

Update remaining

Updated issue summary.

Rerolled

#38: 1987396-38-twig-theme-comment.patch queued for re-testing.

Fixed the test after redirecting to the node when no access.

#43: 1987396-43-twig-theme-comment.patch queued for re-testing.

Updated issue summary.

Also comment_prepare_author() is questionable

The last patch didn't apply anymore and rebasing a re-roll was super complicated so I manually re-rolled with some tweaks to how comment_block is done to hopefully remove the markup from the preprocess function and provide more data to the template.

Updated issue summary.

48: 1987396-48-twig-theme-comment.patch queued for re-testing.

I'd like to point that comments are rendered within field template so probable better to remove comment-wrapper

@andypost do you mean that comment-wrapper is not used at all now? If so, I'll open up a separate issue for that and remove it. This is a conversion issue so I'd rather not double up on it's reasons not to get committed.

Thanks for checking in on this issue:)

Hmm that being said, I've done things to this patch that aren't straight conversion already. Here's a reroll to get it applying again.

So up to you @andypost do you think if I remove that rapper it would still be good to get in or should it be a separate issue?

@joelpittet Yep, I think comment wrapper is useless and should be removed, I'd suggest to make it in #1962846: Use field instance name for header of comment list, drop comment-wrapper template
Also we really need to use field name as header because "Comments" could be "Reviews", "Discussions" ant etc. and node could have more then one comment field attached

@andypost ok good to note, I'll leave it to that issue to make those changes. We'll keep an eye on that issue.

comment-wrapper.html.twig (other than moving the hook_theme declaration of 'template' to the bottom) is not in this issue.

#2061899: Remove references to global $user in Comment module is dealing with the global $user; so I reverted that in this patch.

Re-rolled.

This doesn't have any actual twig conversions anymore, it's become cleanup and replacing the message theme with a function.

Awesome clean-up!!! I'd like to get @larowlan opinion on proposed change

Also comment wrapper should be removed in #1962846: Use field instance name for header of comment list, drop comment-wrapper template

+++ b/core/modules/comment/comment.module
@@ -1475,19 +1459,21 @@ function template_preprocess_comment(&$variables) {
-function theme_comment_post_forbidden($variables) {
...
+function comment_forbidden_message(EntityInterface $entity, $field_name) {

@@ -494,13 +488,8 @@ function comment_entity_view(EntityInterface $entity, EntityViewDisplayInterface
-              'title' => drupal_render($comment_post_forbidden),
+              'title' => comment_forbidden_message($entity, $field_name),

@@ -529,13 +518,8 @@ function comment_entity_view(EntityInterface $entity, EntityViewDisplayInterface
-              'title' => drupal_render($comment_post_forbidden),
+              'title' => comment_forbidden_message($entity, $field_name),

+++ b/core/modules/comment/lib/Drupal/comment/CommentViewBuilder.php
@@ -240,12 +240,7 @@ protected static function buildLinks(CommentInterface $entity, EntityInterface $
-        $links['comment-forbidden']['title'] = drupal_render($comment_post_forbidden);
+        $links['comment-forbidden']['title'] = comment_forbidden_message($commented_entity, $entity->field_name->value);

This function builds a "login or register to post comments" links so better to put it into commantManager.
Also there's drupal_static used so probably we can get rid of it

+++ b/core/modules/comment/comment.module
@@ -1475,19 +1459,21 @@ function template_preprocess_comment(&$variables) {
+function comment_forbidden_message(EntityInterface $entity, $field_name) {

Yeah, I agree with @andypost, please move this to the CommentManager and the static to a new property on the manager. We're trying to avoid adding new global functions.
Otherwise, +1.

Moves comment_forbidden_message to \Drupal\comment\CommentManagerInterface::forbiddenMessage

Like this?

Cleaned-up implementation of #65, RTBC because there's agreement.
Summary updated with latest change, also fixed commit message

+++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
@@ -259,17 +258,18 @@ public function getFieldUIPageTitle($commented_entity_type, $field_name) {
+        if ($entity->$field_name->getFieldDefinition()->getSetting('form_location') == COMMENT_FORM_SEPARATE_PAGE) {

$entity->$field_name->

should be

$entity->{$field_name}->
if that is the way you are trying to do it.

69: 1987396-comment-theme-69.patch queued for re-testing.

Can't reproduce failure locally.
Changed this line to more performant getter (skipping magic method call)

PS: #71 both syntax is fine

Agreed, and it's ready to go.

+1

re-roll (super minor comment.admin.inc removed, was a comment change)

Looks great. Committed/pushed to 8.x, thanks!