comment.module - Convert PHPTemplate templates to Twig

I'm looking at this as part of Drupalcon code sprint.

Here's a work-in-progress for getting the existing tpl.php files into html.twig format, and updating relevant "support" code like the preprocess + docs. Haven't done the theme functions yet.

Focussing on 1:1 match with current D8 html for now even though I saw a few things that could be improved in the template, I didn't want to be trying to fix two thing at once.

This is a patch against core that replaces all tpl.php files and theme functions for the comment module with html.twig template files.

I removed the 'comment_preview' item from comment_theme() as I could not find a single place where it is used to theme anything - comment previews are actually rendered by comment.html.twig. Everything else has been converted as close to 1:1 resulting markup with current core as possible.

I'll try to get this green. Apparently something's going wrong with previews in the test.

Problem was bartik having a couple of comment templates that were using the old tpl.php style. Since those templates were identical to the core ones, I ripped them out and all my local tests started coming back green. Made a few other minor changes to fix a few exceptions and simplify the twig templates a bit too.

+++ b/core/modules/comment/comment.moduleundefined
@@ -198,16 +198,15 @@ function comment_field_extra_fields() {
-    'comment_preview' => array(
-      'variables' => array('comment' => NULL),

Why are we removing this?

+++ b/core/modules/comment/comment.moduleundefined
@@ -1619,9 +1598,23 @@ function comment_prepare_author(Comment $comment) {
+  }
...
+  $variables['comments'] = !empty($items) ? theme('item_list', array('items' => $items)) : '';
+}
+

This is not yet thoroughly discussed, but we are in the works of deprecating theme() (at least from preprocess).

Please use a render array like:

$variables['comments'] = '';
if ($items) {
$variables['comments'] = array('#theme' => 'item_list', '#items' => 'items');
}

That way it will be rendered just in the template and not yet in the preprocess.

The same could be done for the l() here, but due to performance I would leave it for now and do maybe as follow-up.

+++ b/core/modules/comment/comment.moduleundefined
@@ -1732,63 +1725,58 @@ function template_preprocess_comment(&$variables) {
+  // Split out some variables to make it easier on theme devs.
+  $variables['comments'] = $variables['content']['comments'] ?: array();
+  $variables['form'] = $variables['content']['comment_form'] ?: array();

These are both no-ops (the ?:), because if it is FALSE the result is array() anyway.

Please use the longer form with:

isset(x)?x:array();

Thanks!

+++ b/core/modules/comment/templates/comment-block.html.twigundefined
@@ -0,0 +1,19 @@
+{% if comments is empty %}
+  {{ 'No comments available.'|t }}

We might want to use either here:

{% if not comments

or

later

{% if comments is not empty

But it should be consistent ...

+++ b/core/modules/comment/templates/comment-wrapper.html.twigundefined
@@ -0,0 +1,47 @@
+<section id="comments" class="{{ attributes.class }}"{{ attributes }}>
+  {# TODO forum module should extend comment template to keep the code in here clean #}
+  {% if comments and node.type != 'forum' %}

"@todo" is easier to find in the code by grepping for it.

Should be consistent with comment-block.html.twig.

Either use the long form here

{% if comments is not empty

or the short form above.

+++ b/core/modules/comment/templates/comment.html.twigundefined
@@ -0,0 +1,95 @@
+  <div class="content"{{ content_attributes }}>
+    {# We hide the links now so that we can render them later. #}
+    {{ hide(content.links) }}
+    {{ content }}

The <div class="content" is wrong and should include the content_attributes as else we might end up with two class="" statements.

Might need to fix a markup test if you change it here.

This should use:

{% hide(...) %}

as

{{ is for printing.

Great start!

Please continue the nice work!

- Removing 'comment_preview' because I really don't think it is doing anything. No existing theme function, no existing template file, no reference to it anywhere else in the code base.
- updated the item_list in comment_block to be render array style
- ignoring l() for now, happy to make a follow-up issue
- swapped out ?: with isset(), this is still necessary for now as Twig was throwing exceptions when I asked it to print NULL values. Happy to make a follow up issue to get Twig handling NULL better when printing, which will make preprocessors and templates both easier to write.
- using short form "if not comments" in comments-block.html.twig since I very much prefer the converse "if comments" over "if comments is not empty"
- The <div class="content"> situation is due to copying core as of yesterday at midday, I've replaced with <div class="content {{ content_attributes.class }}"{{ content_attributes }}>
- using {% hide() %} in comment.html.twig
- using @todo instead of TODO

Patch attached. Testbot is currently taking close to 2 hours to complete requests but I'm getting 100% green running the comments tests on my local.

Thanks for all your work on this, @thedavidmeister! I found some minor documentation fixes that can be done to move this further.

+++ b/core/modules/comment/comment.moduleundefined
@@ -1619,9 +1598,29 @@ function comment_prepare_author(Comment $comment) {
+ * Preprocess variables for comment-block.html.twig
...
+ * Preprocesses variables for comment.html.twig

@@ -1732,63 +1731,58 @@ function template_preprocess_comment(&$variables) {
+ * Preprocesses variables for comment-post-forbidden.html.twig

Let's go with "Preprocesses variables for…" for now for these, and they should end in a period per http://drupal.org/node/1354#drupal. We're working out the exact standards for documenting preprocess functions in #1913208: [policy] Standardize template preprocess function documentation.

+++ b/core/modules/comment/comment.moduleundefined
@@ -1619,9 +1598,29 @@ function comment_prepare_author(Comment $comment) {
+ * @see  comment-block.html.twig

Extra space after @see, please remove.

+++ b/core/modules/comment/templates/comment-block.html.twigundefined
@@ -0,0 +1,19 @@
+ * @see template_preprocess
+ * @see template_preprocess_comment_block

+++ b/core/modules/comment/templates/comment-wrapper.html.twigundefined
@@ -0,0 +1,47 @@
+ * @see template_preprocess
+ * @see template_preprocess_comment_wrapper

+++ b/core/modules/comment/templates/comment.html.twigundefined
@@ -0,0 +1,95 @@
+ * @see template_preprocess
+ * @see template_preprocess_comment

These @see references need to end in parens since they point to functions, i.e. @see template_preprocess(), per http://drupal.org/node/1354#see. The @see references were updated yesterday in the sandbox but need to be updated here as well to meet documentation standards.

Standards cleanup as requested by Cottser.

Thanks @thedavidmeister! I should have mentioned this earlier but an interdiff would be very helpful as well :)

sorry, interdiff makes sense. I'll keep that in mind for the next revision/patch I do.

whaaaat??

#12: 1898054-twig-for-comments-12.patch queued for re-testing.

I don't know what happened to the testbot. Fatal errors were coming from the contact form of all places and all I changed was a little whitespace and some comments... I re-tested it and everything came back green.

i just got patchutils and ran the interdiff command... thanks for showing me that link Cottser.

Followup issue for converting l() to a render array #1914000: Convert l() call in template_preprocess_comment_block() into a render array to be handled by Twig..

I tried to replicate the problems I was having with exceptions for null values being printed into Twig templates. I think I must have been doing something wrong yesterday as I couldn't reproduce any problems at all tonight during tests.

It's a good thing, no need to create new follow-up issues and I simplified the preprocess functions a little bit in this patch. Nothing else has changed :)

Contributors (via git blame of preprocess functions and template files as well as a quick issue search in the sandbox) added to issue summary.

@thedavidmeister - I noticed a couple other very minor tweaks as I read through the patch again. No need to set to "needs work" for these.

1. comment-wrapper.html.twig needs an @ingroup themeable added, just like comment.html.twig.

2. +++ b/core/modules/comment/templates/comment-wrapper.html.twigundefined
   @@ -0,0 +1,47 @@
   + * Provides an html container for comments.
   

   HTML should be all caps.

np.

This looks really close guys, but it looks like we deleted the PHPTemplate templates from the Bartik theme, but didn't add twig versions back in their place. Did they just get excluded from a patch by accident?

I removed them deliberately. Comment #7:

Problem was bartik having a couple of comment templates that were using the old tpl.php style. Since those templates were identical to the core ones, I ripped them out and all my local tests started coming back green.

I couldn't see why we need templates in Bartik that are identical/duplicates of what's in core so I didn't add them back in. Is that really bad?

Patch from #24.

Nice work. Here's some nitpicks.

+++ b/core/modules/comment/comment.module
@@ -1616,9 +1595,26 @@ function comment_prepare_author(Comment $comment) {
 /**
- * Preprocesses variables for comment.tpl.php.
+ * Preprocesses variables for comment-block.html.twig.
  *
- * @see comment.tpl.php
+ * @see comment-block.html.twig

This docblock language needs to be edited as per #1913208: [policy] Standardize template preprocess function documentation.

+++ b/core/modules/comment/comment.module
@@ -1616,9 +1595,26 @@ function comment_prepare_author(Comment $comment) {
+/**
+ * Preprocesses variables for comment.html.twig.
+ *

Same here.

+++ b/core/modules/comment/comment.module
@@ -1729,63 +1724,58 @@ function template_preprocess_comment(&$variables) {
 /**
- * Returns HTML for a "you can't post comments" notice.
+ * Preprocesses variables for comment-post-forbidden.html.twig.
  *
- * @param $variables
- *   An associative array containing:
- *   - node: The comment node.
- *
- * @ingroup themeable
+ * @see comment-post-forbidden.html.twig

Needs updated docblock as per new convention. I'd say maybe keep the @param since it's already there and still relevant.

+++ b/core/modules/comment/templates/comment-post-forbidden.html.twig
@@ -0,0 +1,31 @@
+{#
+  We only output a link if the current user is logged out and we are certain
+  that users will get permission to post comments by logging in.
+#}
+{% if logged_out and can_post_comments %}
+  {% if user_register %}
+    {{ '<a href="@login">Log in</a> or <a href="@register">register</a> to post comments' | t({'@login': user_login_url, '@register': user_register_url}) }}
+  {% else %}
+    {{ '<a href="@login">Log in</a> to post comments' | t({'@login': user_login_url}) }}
+  {% endif %}

We don't need to run these through the t filter. We aren't translating URLs. Just {{ user_login_url }} and {{ user_register_url }} inbetween raw <a> markup should do it.

--- /dev/null
+++ b/core/modules/comment/templates/comment-wrapper.html.twig
@@ -0,0 +1,49 @@
+{#
+/**
+ * @file
+ * Provides an HTML container for comments.
+ *
+ * Available variables:
+ * - content: The array of content-related elements for the node. Use
+ *   render_var($content) to print them all, or print a subset such as
+ *   render_var($content['comment_form']).
+ * - attributes: Remaining html attributes for the containing element.
+ *   It includes the 'class' information, which includes:
+ *   - comment-wrapper: The current template type, i.e., "theming hook".
+ *   - title_prefix: An array containing additional output populated by
+ *     modules, intended to be displayed in front of the main title tag that
+ *     appears in the template.
+ *   - title_suffix: An array containing additional output populated by
+ *     modules, intended to be displayed after the main title tag that
+ *     appears in the template.
+ *
+ * The following variables are provided for contextual information.
+ * - node: The node entity to which the comments belong.
+ *   The constants below the variables show the possible values and should be
+ *   used for comparison.
+ *   - display_mode
+ *     - COMMENT_MODE_FLAT
+ *     - COMMENT_MODE_THREADED
+ *
+ * @see template_preprocess()
+ * @see template_preprocess_comment_wrapper()
+ *
+ * @ingroup themeable
+ */

@file
Default theme implementation for a comments container.

render_var($content) comments are not relevant in Twig.

display_mode CONSTANTS may come through as int. Double check and update docs here accordingly.

+++ b/core/modules/comment/templates/comment-wrapper.html.twig
@@ -0,0 +1,49 @@
+  {% if comments and node.type != 'forum' %}

{% if comments and node.type is not 'forum' %} ... get used to natural language in conditions. :)

+++ b/core/themes/bartik/templates/comment-wrapper.html.twig
@@ -0,0 +1,49 @@
+<section id="comments" class="{{ attributes.class }}"{{ attributes }}>
+  {# @todo forum module should extend comment template to keep the code in here clean #}
+  {% if comments and node.type != 'forum' %}
+    {{ title_prefix }}
+    <h2 class="title">{{ 'Comments' | t }}</h2>
+    {{ title_suffix }}
+  {% endif %}
+
+  {{ comments }}
+
+  {% if form %}
+    <h2 class="title comment-form">{{ 'Add new comment' | t }}</h2>
+    {{ form }}
+  {% endif %}
+

As per Twig docs on filters, remove space surrounding pipe | symbol:

{{ 'Comments'|t }}, etc...

That's about all I see.

#4: 1898054-twig-for-comments-4.patch queued for re-testing.

Oops, didn't expect that to happen. I thought that link would run some sort of automated test. Don't know how to undo what I did. Please disregard.

@thedavidmeister - the templates in core should not be the same as the ones in Bartik. If they look the same in what we've done - then we're doing something wrong. :)

We'll need to open the core .tpl.php files and the bartik .tpl.php files that are currently in 8, and do a diff. The same markup differences that exist in those files will need to exist in our twig versions.

For now, let's leave the templates in bartik alone (as .tpl.php files) until we figure out #1938948: Temporarily allow PHPTemplate themes to use .html.twig templates during Twig conversion

After that ges resolved we may need to replace those as part of this patch as well.

#27 - thanks for that review. I'll look over it soon, just got back from a weekend away so I didn't see it earlier.

#30 - I did a diff, comment-wrapper.tpl.php is definitely identical in core and in bartik except the bartik one says "this is bartik's template". There are some differences in comment.tpl.php in bartik that I didn't originally see - I'll have to make sure they're reflected in the twig template. Having Bartik templates in place makes it impossible to see whether the core templates themselves are really passing tests or not, this is a serious mistake IMO, why on earth is the default theme used by core for tests overriding core templates and changing the markup?? in practice this allows tests to pass when they wouldn't for any theme that isn't overriding templates in the same way that Bartik does.

I just opened #1939286: The theme that Drupal core runs tests in on d.o. (Bartik) should not be allowed to override core templates so we don't have to discuss whether Bartik "should" have it's own template files in this thread. I'll just try to convert everything 1:1 even though I hate that it's there at all :)

@thedavidmeister - I know #1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig has been keeping you busy - mind if I work on a new patch for this issue?

That would be great actually, thanks for the offer! Hopefully I can get to some kind of closure on the l() issue soon and get back into making templates ;)

I was thinking that we need two patches for this thread, one with the Bartick templates in it that we intend to have committed and one without, so we can flag any failed tests against the core templates that won't be picked up by the bots with the Bartick templates in place but would still effect most end-users.

Okay, I'm on it. Expect a revised patch a bit later in the week.

First - a reroll, the patch from #26 no longer applied.

Addressed everything from #27 and did a bunch more documentation cleanup/updates (i.e. remove references to arrays from .html.twig files) and converted a couple theme() calls to render arrays.

This is the only thing mentioned in the review in #27 that was not changed, regarding comment-post-forbidden.html.twig:

We don't need to run these through the t filter. We aren't translating URLs. Just {{ user_login_url }} and {{ user_register_url }} inbetween raw <a> markup should do it.

I think these look just fine, I don't see how else to get the strings to t() for translation. Left as is, just removed the spaces surrounding the pipe character.

@todo:
Compare core vs. Bartik templates and make the Twig templates in this patch match those differences, would fix the below regression and probably others.

HEAD:

This patch:

#37 - thanks for the update, much appreciated. If you're working on this again this weekend I'll try to find time to do a review for you once the Bartik templates are updated :)

+++ b/core/modules/comment/templates/comment-wrapper.html.twigundefined
@@ -0,0 +1,55 @@
+ * - node: The node entity to which the comments belong.
+ *   The constants below the variables show the possible values and should be
+ *   used for comparison, as in the following example:
+ *   @code
+ *   {% if display_mode is constant('COMMENT_MODE_THREADED') %}
+ *     <h3>{{ 'These comments are displayed in a threaded list.'|t }}</h3>
+ *   {% endif %}
+ *   @endcode
+ *   - display_mode: The display mode for the comment listing, flat or threaded.
+ *     - COMMENT_MODE_FLAT
+ *     - COMMENT_MODE_THREADED

Actually, the "The constants below" and code sample should be part of the display_mode list item, otherwise this will make very little sense when looking at on api.d.o :)

Edit: maybe not… see http://api.drupal.org/api/drupal/core%21modules%21comment%21templates%21...

Well that's weird, it didn't like the render arrays I guess. Will check it out tonight.

Will make a follow-up for #731724: Convert comment settings into a field to make them work with CMI and non-node entities

This should fix all the test failures. Next up, Bartik!

'author' and 'parent_author' go through t(), so I think that's a no go to convert to render arrays for now. See #1706612: remove 'submitted' variable in templates for ease of theme development that would help with at least 'author'.

Also {% node.type is not 'forum' %} from #27 doesn't seem to be a thing in Twig, != works though.

#43 - Yeah, apparently "node.type is not 'forum'" doesn't work because "is" in Twig is only related to Twig "tests" http://twig.sensiolabs.org/doc/tests/index.html and 'forum' is not a Twig test. I definitely remember going through all this in IRC with somebody - guess I forgot to bring it back here, sorry!

You could possibly do "node.type is not sameas('forum')" http://twig.sensiolabs.org/doc/tests/sameas.html but that's rather verbose, I think I like it less than !=

I also vaguely remember going through the render arrays in t() stuff back at Drupalcon when I first picked up this issue (t() hates render arrays)... I think there's a little double handling going on here, but at least we're ending up in the same place :)

Yeah, I found sameas() as well but preferred !=. Thanks @thedavidmeister :)

Updating status, need to update the Bartik templates.

Updated issue summary.

Tests seem to pass locally now without changing Bartik, let's see if testbot agrees.

Snuck in a couple comment.tpl.php -> comment.html.twig documentation changes as well.

Add conversion summary table

Update remaining

This could use manual testing to check our markup, if someone can write up brief testing steps and add to the summary (similar to #1898432: node.module - Convert PHPTemplate templates to Twig) that would be great.

Cottser left this assigned to himself but the status is "needs review". I'm assigning this to myself to review and test the most recent patch manually.

Nitpicks:

comment-wrapper.html.twig:

 * - node: The node entity to which the comments belong.
 *   The constants below the variables show the possible values and should be
 *   used for comparison, as in the following example:
 *   @code
 *   {% if display_mode is constant('COMMENT_MODE_THREADED') %}
 *     <h3>{{ 'These comments are displayed in a threaded list.'|t }}</h3>
 *   {% endif %}
 *   @endcode
 *   - display_mode: The display mode for the comment listing, flat or threaded.
 *     - COMMENT_MODE_FLAT
 *     - COMMENT_MODE_THREADED

"The constants ...." is not supposed to be indented below "- node:" is it? It looks like it's supposed to be a new section - it was previously in comment-wrapper.tpl.php.

  {# @todo Forum module should extend the comment template to keep the code in
       here clean. #}

Is there a followup issue open for this on d.o.?

comment.html.twig:

 * - created: Formatted date and time for when the comment was created.
 *   Preprocess functions can reformat it by calling format_date() with the
 *   desired parameters on the comment->created variable.
 * - changed: Formatted date and time for when the comment was last changed.
 *   Preprocess functions can reformat it by calling format_date() with the
 *   desired parameters on the comment->changed variable.

Inside the preprocess this variable would be $comment->changed, not comment->changed.

* - picture: The comment author's picture.

user_picture?

* - classes: String of classes that can be used to style contextually through

attributes.class?

- There's no explanation of what the variables "parent" or "content_attributes" do in the documentation, they're just used in the template. comment.tpl.php had a nice discussion of the $parent_* variables.

comment-post-forbidden.html.twig

* Default theme implementation for a "you can't post comments" notice.

The preprocess was changed to "for comment forbidden notice templates." from "you can't post comments" notice. This should match that.

 * - can_post_comments: Whether the user will be able to post comments once
 *   logged in.

Technically it's only whether authenticated users can post comments. A site might allow some roles to post but not others. The user might be able to post once they have logged in but can_post_comments would still be false if they had some special role. Maybe this should say "Whether authenticated users can post comments once logged in."

  {% if logged_out and can_post_comments %}

This should be decided before we calling a template called "post-forbidden", not inside it - that or this template could be merged into comment-wrapper. This doesn't impact the conversion as it was always in the theme function but maybe we should open a followup ticket and add a @todo notice.

If patches posted here could have a version posted with the Bartik templates deleted until #1595028: Convert tests using Standard profile to use Testing profile instead lands that would be great for speeding up reviews.

That is to say, for each re-roll upload one patch that doesn't touch Bartik that we intend to have committed, and one "test safe" patch with the Bartik templates deleted that we don't intend to commit so we can be *sure* that the testbot is not just happily passing everything by using the tpl.php files in the Bartik theme regardless of whether the Twig template is working or not. Both patches need to come back green before the "real" patch can be committed.

Also, I'd like to confirm before we move on - are we converting Bartik's comment templates in this issue or in #1938840: bartik.theme - Convert PHPTemplate templates to Twig? My vote is to handle that in the Bartik specific issue and really focus on core's handling of comments in this issue.

Thanks for the thorough review @thedavidmeister. For testing the conversion in this issue, please use the Stark theme, and we'll handle the Bartik overrides in #1938840: bartik.theme - Convert PHPTemplate templates to Twig.

Update remaining and conversion table

I was originally hoping to update this patch, but currently have too many others on the go. If someone else wants to look at updating the patch based on the feedback in #50 that would be great :) Any questions just post here, even a partially updated patch would help move this forward though!

When this makes it to commit, someone should look at decoupling comment-wrapper.html.twig from the forum module: #1285842: Forum module should implement its own node and comment templates to make theming easier

There are still a couple of calls to theme() in template_preprocess_comment()

$variables['author'] = theme('username', array('account' => $account));

$variables['parent_author'] = theme('username', array('account' => $account_parent));

Removing these causes array to string conversion exceptions in the tests. Should they be left as they are, or should the tests be altered to render the arrays before using them?

#57 - when you say "removing them" do you mean turning those variables into renderable arrays? Exactly which tests in what file are failing?

I think we need to keep those as theme() calls for now, they are passed around as strings.

#58 - When I run the comment tests with the those functions converted like so:

$variables['author'] = array(
    '#theme' => 'username',
    '#account' => $account,
  );
  
...

    $variables['parent_author'] = array(
      '#theme' => 'username',
      '#account' => $account_parent,
    );

I get this back:

3475 passes, 2 fails, 360 exceptions, and 858 debug messages

The vast majority of these exceptions are array to string conversions triggered by CommentTestBase::postComment calling drupal_post() with an array rather than a string for the author. This should be easy enough to fix for these tests, however...

#59 - I've not run the entire test suite with the changes above, so I have no idea how many things it would break. I think you're right and we should leave it for now, but this should maybe have a follow up issue?

Removes whitespace fails from #56

#60 - indeed. Would you mind opening the followup issue and linking it from here?

#62 - Issue created #1972702: Remove calls to theme() from template_preprocess_comment()

+++ b/core/modules/comment/templates/comment-wrapper.html.twigundefined
@@ -0,0 +1,55 @@
+ * - content: The content-related elements for the comment display. Use
+ *   {{ content }} to print them all, or print a subset such as
+ *   {{ content.comment_form }}.

Let's remove the curly braces here per http://drupal.org/node/1823416#variables-inline please, and use single quotes.

This is looking really good, thanks @shanethehat and @thedavidmeister for keeping this one moving :)

+1 for merging/deleting theme_comment_post_forbidden() but with the proposed comment_forbidden_message() in #69, what would the process of making modifications to this message be now that it's not in the theme system?

I can absolutely see a use-case for wanting to modify it. Once I had a client who wanted "post comments" changed to "leave a message".

Calls to the forbidden message are in two places:

1. comment_node_view() which can be overridden via a subsequent custom hook_node_view()
2. comment_links(), which can be overridden via hook_comment_view().

Are these hooks something we'd consider most "front end developers" capable/excited to be using to get consistent theming for comments?

My gut tells me we might be burying this message a little deep for the average site builder.

#77 - @c4rl, Unless I'm missing something, that link to your Twitter seems to be saying the same thing I was in #76?

When the only way you know to override output is the theme system, everything looks like a theme function.

Life is hard for a lot of themers I suppose. I was mostly concerned that these themers probably don't have a lot of experience with hook_entity_view() and related functions and so might not even think to look there for overriding what they might see as "regular theme markup".

The fact that the "forbidden" link happens to be put through a theme function is inconsistent with the rest of the links (which are not put through theme functions).

Luckily this reminded me that while all these links are not individually rendered by theme functions, they do all end up running through theme_links() anyway (duh), so I applied the patch in #75 and did this:

function foo_preprocess_links(&$variables) {
  if(isset($variables['links']['comment-forbidden'])) {
    $variables['links']['comment-forbidden']['title'] = 'foo';
  }
}

Everything works as you'd expect, I can override forbidden comment links through the theme system with or without theme_comment_post_forbidden(), just not inside a template (which never existed in D7 anyway). That function really is 100% redundant with other things that already exist and do the job in a better/more generalised way.

I'm going to (continue to) +1 the recommendation to kill off this theme function, but without any reservations this time :)

Glad you came around. :)

I was never really against removing it, or I would have set this to "needs work" :P I remember discussing with @jenlampton pulling it out right back when I was rolling the first half-patch in #3!

hook_entity_view() is not difficult to understand (if you're learning it in context of something you'd want to use it for) but it's certainly not most people's "go to" place for modifying markup either.

I'm personally fine with using alters where appropriate, but I really do like on a pragmatic level (whether it's intentional or pure coincidence) that themers can modify the whole comment system's front-end markup through the theme system if they really wanted to - for whatever reason that makes sense to them.

Patch review:
- I love the removal of the forbidden template
- the link in the comment-wrapper template should be changed to http://drupal.org/node/1783190
- comment markup is a mess (not our problem, see #1965650: Comment rendering broken: comment body rendered outside of the bubble and re-roll once that is resolved.

Blocked for now :(

I wondered about the way the comment looked but the markup matched up so I didn't question it. Thanks @jenlampton.

Updated the link in the @todo in comment-wrapper.html.twig and slightly tweaked it.

Described testing steps, denoted removals

Removed the todo (see #1757550-38: [Meta] Convert core theme functions to Twig templates for reasoning).

Second patch removes the change to Bartik's comment.tpl.php, it could possibly trip up the tests that use the standard profile though. If it passes, let's go with the "leave-bartik" one to avoid conflicts.

Okay, looking good. 1898054-85-leave-bartik.patch is the patch to review :)

@Cottser, you need to delete the Bartik templates completely in one of the patches you set for review if you want the comment tests to be run against the new, core Twig templates.

Remove sandbox link

Updating commit message

I suppose it's been a while, here goes.

The patch to review is still 1898054-85-leave-bartik.patch in #85 :)

Here are just the .tpl.php conversions.

See below patch for the .tpl.php conversions.

Oops, wrong patch. Ignore #92.

I'll be working on profiling this.

Nitpicks:

+++ b/core/modules/comment/templates/comment.html.twig
@@ -0,0 +1,108 @@
+ *   Preprocess functions can reformat it by calling format_date() with the
+ *   desired parameters on the $comment->created variable.
+ * - changed: Formatted date and time for when the comment was last changed.
+ *   Preprocess functions can reformat it by calling format_date() with the
+ *   desired parameters on the $comment->changed variable.

A few $'s left in the twig files, should be removed. Al la: http://drupal.org/node/1823416#variables

+++ b/core/modules/comment/templates/comment.html.twig
@@ -0,0 +1,108 @@
+    {% if signature %}
+    <div class="user-signature">
+      {{ signature }}
+    </div>
+    {% endif %}

Please indent the contents of the if block.

Here is a quick reroll with the corrections from #95.

#96: twig_comment-1898054-96.patch queued for re-testing.

Thank you @idflood!

+++ b/core/modules/comment/templates/comment.html.twigundefined
@@ -0,0 +1,108 @@
+ *   desired parameters on the comment->created variable.
...
+ *   desired parameters on the comment->changed variable.

Changes look good, but we'll need to change e.g. comment->created to 'comment.created' - add single quotes, and period instead of arrow.

Here is the patch with the correction described in #99.

removed the remaining $'s from the twig template, moved attributes to preprocess, removed some empty string settings from Cottser's diff. See how this plays...

The patch in #101 seems to be working well with the patch from #1938840: bartik.theme - Convert PHPTemplate templates to Twig.

Good point, I did test that the day before and forgot... here's the same patch with bartik template applied. Thanks @idflood

Same as #104 except only the comment files were pulled from #1938840-48: bartik.theme - Convert PHPTemplate templates to Twig

Okay, I was wrong about needing to initialize the variables. This rolls back those changes. Maybe we can gain back some performance via #1857946: Comment parent template variables are built twice.

Sorry for the delay in posting this, here are the initial profiling results based on the patch in #93.

Tested with Stark theme, 50 comments on a node page - flat comments (no threading). APC class loader enabled.

The first result is HEAD (at the time of #93) vs. itself - to show the the baseline run used for comparing runs is accurate.

The second result is HEAD vs. #93.

=== comment-head..comment-head compared (518b13c95a609..518b14dd8356b):

ct  : 189,417|189,417|0|0.0%
wt  : 675,184|675,983|799|0.1%
cpu : 647,275|647,927|652|0.1%
mu  : 14,863,352|14,863,352|0|0.0%
pmu : 15,372,952|15,372,952|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=518b13c95a609&...

=== comment-head..comment-1898054-93 compared (518b13c95a609..518b15389813f):

ct  : 189,417|194,958|5,541|2.9%
wt  : 675,184|695,459|20,275|3.0%
cpu : 647,275|667,235|19,960|3.1%
mu  : 14,863,352|14,971,816|108,464|0.7%
pmu : 15,372,952|15,580,776|207,824|1.4%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=518b13c95a609&...

The changes since #93 should improve these numbers by at least a bit (somewhere in the neighbourhood of 2.8%) but I need to re-profile and also test threaded comments.

Quick reroll now that #1990732: template_preprocess_comment() is preparing the parent's author with the wrong comment's uid is in, going to look at re-profiling this.

I'll run though the manual testing

But first another reroll

Manual testing for comment-wrapper and comment done. The only difference is the order of the attributes on the wrapper element, but all elements are present and correct.

Not testing comment block because it is a theme function so should be part of the acceptance critera for #1987396: Refactor & Clean-up comment.module theme functions

Before:

<section class="comment-wrapper" id="comments">
<h2 class="title">Comments</h2>
<a id="comment-1">
</a>
<article typeof="sioc:Post sioct:Comment" about="/comment/1#comment-1" class="comment by-node-author by-viewer clearfix">
<h3 datatype="" property="dc:title">
<a rel="bookmark" class="permalink" href="/comment/1#comment-1">Comment1 Subject</a>
</h3>
<footer>
<article about="/user/1" typeof="sioc:UserAccount" class="profile">
</article>
<p class="submitted">
<span rel="sioc:has_creator" datatype="xsd:dateTime" content="2013-05-15T23:16:42+01:00" property="dc:date dc:created">Submitted by <a lang="" datatype="" property="foaf:name" typeof="sioc:UserAccount" about="/user/1" class="username" title="View user profile." href="/user/1">admin</a> on Wed, 05/15/2013 - 23:16</span>
</p>
<a rel="bookmark" class="permalink" href="/comment/1#comment-1">Permalink</a>
</footer>
<div class="content">
<span class="rdf-meta" resource="/node/1" rel="sioc:reply_of">
</span>
<div data-edit-id="comment/1/comment_body/und/full" class="field field-name-comment-body field-type-text-long field-label-hidden edit-processed">
<div class="field-items">
<div property="content:encoded" class="field-item even">
<p>Comment 1 body</p>
</div>
</div>
</div>
</div>
<ul class="links inline">
<li class="comment-delete odd first">
<a href="/comment/1/delete">delete</a>
</li>
<li class="comment-edit even">
<a href="/comment/1/edit">edit</a>
</li>
<li class="comment-reply odd last">
<a href="/comment/reply/1/1">reply</a>
</li>
</ul>
</article>
<h2 class="title comment-form">Add new comment</h2>
<form accept-charset="UTF-8" id="comment-form" method="post" action="/comment/reply/1" class="comment-node-article-comment-form comment-form">
<div>
<div class="form-item form-type-item form-item-name" id="edit-name">
<label for="edit-name">Your name</label>
<a lang="" datatype="" property="foaf:name" typeof="sioc:UserAccount" about="/user/1" class="username" title="View user profile." href="/user/1">admin</a>
</div>
<div class="form-item form-type-textfield form-item-subject">
<label for="edit-subject">Subject</label>
<input type="text" class="form-text" maxlength="64" size="60" value="" name="subject" id="edit-subject">
</div>
<div id="edit-comment-body" class="field-type-text-long field-name-comment-body field-widget-text-textarea form-wrapper">
<div id="comment-body-add-more-wrapper">
<div class="text-format-wrapper form-item">
<div class="form-item form-type-textarea form-item-comment-body-und-0-value">
<label for="edit-comment-body-und-0-value">Comment<abbr title="This field is required." class="form-required">*</abbr>
</label>
<div class="form-textarea-wrapper">
<textarea aria-required="true" placeholder="" cols="60" rows="5" name="comment_body[und][0][value]" id="edit-comment-body-und-0-value" class="text-full form-textarea required resize-vertical" data-editor-required="true" style="visibility: hidden; display: none;">
</textarea>
<div lang="en" aria-labelledby="cke_edit-comment-body-und-0-value_arialbl" role="application" dir="ltr" class="cke_1 cke cke_reset cke_chrome cke_editor_edit-comment-body-und-0-value cke_ltr cke_browser_gecko" id="cke_edit-comment-body-und-0-value">
<span class="cke_voice_label" id="cke_edit-comment-body-und-0-value_arialbl">Rich Text Editor</span>
<div role="presentation" class="cke_inner cke_reset">
<span style="height: auto; -moz-user-select: none;" role="presentation" class="cke_top cke_reset_all" id="cke_1_top">
<span class="cke_voice_label" id="cke_5">Editor toolbars</span>
<span onmousedown="return false;" aria-labelledby="cke_5" role="group" class="cke_toolbox" id="cke_1_toolbox">
<span role="toolbar" class="cke_toolbar" id="cke_6">
<span class="cke_toolbar_start">
</span>
<span role="presentation" class="cke_toolgroup">
<a onclick="CKEDITOR.tools.callFunction(5,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(4,event);" onfocus="return CKEDITOR.tools.callFunction(3,event);" onkeydown="return CKEDITOR.tools.callFunction(2,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_7_label" role="button" hidefocus="true" tabindex="-1" title="Bold" class="cke_button cke_button__bold  cke_button_off" id="cke_7">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -32px;" class="cke_button_icon cke_button__bold_icon">&nbsp;</span>
<span class="cke_button_label cke_button__bold_label" id="cke_7_label">Bold</span>
</a>
<a onclick="CKEDITOR.tools.callFunction(9,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(8,event);" onfocus="return CKEDITOR.tools.callFunction(7,event);" onkeydown="return CKEDITOR.tools.callFunction(6,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_8_label" role="button" hidefocus="true" tabindex="-1" title="Italic" class="cke_button cke_button__italic  cke_button_off" id="cke_8">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -64px;" class="cke_button_icon cke_button__italic_icon">&nbsp;</span>
<span class="cke_button_label cke_button__italic_label" id="cke_8_label">Italic</span>
</a>
</span>
<span class="cke_toolbar_end">
</span>
</span>
<span role="toolbar" class="cke_toolbar" id="cke_9">
<span class="cke_toolbar_start">
</span>
<span role="presentation" class="cke_toolgroup">
<a onclick="CKEDITOR.tools.callFunction(13,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(12,event);" onfocus="return CKEDITOR.tools.callFunction(11,event);" onkeydown="return CKEDITOR.tools.callFunction(10,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_10_label" role="button" hidefocus="true" tabindex="-1" title="Link" class="cke_button cke_button__link  cke_button_off" id="cke_10">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -832px;" class="cke_button_icon cke_button__link_icon">&nbsp;</span>
<span class="cke_button_label cke_button__link_label" id="cke_10_label">Link</span>
</a>
<a onclick="CKEDITOR.tools.callFunction(17,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(16,event);" onfocus="return CKEDITOR.tools.callFunction(15,event);" onkeydown="return CKEDITOR.tools.callFunction(14,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_11_label" role="button" hidefocus="true" tabindex="-1" title="Unlink" class="cke_button cke_button__unlink cke_button_disabled" id="cke_11" aria-disabled="true">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -864px;" class="cke_button_icon cke_button__unlink_icon">&nbsp;</span>
<span class="cke_button_label cke_button__unlink_label" id="cke_11_label">Unlink</span>
</a>
</span>
<span class="cke_toolbar_end">
</span>
</span>
<span role="toolbar" class="cke_toolbar" id="cke_12">
<span class="cke_toolbar_start">
</span>
<span role="presentation" class="cke_toolgroup">
<a onclick="CKEDITOR.tools.callFunction(21,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(20,event);" onfocus="return CKEDITOR.tools.callFunction(19,event);" onkeydown="return CKEDITOR.tools.callFunction(18,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_13_label" role="button" hidefocus="true" tabindex="-1" title="Insert/Remove Bulleted List" class="cke_button cke_button__bulletedlist  cke_button_off" id="cke_13">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -928px;" class="cke_button_icon cke_button__bulletedlist_icon">&nbsp;</span>
<span class="cke_button_label cke_button__bulletedlist_label" id="cke_13_label">Insert/Remove Bulleted List</span>
</a>
<a onclick="CKEDITOR.tools.callFunction(25,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(24,event);" onfocus="return CKEDITOR.tools.callFunction(23,event);" onkeydown="return CKEDITOR.tools.callFunction(22,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_14_label" role="button" hidefocus="true" tabindex="-1" title="Insert/Remove Numbered List" class="cke_button cke_button__numberedlist  cke_button_off" id="cke_14">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -992px;" class="cke_button_icon cke_button__numberedlist_icon">&nbsp;</span>
<span class="cke_button_label cke_button__numberedlist_label" id="cke_14_label">Insert/Remove Numbered List</span>
</a>
</span>
<span class="cke_toolbar_end">
</span>
</span>
<span role="toolbar" class="cke_toolbar" id="cke_15">
<span class="cke_toolbar_start">
</span>
<span role="presentation" class="cke_toolgroup">
<a onclick="CKEDITOR.tools.callFunction(29,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(28,event);" onfocus="return CKEDITOR.tools.callFunction(27,event);" onkeydown="return CKEDITOR.tools.callFunction(26,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_16_label" role="button" hidefocus="true" tabindex="-1" title="Block Quote" class="cke_button cke_button__blockquote  cke_button_off" id="cke_16">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -224px;" class="cke_button_icon cke_button__blockquote_icon">&nbsp;</span>
<span class="cke_button_label cke_button__blockquote_label" id="cke_16_label">Block Quote</span>
</a>
<a onclick="CKEDITOR.tools.callFunction(33,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(32,event);" onfocus="return CKEDITOR.tools.callFunction(31,event);" onkeydown="return CKEDITOR.tools.callFunction(30,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_17_label" role="button" hidefocus="true" tabindex="-1" title="Image" class="cke_button cke_button__image  cke_button_off" id="cke_17">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -480px;" class="cke_button_icon cke_button__image_icon">&nbsp;</span>
<span class="cke_button_label cke_button__image_label" id="cke_17_label">Image</span>
</a>
</span>
<span class="cke_toolbar_end">
</span>
</span>
<span role="toolbar" class="cke_toolbar" id="cke_18">
<span class="cke_toolbar_start">
</span>
<span role="presentation" class="cke_toolgroup">
<a onclick="CKEDITOR.tools.callFunction(37,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(36,event);" onfocus="return CKEDITOR.tools.callFunction(35,event);" onkeydown="return CKEDITOR.tools.callFunction(34,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_19_label" role="button" hidefocus="true" tabindex="-1" title="Source" class="cke_button cke_button__source  cke_button_off" id="cke_19">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -1312px;" class="cke_button_icon cke_button__source_icon">&nbsp;</span>
<span class="cke_button_label cke_button__source_label" id="cke_19_label">Source</span>
</a>
</span>
<span class="cke_toolbar_end">
</span>
</span>
<span class="cke_toolbar_break">
</span>
</span>
</span>
<div role="presentation" class="cke_contents cke_reset" id="cke_1_contents" style="height: 200px;">
<span class="cke_voice_label" id="cke_26">Press ALT 0 for help</span>
<iframe frameborder="0" style="width: 100%; height: 100%;" class="cke_wysiwyg_frame cke_reset" aria-describedby="cke_26" title="Rich Text Editor,edit-comment-body-und-0-value" src="" tabindex="0" allowtransparency="true">
</iframe>
</div>
<span role="presentation" class="cke_bottom cke_reset_all" id="cke_1_bottom" style="-moz-user-select: none;">
<span onmousedown="CKEDITOR.tools.callFunction(0, event)" title="Resize" class="cke_resizer cke_resizer_vertical cke_resizer_ltr" id="cke_1_resizer">◢</span>
<span class="cke_voice_label" id="cke_1_path_label">Elements path</span>
<span aria-labelledby="cke_1_path_label" role="group" class="cke_path" id="cke_1_path">
<a aria-label="body element" role="button" onclick="CKEDITOR.tools.callFunction(38,1); return false;" onkeydown="return CKEDITOR.tools.callFunction(39,1, event );" hidefocus="true" onblur="this.style.cssText = this.style.cssText;" title="body element" class="cke_path_item" tabindex="-1" href="javascript:void('body')" id="cke_elementspath_20_1">body</a>
<a aria-label="p element" role="button" onclick="CKEDITOR.tools.callFunction(38,0); return false;" onkeydown="return CKEDITOR.tools.callFunction(39,0, event );" hidefocus="true" onblur="this.style.cssText = this.style.cssText;" title="p element" class="cke_path_item" tabindex="-1" href="javascript:void('p')" id="cke_elementspath_20_0">p</a>
<span class="cke_path_empty">&nbsp;</span>
</span>
</span>
</div>
</div>
</div>
</div>
<div id="edit-comment-body-und-0-format" class="filter-wrapper form-wrapper">
<div id="edit-comment-body-und-0-format-help" class="filter-help form-wrapper">
<p>
<a target="_blank" href="/filter/tips">More information about text formats</a>
</p>
</div>
<div class="form-item form-type-select form-item-comment-body-und-0-format">
<label for="edit-comment-body-und-0-format--2">Text format</label>
<select name="comment_body[und][0][format]" id="edit-comment-body-und-0-format--2" data-editor-for="edit-comment-body-und-0-value" class="filter-list editor form-select editor-processed">
<option selected="selected" value="basic_html">Basic HTML</option>
<option value="restricted_html">Restricted HTML</option>
<option value="full_html">Full HTML</option>
</select>
</div>
<div id="edit-comment-body-und-0-format-guidelines" class="filter-guidelines form-wrapper filter-guidelines-processed">
<div class="filter-guidelines-item filter-guidelines-basic_html" style="display: block;">
<h4 class="label" style="display: none;">Basic HTML</h4>
<ul class="tips">
<li>Allowed HTML tags: &lt;a&gt; &lt;em&gt; &lt;strong&gt; &lt;cite&gt; &lt;blockquote&gt; &lt;code&gt; &lt;ul&gt; &lt;ol&gt; &lt;li&gt; &lt;dl&gt; &lt;dt&gt; &lt;dd&gt; &lt;h4&gt; &lt;h5&gt; &lt;h6&gt; &lt;p&gt; &lt;span&gt; &lt;img&gt;</li>
<li>Only images hosted on this site may be used in &lt;img&gt; tags.</li>
</ul>
</div>
<div class="filter-guidelines-item filter-guidelines-restricted_html" style="display: none;">
<h4 class="label" style="display: none;">Restricted HTML</h4>
<ul class="tips">
<li>Allowed HTML tags: &lt;a&gt; &lt;em&gt; &lt;strong&gt; &lt;cite&gt; &lt;blockquote&gt; &lt;code&gt; &lt;ul&gt; &lt;ol&gt; &lt;li&gt; &lt;dl&gt; &lt;dt&gt; &lt;dd&gt; &lt;h4&gt; &lt;h5&gt; &lt;h6&gt;</li>
<li>Lines and paragraphs break automatically.</li>
<li>Web page addresses and e-mail addresses turn into links automatically.</li>
</ul>
</div>
<div class="filter-guidelines-item filter-guidelines-full_html" style="display: none;">
<h4 class="label" style="display: none;">Full HTML</h4>
</div>
</div>
</div>
</div>
</div>
</div>
<input type="hidden" value="form-Cj-7bi0UfLdAYkS1mG3sDVmEkcL8OLzQub5vfp_chSE" name="form_build_id">
<input type="hidden" value="1UVV3rUIGsgEbzWAl8hjBjUAYYRvu8qJV8g7d9T2cRU" name="form_token">
<input type="hidden" value="comment_node_article_comment_form" name="form_id">
<div id="edit-actions" class="form-actions form-wrapper">
<input type="submit" class="button button-primary form-submit" value="Save" name="op" id="edit-submit">
<input type="submit" class="button form-submit" value="Preview" name="op" id="edit-preview">
</div>
</div>
</form>
</section>

After:

<section id="comments" class="comment-wrapper">
<h2 class="title">Comments</h2>
<a id="comment-1">
</a>
<article typeof="sioc:Post sioct:Comment" about="/comment/1#comment-1" class="comment by-node-author by-viewer clearfix">
<h3 datatype="" property="dc:title">
<a rel="bookmark" class="permalink" href="/comment/1#comment-1">Comment1 Subject</a>
</h3>
<footer>
<article about="/user/1" typeof="sioc:UserAccount" class="profile">
</article>
<p class="submitted">
<span rel="sioc:has_creator" datatype="xsd:dateTime" content="2013-05-15T23:16:15+01:00" property="dc:date dc:created">Submitted by <a lang="" datatype="" property="foaf:name" typeof="sioc:UserAccount" about="/user/1" class="username" title="View user profile." href="/user/1">admin</a> on Wed, 05/15/2013 - 23:16</span>
</p>
<a rel="bookmark" class="permalink" href="/comment/1#comment-1">Permalink</a>
</footer>
<div class="content">
<span class="rdf-meta" resource="/node/1" rel="sioc:reply_of">
</span>
<div data-edit-id="comment/1/comment_body/und/full" class="field field-name-comment-body field-type-text-long field-label-hidden edit-processed">
<div class="field-items">
<div property="content:encoded" class="field-item even">
<p>Comment 1 body</p>
</div>
</div>
</div>
</div>
<ul class="links inline">
<li class="comment-delete odd first">
<a href="/comment/1/delete">delete</a>
</li>
<li class="comment-edit even">
<a href="/comment/1/edit">edit</a>
</li>
<li class="comment-reply odd last">
<a href="/comment/reply/1/1">reply</a>
</li>
</ul>
</article>
<h2 class="title comment-form">Add new comment</h2>
<form accept-charset="UTF-8" id="comment-form" method="post" action="/comment/reply/1" class="comment-node-article-comment-form comment-form">
<div>
<div class="form-item form-type-item form-item-name" id="edit-name">
<label for="edit-name">Your name</label>
<a lang="" datatype="" property="foaf:name" typeof="sioc:UserAccount" about="/user/1" class="username" title="View user profile." href="/user/1">admin</a>
</div>
<div class="form-item form-type-textfield form-item-subject">
<label for="edit-subject">Subject</label>
<input type="text" class="form-text" maxlength="64" size="60" value="" name="subject" id="edit-subject">
</div>
<div id="edit-comment-body" class="field-type-text-long field-name-comment-body field-widget-text-textarea form-wrapper">
<div id="comment-body-add-more-wrapper">
<div class="text-format-wrapper form-item">
<div class="form-item form-type-textarea form-item-comment-body-und-0-value">
<label for="edit-comment-body-und-0-value">Comment<abbr title="This field is required." class="form-required">*</abbr>
</label>
<div class="form-textarea-wrapper">
<textarea aria-required="true" placeholder="" cols="60" rows="5" name="comment_body[und][0][value]" id="edit-comment-body-und-0-value" class="text-full form-textarea required resize-vertical" data-editor-required="true" style="visibility: hidden; display: none;">
</textarea>
<div lang="en" aria-labelledby="cke_edit-comment-body-und-0-value_arialbl" role="application" dir="ltr" class="cke_1 cke cke_reset cke_chrome cke_editor_edit-comment-body-und-0-value cke_ltr cke_browser_gecko" id="cke_edit-comment-body-und-0-value">
<span class="cke_voice_label" id="cke_edit-comment-body-und-0-value_arialbl">Rich Text Editor</span>
<div role="presentation" class="cke_inner cke_reset">
<span style="height: auto; -moz-user-select: none;" role="presentation" class="cke_top cke_reset_all" id="cke_1_top">
<span class="cke_voice_label" id="cke_5">Editor toolbars</span>
<span onmousedown="return false;" aria-labelledby="cke_5" role="group" class="cke_toolbox" id="cke_1_toolbox">
<span role="toolbar" class="cke_toolbar" id="cke_6">
<span class="cke_toolbar_start">
</span>
<span role="presentation" class="cke_toolgroup">
<a onclick="CKEDITOR.tools.callFunction(5,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(4,event);" onfocus="return CKEDITOR.tools.callFunction(3,event);" onkeydown="return CKEDITOR.tools.callFunction(2,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_7_label" role="button" hidefocus="true" tabindex="-1" title="Bold" class="cke_button cke_button__bold  cke_button_off" id="cke_7">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -32px;" class="cke_button_icon cke_button__bold_icon">&nbsp;</span>
<span class="cke_button_label cke_button__bold_label" id="cke_7_label">Bold</span>
</a>
<a onclick="CKEDITOR.tools.callFunction(9,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(8,event);" onfocus="return CKEDITOR.tools.callFunction(7,event);" onkeydown="return CKEDITOR.tools.callFunction(6,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_8_label" role="button" hidefocus="true" tabindex="-1" title="Italic" class="cke_button cke_button__italic  cke_button_off" id="cke_8">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -64px;" class="cke_button_icon cke_button__italic_icon">&nbsp;</span>
<span class="cke_button_label cke_button__italic_label" id="cke_8_label">Italic</span>
</a>
</span>
<span class="cke_toolbar_end">
</span>
</span>
<span role="toolbar" class="cke_toolbar" id="cke_9">
<span class="cke_toolbar_start">
</span>
<span role="presentation" class="cke_toolgroup">
<a onclick="CKEDITOR.tools.callFunction(13,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(12,event);" onfocus="return CKEDITOR.tools.callFunction(11,event);" onkeydown="return CKEDITOR.tools.callFunction(10,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_10_label" role="button" hidefocus="true" tabindex="-1" title="Link" class="cke_button cke_button__link  cke_button_off" id="cke_10">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -832px;" class="cke_button_icon cke_button__link_icon">&nbsp;</span>
<span class="cke_button_label cke_button__link_label" id="cke_10_label">Link</span>
</a>
<a onclick="CKEDITOR.tools.callFunction(17,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(16,event);" onfocus="return CKEDITOR.tools.callFunction(15,event);" onkeydown="return CKEDITOR.tools.callFunction(14,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_11_label" role="button" hidefocus="true" tabindex="-1" title="Unlink" class="cke_button cke_button__unlink cke_button_disabled" id="cke_11" aria-disabled="true">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -864px;" class="cke_button_icon cke_button__unlink_icon">&nbsp;</span>
<span class="cke_button_label cke_button__unlink_label" id="cke_11_label">Unlink</span>
</a>
</span>
<span class="cke_toolbar_end">
</span>
</span>
<span role="toolbar" class="cke_toolbar" id="cke_12">
<span class="cke_toolbar_start">
</span>
<span role="presentation" class="cke_toolgroup">
<a onclick="CKEDITOR.tools.callFunction(21,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(20,event);" onfocus="return CKEDITOR.tools.callFunction(19,event);" onkeydown="return CKEDITOR.tools.callFunction(18,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_13_label" role="button" hidefocus="true" tabindex="-1" title="Insert/Remove Bulleted List" class="cke_button cke_button__bulletedlist  cke_button_off" id="cke_13">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -928px;" class="cke_button_icon cke_button__bulletedlist_icon">&nbsp;</span>
<span class="cke_button_label cke_button__bulletedlist_label" id="cke_13_label">Insert/Remove Bulleted List</span>
</a>
<a onclick="CKEDITOR.tools.callFunction(25,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(24,event);" onfocus="return CKEDITOR.tools.callFunction(23,event);" onkeydown="return CKEDITOR.tools.callFunction(22,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_14_label" role="button" hidefocus="true" tabindex="-1" title="Insert/Remove Numbered List" class="cke_button cke_button__numberedlist  cke_button_off" id="cke_14">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -992px;" class="cke_button_icon cke_button__numberedlist_icon">&nbsp;</span>
<span class="cke_button_label cke_button__numberedlist_label" id="cke_14_label">Insert/Remove Numbered List</span>
</a>
</span>
<span class="cke_toolbar_end">
</span>
</span>
<span role="toolbar" class="cke_toolbar" id="cke_15">
<span class="cke_toolbar_start">
</span>
<span role="presentation" class="cke_toolgroup">
<a onclick="CKEDITOR.tools.callFunction(29,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(28,event);" onfocus="return CKEDITOR.tools.callFunction(27,event);" onkeydown="return CKEDITOR.tools.callFunction(26,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_16_label" role="button" hidefocus="true" tabindex="-1" title="Block Quote" class="cke_button cke_button__blockquote  cke_button_off" id="cke_16">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -224px;" class="cke_button_icon cke_button__blockquote_icon">&nbsp;</span>
<span class="cke_button_label cke_button__blockquote_label" id="cke_16_label">Block Quote</span>
</a>
<a onclick="CKEDITOR.tools.callFunction(33,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(32,event);" onfocus="return CKEDITOR.tools.callFunction(31,event);" onkeydown="return CKEDITOR.tools.callFunction(30,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_17_label" role="button" hidefocus="true" tabindex="-1" title="Image" class="cke_button cke_button__image  cke_button_off" id="cke_17">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -480px;" class="cke_button_icon cke_button__image_icon">&nbsp;</span>
<span class="cke_button_label cke_button__image_label" id="cke_17_label">Image</span>
</a>
</span>
<span class="cke_toolbar_end">
</span>
</span>
<span role="toolbar" class="cke_toolbar" id="cke_18">
<span class="cke_toolbar_start">
</span>
<span role="presentation" class="cke_toolgroup">
<a onclick="CKEDITOR.tools.callFunction(37,this);return false;" onmousedown="return CKEDITOR.tools.callFunction(36,event);" onfocus="return CKEDITOR.tools.callFunction(35,event);" onkeydown="return CKEDITOR.tools.callFunction(34,event);" onblur="this.style.cssText = this.style.cssText;" aria-haspopup="false" aria-labelledby="cke_19_label" role="button" hidefocus="true" tabindex="-1" title="Source" class="cke_button cke_button__source  cke_button_off" id="cke_19">
<span style="background-image:url(http://local.drupal8/core/misc/ckeditor/plugins/icons.png?t=D2MB);background-position:0 -1312px;" class="cke_button_icon cke_button__source_icon">&nbsp;</span>
<span class="cke_button_label cke_button__source_label" id="cke_19_label">Source</span>
</a>
</span>
<span class="cke_toolbar_end">
</span>
</span>
<span class="cke_toolbar_break">
</span>
</span>
</span>
<div role="presentation" class="cke_contents cke_reset" id="cke_1_contents" style="height: 200px;">
<span class="cke_voice_label" id="cke_26">Press ALT 0 for help</span>
<iframe frameborder="0" style="width: 100%; height: 100%;" class="cke_wysiwyg_frame cke_reset" aria-describedby="cke_26" title="Rich Text Editor,edit-comment-body-und-0-value" src="" tabindex="0" allowtransparency="true">
</iframe>
</div>
<span role="presentation" class="cke_bottom cke_reset_all" id="cke_1_bottom" style="-moz-user-select: none;">
<span onmousedown="CKEDITOR.tools.callFunction(0, event)" title="Resize" class="cke_resizer cke_resizer_vertical cke_resizer_ltr" id="cke_1_resizer">◢</span>
<span class="cke_voice_label" id="cke_1_path_label">Elements path</span>
<span aria-labelledby="cke_1_path_label" role="group" class="cke_path" id="cke_1_path">
<a aria-label="body element" role="button" onclick="CKEDITOR.tools.callFunction(38,1); return false;" onkeydown="return CKEDITOR.tools.callFunction(39,1, event );" hidefocus="true" onblur="this.style.cssText = this.style.cssText;" title="body element" class="cke_path_item" tabindex="-1" href="javascript:void('body')" id="cke_elementspath_20_1">body</a>
<a aria-label="p element" role="button" onclick="CKEDITOR.tools.callFunction(38,0); return false;" onkeydown="return CKEDITOR.tools.callFunction(39,0, event );" hidefocus="true" onblur="this.style.cssText = this.style.cssText;" title="p element" class="cke_path_item" tabindex="-1" href="javascript:void('p')" id="cke_elementspath_20_0">p</a>
<span class="cke_path_empty">&nbsp;</span>
</span>
</span>
</div>
</div>
</div>
</div>
<div id="edit-comment-body-und-0-format" class="filter-wrapper form-wrapper">
<div id="edit-comment-body-und-0-format-help" class="filter-help form-wrapper">
<p>
<a target="_blank" href="/filter/tips">More information about text formats</a>
</p>
</div>
<div class="form-item form-type-select form-item-comment-body-und-0-format">
<label for="edit-comment-body-und-0-format--2">Text format</label>
<select name="comment_body[und][0][format]" id="edit-comment-body-und-0-format--2" data-editor-for="edit-comment-body-und-0-value" class="filter-list editor form-select editor-processed">
<option selected="selected" value="basic_html">Basic HTML</option>
<option value="restricted_html">Restricted HTML</option>
<option value="full_html">Full HTML</option>
</select>
</div>
<div id="edit-comment-body-und-0-format-guidelines" class="filter-guidelines form-wrapper filter-guidelines-processed">
<div class="filter-guidelines-item filter-guidelines-basic_html" style="display: block;">
<h4 class="label" style="display: none;">Basic HTML</h4>
<ul class="tips">
<li>Allowed HTML tags: &lt;a&gt; &lt;em&gt; &lt;strong&gt; &lt;cite&gt; &lt;blockquote&gt; &lt;code&gt; &lt;ul&gt; &lt;ol&gt; &lt;li&gt; &lt;dl&gt; &lt;dt&gt; &lt;dd&gt; &lt;h4&gt; &lt;h5&gt; &lt;h6&gt; &lt;p&gt; &lt;span&gt; &lt;img&gt;</li>
<li>Only images hosted on this site may be used in &lt;img&gt; tags.</li>
</ul>
</div>
<div class="filter-guidelines-item filter-guidelines-restricted_html" style="display: none;">
<h4 class="label" style="display: none;">Restricted HTML</h4>
<ul class="tips">
<li>Allowed HTML tags: &lt;a&gt; &lt;em&gt; &lt;strong&gt; &lt;cite&gt; &lt;blockquote&gt; &lt;code&gt; &lt;ul&gt; &lt;ol&gt; &lt;li&gt; &lt;dl&gt; &lt;dt&gt; &lt;dd&gt; &lt;h4&gt; &lt;h5&gt; &lt;h6&gt;</li>
<li>Lines and paragraphs break automatically.</li>
<li>Web page addresses and e-mail addresses turn into links automatically.</li>
</ul>
</div>
<div class="filter-guidelines-item filter-guidelines-full_html" style="display: none;">
<h4 class="label" style="display: none;">Full HTML</h4>
</div>
</div>
</div>
</div>
</div>
</div>
<input type="hidden" value="form-LSK2-Hh5ewdoNmEDsmT6WK102Inp-qi2m8mO3r4xZiU" name="form_build_id">
<input type="hidden" value="kSytH424u4pHK-5x_krA12liXRiTl0wVfZu0O1FJNJk" name="form_token">
<input type="hidden" value="comment_node_article_comment_form" name="form_id">
<div id="edit-actions" class="form-actions form-wrapper">
<input type="submit" class="button button-primary form-submit" value="Save" name="op" id="edit-submit">
<input type="submit" class="button form-submit" value="Preview" name="op" id="edit-preview">
</div>
</div>
</form>
</section>

This corrects the section tag attributes order.

<section id="comments" class="comment-wrapper">

I think this is good to go, after profiling.

Updated issue summary.

Adding a change to the bartik theme that removes a trailing space from a class list. See #1938840: bartik.theme - Convert PHPTemplate templates to Twig #52-3

I think removing Bartik's templates from here should be fine now, we shouldn't be converting those in two places. Ran comment preview and user picture tests locally and both passed, so hopefully nothing else breaks. I should have probably removed them back in #107.

Unassigning because I'm not sure when I'll get back to profiling this.

Scenario #1: Using Stark, devel generate a dozen or so nodes and set the "Maximum number of comments per node" to 30. Then set site front page to node with most comments (also, edit node comment settings to "open"). Comparing 8.x (6718550c) to #115.

=== 8.x..8.x compared (5199dd4798f9f..5199de96067c8):

ct  : 143,205|143,205|0|0.0%
wt  : 1,070,345|1,071,389|1,044|0.1%
cpu : 1,064,067|1,064,066|-1|-0.0%
mu  : 7,776,504|7,776,504|0|0.0%
pmu : 8,058,452|8,058,452|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5199dd4798f9f&...

=== 8.x..comment-1898054-115 compared (5199dd4798f9f..5199df04dd672):

ct  : 143,205|146,004|2,799|2.0%
wt  : 1,070,345|1,094,153|23,808|2.2%
cpu : 1,064,067|1,092,069|28,002|2.6%
mu  : 7,776,504|7,832,892|56,388|0.7%
pmu : 8,058,452|8,114,632|56,180|0.7%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5199dd4798f9f&...

Scenario #2: Using Stark, create a page view of the nodes created in scenario #1 (node with comments). Set site default front page. Again, comparing 8.x (6718550c) to #115.

=== 8.x..8.x compared (5199e3a2bf6f6..5199e51c1ee20):

ct  : 223,105|223,105|0|0.0%
wt  : 1,653,856|1,653,213|-643|-0.0%
cpu : 1,648,103|1,644,103|-4,000|-0.2%
mu  : 10,075,840|10,075,840|0|0.0%
pmu : 10,525,656|10,525,656|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5199e3a2bf6f6&...

=== 8.x..comment-1898054-115 compared (5199e3a2bf6f6..5199e5a1880e2):

ct  : 223,105|226,808|3,703|1.7%
wt  : 1,653,856|1,681,454|27,598|1.7%
cpu : 1,648,103|1,676,104|28,001|1.7%
mu  : 10,075,840|10,133,580|57,740|0.6%
pmu : 10,525,656|10,585,052|59,396|0.6%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5199e3a2bf6f6&...

2% slower? that's pretty bad. Does anyone have any idea why it would be so much slower because I don't see anything obvious in the patch..

Yes, I can explain:

It is the getContextReference that comes to haunt me now ...

Compare:

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5199e3a2bf6f6&...

with

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5199e3a2bf6f6&...

These are 16 ms, which are splitted like:

- 7.5 ms getContextReference
- 5 ms getAttribute - of that are 2ms TwigReference::offsetGet
- around 3.5 ms internal overhead for twig_render_var

So I think we need to live with around 2%.

I'm turning this over for an Alex Pott review since it's been profiled and we just need a sign-off.

@ezeedub how many comments were on the node that you profiled? Looks like less than 30, whereas for example this issue has 120.

@Fabian am I reading your diff right that you profiled with 37 comments? That'd mean 0.5ms overhead per comment which seems a lot -i.e. 150ms for 300 comments. Also wondering whether any of this is specific to the comment conversion or whether we'll see the same overhead with other templates that are called a lot during the same page.

Reading through getContextReference() it's not doing much expensive it's just doing it 465 times...

Only potential micro-optimization I could see for now was:

    if ($this->twig_reference == NULL) {
      $this->twig_reference = new TwigReference();
    }

What about just setting $this->twig_reference in __construct()? If it's not used then one call gets wasted during a request, but otherwise that's 400+ comparisons saved.

Also on the patch itself:

  $variables['attributes']['id'] = 'comments';
+
   // Add a comment wrapper class.
   $variables['attributes']['class'][] = 'comment-wrapper';
+
+  // Create separate variables for the comments and comment form.
+  $variables['comments'] = $variables['content']['comments'];
+  $variables['form'] = $variables['content']['comment_form'];
 }

Why the extra IDs/classes?

+  {% if comments and node.type != 'forum' %}
+    {{ title_prefix }}
+    <h2 class="title">{{ 'Comments'|t }}</h2>
+    {{ title_suffix }}
+  {% endif %}
+
+  {{ comments }}

Forum module shouldn't really be referenced by the core template, also forum module allows any node type to be placed in forums so this'd be inconsistent if a site has a couple of different node types used. edit: I see this was already in the template and just moved, but we could just drop that here IMO.

#121 - @catch, checkout #1285842: Forum module should implement its own node and comment templates to make theming easier for the forum issue. Not sure that the conversion task should be taking that responsibility.

#120 -

Also wondering whether any of this is specific to the comment conversion or whether we'll see the same overhead with other templates that are called a lot during the same page.

I think the profiling here #1843746: Convert views/templates/views-view-field.tpl.php to Twig is relevant to that question.

Could we actually profile 300 comments to see if it adds 150ms as per #120?

Can we please also test @catch's idea in #120 for a potential optimization here (and for other commonly used templates)?

From #121:

Why the extra IDs/classes?

We are not adding anything here, it is just a straight conversion :)

The second point was addressed in #123, that is out of scope for this direct conversion.

Profiling is nice but actually micro-optimizing at this point wouldn't really be worth delaying this further IMO. Setting to RTBC.

Edit: I did roll the last patch but it was just removing code and @Fabianx gave this a +1 :)

+1 from me as well.

+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch

We have a follow-up to address performance #2002094: Improve performance of comment.html.twig

Committed 5f9bee7 and pushed to 8.x. Thanks!