Updated: Comment #9

Problem/Motivation

Currently comment list is hardcoded into custom template comment-wrapper with according preprocess function.
It's unclear which template should developer use - field or wrapper, both templates lacks some useful properties.

Proposed resolution

Use a field__comment template suggestion to replace custom theme template.
Use field instance settings and template to render wrapper for comment list and form.

Remaining tasks

review theme suggestion and twig usage
optimize the render

User interface changes

no, just a markup
Before

After

API changes

no

Original report by @andypost

There's template conversion happens in #1898054: comment.module - Convert PHPTemplate templates to Twig

Todo:
1) Make changes in preprocess
2) follow template changes:
2.1) preprocess variables in @file doc-block
2.2) Replace <h2 class="title"><?php print t('Comments'); ?></h2> with field name

Comments

InternetDevels’s picture

FileSize
2.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch comment-fix_twig_conversion-1962846-1.patch. Unable to apply patch. See the log in the details link for more information. View

Just a first try.

andypost’s picture

Title: make changes to php.tpl files and follow twig conversion » Use field instance name for header of comment list
andypost’s picture

Project: Comment as field » Drupal core
Version: » 8.x-dev
Component: Code » comment.module
andypost’s picture

It looks like we need 2 settings:
1) label for comment list
2) label to add new item to that list

andypost’s picture

Related #1987396: Refactor & Clean-up comment.module theme functions
could remove comment-wrapper template so following

rteijeiro’s picture

Assigned: Unassigned » rteijeiro
Issue summary: View changes

Let's try this one :)

catch’s picture

Removing the comment wrapper template sounds good.

andypost’s picture

Status: Active » Needs review
Issue tags: +Twig
FileSize
6.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,941 pass(es), 5 fail(s), and 0 exception(s). View

Initial stub

Converts template for contextual field template

andypost’s picture

Title: Use field instance name for header of comment list » Use field instance name for header of comment list, drop comment-wrapper template
Issue summary: View changes
Issue tags: +DX (Developer Experience)
Related issues: +#2031883: Markup for: comment.html.twig

proper title and summary
ps: related #2031883: Markup for: comment.html.twig

markcarver’s picture

