Use field instance name for header of comment list, drop comment-wrapper template

Just a first try.

Let's try this one :)

Removing the comment wrapper template sounds good.

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',
   ),
   

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

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").

Correct

Related #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form is in

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?

#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.)

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

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

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.