Task

Clean up the remaining theme functions in comment.module. This was originally a Twig conversion issue but now the issue is about refactoring one theme function that doesn't belong in the theme layer (it just returns a string of HTML), and removing a stale definition from comment_theme() for theme_comment_preview().

Remaining

None at the moment.

Theme function name Conversion status
theme_comment_block Removed in #1938062: Convert the recent_comments block to a view
theme_comment_post_forbidden Moved to CommentInterface::forbiddenMessage(). This really isn't necessary to go through the theme layer, per this comment.
theme_comment_preview Removed. The callback definition in comment_theme() is removed, whilst the actual function was removed way back in #142829: Allow comment-$type.tpl.php

#1898054: comment.module - Convert PHPTemplate templates to Twig
#1757550: [Meta] Convert core theme functions to Twig templates
#1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch

Review Bonus

See #2094585: [meta] [experimental] Core review bonus.

Points: 3

- #607828: Allow passing variables that need to be saved to drupalCreateContentType(): +1 for being old
- https://drupal.org/node/2093161#comment-7889079

Files: 
CommentFileSizeAuthor
#82 1987396-comment-theme-82.patch13.36 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 59,586 pass(es). View
#73 1987396-comment-theme-73.patch13.87 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987396-comment-theme-73.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

Cottser’s picture

Issue summary: View changes

Updated issue summary.

c4rl’s picture

Issue summary: View changes

Updated issue summary.

c4rl’s picture

Issue summary: View changes

Updated issue summary.

c4rl’s picture

Title: comment.module - Convert PHPTemplate templates to Twig » comment.module - Convert theme_ functions to Twig

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.

Cottser’s picture

Status: Active » Needs review
FileSize
10.83 KB
PASSED: [[SimpleTest]]: [MySQL] 55,977 pass(es). View

Here are just the theme_ function conversions.

epersonae2’s picture

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

brunodbo’s picture

Assigned: Unassigned » brunodbo

Assigned to myself for DC PDX sprint.

brunodbo’s picture

FileSize
10.89 KB
PASSED: [[SimpleTest]]: [MySQL] 58,121 pass(es). View

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

brunodbo’s picture

Assigned: brunodbo » Unassigned

Unassigning.

Hydra’s picture

Status: Needs review » Needs work

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

jgraham’s picture

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
jgraham’s picture

Issue tags: -Needs profiling
jgraham’s picture

ezeedub’s picture

Issue tags: +Needs profiling

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%
...
ezeedub’s picture

Status: Needs work » Needs review

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

ezeedub’s picture

Title: comment.module - Convert theme_ functions to Twig » comment.module - Convert theme_ functions to Twigaggregator-page-opml.html.twig

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&...

ezeedub’s picture

Issue summary: View changes

Updating from previous issue

ezeedub’s picture

Title: comment.module - Convert theme_ functions to Twigaggregator-page-opml.html.twig » comment.module - Convert theme_ functions to Twig
ezeedub’s picture

Issue summary: View changes

add atttribution

ezeedub’s picture

Issue summary: View changes

Updated issue summary.

Cottser’s picture

Not sure if manual testing (markup comparison) was done on this, tagging. Adding manual testing steps to the issue summary would help with profiling as well.

Profiling results look reasonable, not much difference. Thanks @ezeedub!

joelpittet’s picture

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

thedavidmeister’s picture

Issue tags: +Novice

manual testing is a novice task

adamcowboy’s picture

Issue tags: +Needs reroll

Needs Re-Roll.

adamcowboy’s picture

adamcowboy’s picture

jenlampton’s picture

Status: Needs review » Needs work
FileSize
11.41 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/comment/comment.module. View

rerolled.

jenlampton’s picture

Status: Needs work » Needs review

doh, status.

jenlampton’s picture

Issue tags: -Needs reroll

doh, tags.

Status: Needs review » Needs work