First, this is awesome!! Definitely +1 (this is how the theme system should be used in the first place). A few notes from briefly scanning the patch in #8:

  1. +++ b/core/modules/comment/comment.module
    @@ -157,9 +157,8 @@ function comment_theme() {
    -    'comment_wrapper' => array(
    -      'render element' => 'content',
    -      'template' => 'comment-wrapper',
    +    'field__comment' => array(
    +      'template' => 'field--comment',
    

    This shouldn't be necessary. Theme registry detects template suggestions and adds the necessary theme suggestion hook referring to the base hook (in this case: field). See: drupal_find_theme_templates()

  2. +++ b/core/modules/comment/comment.module
    @@ -1424,31 +1423,33 @@ function template_preprocess_comment(&$variables) {
    +function template_preprocess_field__comment(&$variables) {
    

    This, unfortunately, does not yet currently work (see parent issue). This is a perfect use case example of why we need preprocessing of theme hook suggestions put back in core.

  3. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
    @@ -176,7 +176,7 @@ public function viewElements(FieldItemListInterface $items) {
    -        '#theme' => 'comment_wrapper__' . $entity->getEntityTypeId() . '__' . $entity->bundle() . '__' . $field_name,
    +        '#theme' => 'field__comment',
    

    We should pass an array here instead:

    '#theme' => array(
      'field__comment__' . $entity->getEntityTypeId() . '__' . $entity->bundle() . '__' . $field_name,
      'field__comment__' . $entity->getEntityTypeId() . '__' . $entity->bundle(),
      'field__comment__' . $entity->getEntityTypeId(),
      'field__comment',
      'field',
    ),
    

    That being said, doesn't $entity->getEntityTypeId() === 'comment'? If so, shouldn't it really just be:

    '#theme' => array(
      'field__' . $entity->getEntityTypeId() . '__' . $entity->bundle() . '__' . $field_name,
      'field__' . $entity->getEntityTypeId() . '__' . $entity->bundle(),
      'field__' . $entity->getEntityTypeId(),
      'field',
    ),
    

The last submitted patch, 8: 1962846-comment-wrapper-8.patch, failed testing.

andypost’s picture

@Mark Carver Thanks a lot for review, I stuked with preprocess, because without theme hook definitions does not work and suggestion template does not work.
$entity in formatter is a any entity where comment field is attached.

3) Also it works with commented out #theme but system_theme_suggestions_field() is wrong now

function system_theme_suggestions_field(array $variables) {
  $suggestions = array();
  $element = $variables['element'];

  $suggestions[] = 'field__' . $element['#field_type'];
  $suggestions[] = 'field__' . $element['#field_name'];
...
  return $suggestions;
}

So if someone will create field named comment (or will use "comment" as base entity field) then theme function will be confused.
Because both "type" and "name" could be same. So filed #2226233-22: Redo changes from field.module to Twig conversion issue that were clobbered

andypost’s picture

Status: Needs work » Needs review
FileSize
2.27 KB
7.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,449 pass(es), 357 fail(s), and 19 exception(s). View

Fixed #10 1) and 2)

Status: Needs review » Needs work

The last submitted patch, 13: 1962846-comment-wrapper-12.patch, failed testing.

Wim Leers’s picture

+++ b/core/modules/system/system.module
@@ -706,7 +706,6 @@ function system_theme_suggestions_field(array $variables) {
-  $suggestions[] = 'field__' . $element['#field_name'];
...
   $suggestions[] = 'field__' . $element['#entity_type'] . '__' . $element['#field_name'];

Good catch with deleting that one. Instead, from now on the one on the second line I cited should always be used.

markcarver’s picture

  1. +++ b/core/modules/comment/comment.module
    @@ -157,9 +157,9 @@ function comment_theme() {
    -    'comment_wrapper' => array(
    -      'render element' => 'content',
    -      'template' => 'comment-wrapper',
    +    'field__comment' => array(
    +      'template' => 'field--comment',
    +      'base hook' => 'field',
    
    @@ -1424,31 +1424,33 @@ function template_preprocess_comment(&$variables) {
    +function template_preprocess_field__comment(&$variables) {
    

    I get that it works if we manually put this in the hook_theme definition.

    That isn't the point nor is this "Best Practices™".

    We shouldn't be putting suggestions inside hook_theme definitions. Let alone a "base hook". These are supposed to be detected automatically.

    This is a perfect example of how broken the theme system is. The parent issue will fix the detection of preprocess suggestion functions.

  2. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
    @@ -176,7 +176,14 @@ public function viewElements(FieldItemListInterface $items) {
    +          'field__comment__' . $entity->getEntityTypeId(),
    +          'field__comment',
    +          'field',
    

    Again, doesn't $entity->getEntityTypeId() === 'comment'? This seems redundant:

    field__comment__comment

    edit: Furthermore, these actually aren't needed. I wasn't aware that system_theme_suggestions_field() existed. A simple '#theme' => 'field' will suffice here.

  3. +++ b/core/modules/system/system.module
    +++ b/core/modules/system/system.module
    @@ -706,7 +706,6 @@ function system_theme_suggestions_field(array $variables) {
    
    @@ -706,7 +706,6 @@ function system_theme_suggestions_field(array $variables) {
    -  $suggestions[] = 'field__' . $element['#field_name'];
    

    First, separate issue, but worth mentioning that this should be the field module that's providing these, not "system".

    Second, this shouldn't be taken out. Fields are reusable across entity types. Consider:

    1. You have a field named: auto_upload_file.
    2. You want to theme this field the same for every entity that uses it.

    Would you rather:

    1. Create a template once for all entities that use this field (to keep things consistent): field--auto-upload-field.html.twig
    2. Or duplicate this for every entity that uses it: field--node--auto-upload-field.html.twig, field--comment--auto-upload-field.html.twig, field--user--auto-upload-field.html.twig, field--message--auto-upload-field.html.twig

edit: I understand there's a naming conflict, but this is specific to this issue. The solution is simple: name the field something that isn't the entity (like: "comments").

andypost’s picture

Status: Needs work » Needs review
Issue tags: +theme system cleanup

taggin

The last submitted patch, 1: comment-fix_twig_conversion-1962846-1.patch, failed testing.

markcarver’s picture

Status: Needs review » Needs work
markcarver’s picture

Assigned: rteijeiro » Unassigned
andypost’s picture

@Mark seems this one should be postponed on #939462: Specific preprocess functions for theme hook suggestions are not invoked
Without special preprocess functions we can't move forward

markcarver’s picture

Status: Needs work » Postponed

Correct

andypost’s picture

edit: I understand there's a naming conflict, but this is specific to this issue. The solution is simple: name the field something that isn't the entity (like: "comments").

this does not work now, because there's a lot of base fields now available so at least this dimension needs separate prefix
$suggestions[] = 'field__field_' . $element['#field_name'];

at the same time we could use some kind of "context" (plugin bag) to populate preprocess function name suggestions, nearly the same we have on core token registry

andypost’s picture

There's more node fields have own (separate) templates now.
I think better to fix the issue with @todo linked to parent issue

andypost’s picture

Status: Postponed » Needs review
FileSize
7.65 KB
8.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,357 pass(es), 5 fail(s), and 0 exception(s). View

I see no actual reason to postpone the issue anymore, so another approach

Status: Needs review » Needs work

The last submitted patch, 27: 1962846-comment-wrapper-27.patch, failed testing.

andypost’s picture

5 failures shows that now we see "Comment settings" as field named!
Here is a question - is there any ability to provide different label for widget and formatter?

andypost’s picture

Status: Needs work » Needs review
FileSize
2.76 KB
11.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,174 pass(es), 3 fail(s), and 0 exception(s). View

Suppose better to tune the widget title

Status: Needs review » Needs work

The last submitted patch, 30: 1962846-comment-wrapper-30.patch, failed testing.

andypost’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.71 KB
12.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,183 pass(es). View
58.7 KB
54.64 KB

Should fix remaining tests.
RDF attributes are added to each field item, so I found only this way to merge first item attributes to main attributes

So the markup change is

Before

After

andypost’s picture

FileSize
12.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,239 pass(es). View
Wim Leers’s picture

Status: Needs review » Needs work

Awesome! Thanks :) I could only find nitpicks:

  1. +++ b/core/modules/comment/comment.module
    @@ -1207,31 +1207,37 @@ function template_preprocess_comment(&$variables) {
    +    // Adjust a comment field special variables.
    

    Broken sentence :)

  2. +++ b/core/modules/comment/comment.module
    @@ -1207,31 +1207,37 @@ function template_preprocess_comment(&$variables) {
    +    // Append additional attributes (eg. rdf) from the first field item.
    

    s/rdf/RDFa/

  3. +++ b/core/modules/comment/src/CommentManager.php
    @@ -212,7 +212,7 @@ public function addDefaultField($entity_type, $bundle, $field_name = 'comment',
    -          'label' => 'hidden',
    +          'label' => 'above',
    

    Why?

    Is this a change in behavior, or is this a change to reflect the actual behavior?

  4. +++ b/core/modules/comment/templates/field--comment.html.twig
    @@ -0,0 +1,37 @@
    + * - comments: List of comments rendered through comment.html.twig.
    + * - content_attributes: HTML attributes for the form title.
    + * - comment_form: The 'Add new comment' form.
    

    This should also list comment_display_mode and comment_type?

andypost’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
12.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,478 pass(es). View

Fixed #34 (1,2,4)
@Wim #34.3 is needed to set proper label placement for comment thread title - comment field label is used for.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
12.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1962846-comment-wrapper-36.patch. Unable to apply patch. See the log in the details link for more information. View
720 bytes

#35, thanks, manually confirmed your answer for #34.3. With all that out of the way, I can RTBC this.

(I'm only fixing a tiny typo introduced in #35 in this reroll, so we don't have to wait for another reroll/retest cycle.)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 36: 1962846-comment-wrapper-36.patch, failed testing.

andypost’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 36: 1962846-comment-wrapper-36.patch, failed testing.

andypost’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
12.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,176 pass(es). View

re-roll

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 49f4fdd and pushed to 8.x. Thanks!

  • alexpott committed 49f4fdd on
    Issue #1962846 by andypost, Wim Leers, InternetDevels: Use field...
markcarver’s picture

+++ b/core/modules/comment/comment.module
@@ -134,9 +134,9 @@ function comment_theme() {
+    'field__comment' => array(
+      'base hook' => 'field',
+      'template' => 'field--comment',
     ),

This was originally postponed on #939462: Specific preprocess functions for theme hook suggestions are not invoked because then we wouldn't have to deal with explicitly defining something that should be detected automatically when the theme registry is built.

That being said (and since this made it in), a follow-up should be created to remove this once that issue is resolved.

andypost’s picture

@Mark I think better to add this to summary of the issue #939462: Specific preprocess functions for theme hook suggestions are not invoked because there's a lot of the same usage all over core (node base fields at least), so I think no reason to add a @todo for each case

markcarver’s picture

No. Issues that like do not need added scope creep. I've created the follow-up so it can be tracked and fixed later down the road, no @todo necessary.

Status: Fixed » Closed (fixed)

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