Move classes out of the preprocess functions and into the Twig templates. Use the addClass() attribute method to add classes in the template. Use the clean_class filter to filter class names, if necessary. Maintain all existing functionality and ensure all existing class names are still in the markup, even ones that are inherited.

See the following issues for more detailed examples:
#2217731: Move field classes out of preprocess and into templates
#2254153: Move node classes out of preprocess and into templates

See this change record for information about using the addClass() method:
https://www.drupal.org/node/2315471

See this change record for more information about the phase 1 process of moving class from preprocess to templates:
https://www.drupal.org/node/2325067

Preprocess Functions Modified

template_preprocess_comment

Twig Templates Modified

comment.html.twig

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

davidhernandez’s picture

Issue tags: +FUDK
lauriii’s picture

Assigned: Unassigned » lauriii
lauriii’s picture

Assigned: lauriii » Unassigned
Status: Active » Needs review
FileSize
4.3 KB
lauriii’s picture

FileSize
1.57 KB

Added vocabulary_name to twig comments

star-szr’s picture

[redacted and moved to the proper issue]

star-szr’s picture

lauriii’s picture

FileSize
1.33 KB

Ups, posted my patch to wrong issue :D Ignore patch in comment #4

lauriii’s picture

Where did that file come from?

RainbowArray’s picture

Is there a way to hide or delete the patches in #4 and #7? It's very confusing.

star-szr’s picture

Re-adding #3 to the issue summary. d.o ghosts--

star-szr’s picture

Status: Needs review » Needs work

This is looking really good, just a couple red spots jumped out at me:

+++ b/core/modules/comment/templates/comment.html.twig
@@ -64,7 +64,17 @@
+    status != 'preview' ? 'permalink', ¶

+++ b/core/themes/bartik/templates/comment.html.twig
@@ -62,7 +62,17 @@
+    status != 'preview' ? 'permalink', ¶

Trailing whitespace, noooo!

lauriii’s picture

Status: Needs work » Needs review
FileSize
4.29 KB
1.02 KB

There, I Fixed It!

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