The last submitted patch, twig-convert_comment-1987396-21.patch, failed testing.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
10.79 KB
FAILED: [[SimpleTest]]: [MySQL] 57,240 pass(es), 55 fail(s), and 302 exception(s). View

syntax fixed

Status: Needs review » Needs work

The last submitted patch, twig-convert_comment-1987396-25.patch, failed testing.

markcarver’s picture

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

andypost’s picture

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

jenlampton’s picture

Status: Needs work » Needs review
FileSize
11.42 KB
PASSED: [[SimpleTest]]: [MySQL] 58,033 pass(es). View

rerolled. Passing tests now :)

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs profiling, +Needs reroll

This needs a reroll.

  1. +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
    @@ -92,11 +92,8 @@ public function form(array $form, array &$form_state) {
    -      $username = array(
    -        '#theme' => 'username',
    -        '#account' => $user,
    -      );
    -      $form['author']['name']['#markup'] = drupal_render($username);
    +      $form['author']['name']['#theme'] = 'username';
    +      $form['author']['name']['#account'] = $user;
    

    We can remove this change from the patch entirely. This was handled by #2008980: Replace theme() with drupal_render() in comment module.

  2. +++ b/core/modules/comment/templates/comment-block.html.twig
    @@ -0,0 +1,19 @@
    + * @see template_preprocess()
    

    We can remove this line.

Cottser’s picture

Issue summary: View changes

Updating commit message (pulling in people that I'm pretty sure worked on this as part of the .tpl.php conversion) and updating conversion table --Cottser

joelpittet’s picture

Status: Needs work » Needs review
FileSize
11.39 KB
PASSED: [[SimpleTest]]: [MySQL] 58,461 pass(es). View
601 bytes
11.42 KB
PASSED: [[SimpleTest]]: [MySQL] 58,557 pass(es). View

@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.

Cottser’s picture

Issue tags: -Needs reroll

Thanks @joelpittet for rocking all these issues!

Yeah, basically I was thinking #31.1 (and maybe the following as well) feels out of scope for Twig conversion at this point and could be moved to a sub-issue of #2046881: [meta] Avoid "early rendered" strings where beneficial to do so, build structured data to pass to drupal_render() once instead.

+++ b/core/modules/comment/comment.admin.inc
@@ -127,7 +126,12 @@ function comment_admin_overview($form, &$form_state, $arg) {
-      'author' => drupal_render($username),
+      'author' => array(
+        'data' => array(
+          '#theme' => 'username',
+          '#account' => comment_prepare_author($comment),
+        ),
+      ),
joelpittet’s picture

Status: Needs review » Needs work
joelpittet’s picture

FileSize
9.93 KB
PASSED: [[SimpleTest]]: [MySQL] 58,176 pass(es). View
1.8 KB

#33 taken care of.

joelpittet’s picture

Status: Needs work » Needs review

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

joelpittet’s picture

Issue summary: View changes

Update remaining

thedavidmeister’s picture

Issue summary: View changes

Updated issue summary.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

Hydra’s picture

FileSize
9.95 KB
FAILED: [[SimpleTest]]: [MySQL] 58,497 pass(es), 1 fail(s), and 46 exception(s). View

Rerolled

Hydra’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

Status: Needs review » Needs work
Issue tags: -Novice, -Needs manual testing, -Needs profiling, -Twig

The last submitted patch, 1987396-38-twig-theme-comment.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Novice, +Needs manual testing, +Needs profiling, +Twig

The last submitted patch, 1987396-38-twig-theme-comment.patch, failed testing.

Hydra’s picture

Status: Needs work » Needs review
FileSize
11.13 KB
FAILED: [[SimpleTest]]: [MySQL] 58,910 pass(es), 0 fail(s), and 46 exception(s). View
1.17 KB

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

Status: Needs review » Needs work
Issue tags: -Novice, -Needs manual testing, -Needs profiling, -Twig

The last submitted patch, 1987396-43-twig-theme-comment.patch, failed testing.

Hydra’s picture

Status: Needs work » Needs review
Hydra’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work
Issue tags: +Novice, +Needs manual testing, +Needs profiling, +Twig

The last submitted patch, 1987396-43-twig-theme-comment.patch, failed testing.

andypost’s picture

Also comment_prepare_author() is questionable

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
6.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987396-48-twig-theme-comment.patch. Unable to apply patch. See the log in the details link for more information. View

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.

joelpittet’s picture

Issue summary: View changes

Updated issue summary.

joelpittet’s picture

Status: Needs review » Needs work

The last submitted patch, 48: 1987396-48-twig-theme-comment.patch, failed testing.

andypost’s picture

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

joelpittet’s picture

@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:)

joelpittet’s picture

Status: Needs work » Needs review
FileSize
6.08 KB
FAILED: [[SimpleTest]]: [MySQL] 58,310 pass(es), 718 fail(s), and 129 exception(s). View

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?

Status: Needs review » Needs work

The last submitted patch, 53: 1987396-53-twig-theme-comment.patch, failed testing.

andypost’s picture

@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

joelpittet’s picture

@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.

joelpittet’s picture

Status: Needs work » Needs review
Related issues: +#2061899: Remove references to global $user in Comment module
FileSize
767 bytes
5.97 KB
FAILED: [[SimpleTest]]: [MySQL] 59,238 pass(es), 4 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 57: 1987396-57-twig-theme-comment.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
4.62 KB
FAILED: [[SimpleTest]]: [MySQL] 59,803 pass(es), 4 fail(s), and 0 exception(s). View

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 59: 1987396-59-twig-theme-comment.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
5.54 KB
PASSED: [[SimpleTest]]: [MySQL] 59,807 pass(es). View

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

joelpittet’s picture

andypost’s picture

Assigned: Unassigned » larowlan
Related issues:

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

larowlan’s picture

+++ 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.

larowlan’s picture

FileSize
12.42 KB
13.92 KB
FAILED: [[SimpleTest]]: [MySQL] 59,904 pass(es), 10 fail(s), and 0 exception(s). View

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

joelpittet’s picture

FileSize
9.75 KB
FAILED: [[SimpleTest]]: [MySQL] 59,886 pass(es), 307 fail(s), and 205 exception(s). View
8.37 KB

Like this?

The last submitted patch, 65: comment-theming-1987396.65.patch, failed testing.

The last submitted patch, 66: 1987396-65-twig-theme-comment.patch, failed testing.

andypost’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing, -Needs profiling, -Novice, -Needs issue summary update
FileSize
1.98 KB
13.87 KB
PASSED: [[SimpleTest]]: [MySQL] 59,985 pass(es). View

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 69: 1987396-comment-theme-69.patch, failed testing.

joelpittet’s picture

+++ 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.

andypost’s picture

Status: Needs work » Needs review

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

andypost’s picture

FileSize
993 bytes
13.87 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987396-comment-theme-73.patch. Unable to apply patch. See the log in the details link for more information. View

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

PS: #71 both syntax is fine

Cottser’s picture

Let's retitle this if there is no Twig conversion happening - something like "Refactor comment.module theme functions"?

joelpittet’s picture

Title: comment.module - Convert theme_ functions to Twig » Refactor & Clean-up comment.module theme functions
Assigned: larowlan » Unassigned
Status: Needs review » Reviewed & tested by the community

Agreed, and it's ready to go.

markcarver’s picture

+1

Cottser’s picture

Issue summary: View changes

Removing the suggested commit message from the summary, this patch changed direction quite a bit.

Cottser’s picture

Issue summary: View changes

Also updating the body of the issue summary to reflect the latest patch.

Cottser’s picture

Issue summary: View changes

s/converting/refactoring/

Cottser’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 73: 1987396-comment-theme-73.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
13.36 KB
PASSED: [[SimpleTest]]: [MySQL] 59,586 pass(es). View

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

catch’s picture

Status: Reviewed & tested by the community » Fixed

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

Status: Fixed » Closed (fixed)

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