I can't find any fault here, looks good. Thanks @lauriii!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: move_comment_classes-2329783-12.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
star-szr’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/comment.module
@@ -721,6 +721,7 @@ function template_preprocess_comment(&$variables) {
+  $variables['author_id'] = $comment->getOwnerId();

@@ -813,28 +814,13 @@ function template_preprocess_comment(&$variables) {
-    if ($commented_entity instanceof EntityOwnerInterface && $comment->getOwnerId() == $commented_entity->getOwnerId()) {
-      $variables['attributes']['class'][] = 'by-' . $commented_entity->getEntityTypeId() . '-author';
-    }

+++ b/core/modules/comment/templates/comment.html.twig
@@ -64,7 +64,17 @@
+    author_id ? 'by-' ~ entity_type ~ '-author',

Sorry I missed this before. Checking for any comment author ID is not the same as checking that this ID is the same as the node author ID. So that needs updating.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.88 KB
4.14 KB

As I discussed on IRC with @Cottser I fixed and moved functionality he mentioned on his comment #17.

Status: Needs review » Needs work

The last submitted patch, 18: move_comment_classes-2329783-18.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
4.17 KB
1.17 KB

Fixed tests

Status: Needs review » Needs work

The last submitted patch, 20: move_comment_classes-2329783-20.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
4.14 KB

Reroll

guciek27’s picture

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

The patch looks good, but there is one more class that is used.

file: comment.module
line: 765

$attributes = $permalink_uri_parent->getOption('attributes') ?: array();
    $attributes += array('class' => array('permalink'), 'rel' => 'bookmark');
    $permalink_uri_parent->setOption('attributes', $attributes)
lauriii’s picture

Status: Needs work » Needs review

Link is generated in the preprocess so we cannot move that to template. Putting back to needs review.

lauriii’s picture

Issue tags: +sprint
BarisW’s picture

Code looks good and applies nicely.
To test it, I've copied the HTML of a comment before and after the patch and ran a diff. No changes in the HTML :)

+1 from me.

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

+1 RTBC

LewisNyman’s picture

+1 RTBC

Tested in Stark, screenshot attached.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 23: move_comment_classes-2329783-23.patch, failed testing.

lauriii’s picture

Status: Needs work » Reviewed & tested by the community

Putting back to RTBC

webchick’s picture

So just a small process check question... should these base classes with all the classes defined not be going into themes/classy these days, and the module's template file basically class-less?

lauriii’s picture

This issue is only for moving classes to templates. There is phase 2 where we will move templates with classes to classy and remove classes from module templates.

star-szr’s picture

Here is the phase 2 issue.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/comment/comment.module
@@ -733,7 +734,7 @@ function template_preprocess_comment(&$variables) {
-    $attributes += array('class' => array('permalink'), 'rel' => 'bookmark');
+    $attributes += array('rel' => 'bookmark');
     $uri->setOption('attributes', $attributes);
     $variables['title'] = \Drupal::l($comment->getSubject(), $uri);

+++ b/core/modules/comment/templates/comment.html.twig
@@ -64,7 +64,17 @@
+    status != 'preview' ? 'permalink',

Doesn't this mean that the permalink class is no longer on the title but on the entire article? Yep manual testing reveals unexpected differences in the html.

diff head.html patch.html
1c1
< <article class="comment by-node-author clearfix clearfix by-viewer new quickedit-processed" role="article" data-quickedit-entity-id="comment/1" data-comment-user-id="1" about="/comment/1" typeof="schema:Comment" data-quickedit-entity-instance-id="0">
---
> <article role="article" data-quickedit-entity-id="comment/1" data-comment-user-id="1" about="/comment/1" typeof="schema:Comment" class="comment permalink by-node-author clearfix by-viewer quickedit-processed" data-quickedit-entity-instance-id="0">
30c30
<       <h3 property="schema:name" datatype=""><a href="/comment/1" class="permalink" rel="bookmark">A test comment</a></h3>
---
>       <h3 property="schema:name" datatype=""><a href="/comment/1" rel="bookmark">A test comment</a></h3>
lauriii’s picture

Status: Needs work » Needs review
FileSize
1.54 KB
3.65 KB

Good catch!

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

I diffed the output in Bartik as Alex did and found no different apart from the position of the classes, we also removed a duplicate clearfix class. That's good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/comment/comment.module
@@ -792,28 +793,8 @@ function template_preprocess_comment(&$variables) {
-    if ($commented_entity instanceof EntityOwnerInterface && $comment->getOwnerId() == $commented_entity->getOwnerId()) {

+++ b/core/modules/comment/templates/comment.html.twig
@@ -64,7 +64,16 @@
+    author_id and author_id == commented_entity.getOwnerId() ? 'by-' ~ commented_entity.getEntityTypeId() ~ '-author',

+++ b/core/themes/bartik/templates/comment.html.twig
@@ -62,7 +62,16 @@
+    author_id and author_id == commented_entity.getOwnerId() ? 'by-' ~ commented_entity.getEntityTypeId() ~ '-author',

I think this will break for comments on a user since a user does not implement EntityOwnerInterface. I think we need to add something like commented_entity.getOwnerId is defined in the condition. As far as I can see we have no tests for adding a comment field to an entity type that does not implement EntityOwnerInterface. We should get a followup to do just that.

jamesquinton’s picture

Status: Needs work » Needs review
FileSize
1.3 KB
3.74 KB

Added commented_entity.getOwnerId is defined.

lauriii’s picture

Good work on the patch! #40 fixes #39. More info about is defined in the Twig Documentation.

joelpittet’s picture

@alexpott that is defined is quite useless it seems with strict variables turned off like we have.

Here's a quick test you can try:

require_once 'vendor/autoload.php';

$twig_options = [
  'strict_variables' => FALSE,
];

$loader = new Twig_Loader_String();
$twig = new Twig_Environment($loader, $twig_options);
$twig->addExtension(new Twig_Extension_Debug());
class HasNoMethod {
  function real() {
    return TRUE;
  }
}
echo $twig->render(
  "{{ HasNoMethod.missing() ? 'true' : 'false' }} : {{ HasNoMethod.real() ? 'true' : 'false' }}",
  ['HasNoMethod' => new HasNoMethod]
);

Output is false : true

This is one of the reasons we chose to set strict_variables to FALSE in core.

If you turn strict_variables on... then you get:
Fatal error: Uncaught exception 'Twig_Error_Runtime' with message 'Method "missing" for object "HasNoMethod" does not exist in ...

davidhernandez’s picture

So...should we stick to the patch in #37 then?

joelpittet’s picture

Yes, I think we should stick with #37

lauriii’s picture

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

So as it seems #37 patch (#2329783-37: Move comment classes from preprocess to templates) is green, let's go back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a unfrozen change (markup) as per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?. Committed e3e7044 and pushed to 8.0.x. Thanks!

  • alexpott committed e3e7044 on 8.0.x
    Issue #2329783 by lauriii, jamesquinton | davidhernandez: Move comment...

Status: Fixed » Needs work

The last submitted patch, 40: move_comment_classes-2329783-40.patch, failed testing.

lauriii’s picture

Status: Needs work » Fixed

Putting back to fixed.

Wim Leers’s picture

+++ b/core/modules/comment/comment.module
@@ -700,6 +700,7 @@ function template_preprocess_comment(&$variables) {
+  $variables['author_id'] = $comment->getOwnerId();

+++ b/core/modules/comment/templates/comment.html.twig
@@ -64,7 +64,16 @@
+    not author_id ? 'by-anonymous',

This perpetuates the "user ID zero is the anonymous user" check which IMHO makes no sense.

This should've used $comment->getOwner->isAnonymous().

Fabianx’s picture

Lets file a follow-up for that, because this issue was a straight port from the preprocess code.

joelpittet’s picture

@Wim Leers and @Fabianx created follow-up feel free to update the IS:
#2372909: Comments to check '$comment->getOwner->isAnonymous()' instead of assuming anonymous is ID 0

Status: Fixed » Closed (fixed)

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