Patch testing issue for Comment-Field

@andypost pinged me on tweeter

Sorry guys for making your life more difficult (again) on this patch :-/.
Is there anything specific on which you need feedback ? AFAICT, using (a variant of) the $field->id as bundle, instead of the field name previously, makes total sense.

@yched Thanx for attention!

The main issue here that getFieldById() uses uuid as argument so logic goes complex because $field->id() could not be used at all.

The second one is that field API can't attach fields and ui on composite bundles (entity_type [delimiter] field_name) and no way to use bundle with a dot 'node.comment'

Comment entity needs bundle to attach fields. Previously we use field_name because it was unique now field_id looks promising because of the uniqueness but it have a dot '.' as splitter for entity and field.
So field_ui can't work with it, probably because some cmi related limits (suppose @larowlan will provide details) some of them was solved in #1831774: Config import assumes that 'config_prefix' contains one dot only also using field_id caused a lot of list($type, $field) = explode('.', $field_id) that makes code harder to read

As comment field could be attached to any entity we need to know entity type of the commented entity and field name via the thread attached. Also they are needed to provide proper views integration. Also currently we have no ability to load a field by it's ID.

Let's continue to roll statistics patches here, it's too hard to scroll the helper issue

@andypost:

The main issue here that getFieldById() uses uuid as argument so logic goes complex because $field->id() could not be used at all

I don't have much context, but I don't get why you try to use getFieldById(). getField($entity_type, $field_name) sounds more appropriate ?

The second one is that field API can't attach fields and ui on composite bundles (entity_type [delimiter] field_name) and no way to use bundle with a dot 'node.comment

But i though you were using "[entity_type]_[field_name]" for the bundle ? I don't see why field / field_ui would stumble on that, it's just a string ?
You should probably use '__' (double underscore) as a separator if you want to avoid ambiguity / clashes (case of entity type and / or field names containing '_' --> where is the separator ?)

I don't see why field / field_ui would stumble on that

because it requires a bundle to attach it's tabs to manage fields and etc

So for now we proceed with '__' as splitter

because it requires a bundle to attach it's tabs to manage fields and etc

Yes, and it should work if you use a bundle name that's [a-z0-9_], which is what you are doing, right ?
So, it doesn't seem like there is a problem ?

Bundle names should be [a-z0-9_], really. Many things outside Field UI will break if they aren't.

Test the merge of HEAD, current code pushed to github to not bring a mess to sandbox, I'll try to fix thing and then push to sandbox

+++ b/core/modules/rdf/rdf.module
@@ -289,36 +289,41 @@ function rdf_preprocess_node(&$variables) {
+    $fields = array_keys(Drupal::service('comment.manager')->getFields('node'));
+    $definitions = array_keys($variables['node']->getPropertyDefinitions());
+    $valid_fields = array_intersect($fields, $definitions);
+    if (!empty($comment_count_mapping['properties'])) {
+      $count = 0;

this expensive calls to service should live inside if()

The proper method to use massageFormValues
PS: not pushed, just to test the approach

Chasing HEAD

Stupid copy/paste

Let's try clean removal

Fix the rest and extend tests for admin screens, check for hidden option should be tested too

PS: interdiff needs a bit of work, so not pushed

merged comment_reply changes and breadcrumbs, fixed tests

@larowlan comment_count have issues so hidden and default for new is always open

And removal remains

re-roll

another round of tremendous merge

Fix user and disabled tests

Fix for the rest

proper merge

Great job @larowlan and @andypost. Here is a less technical review of the patch. Feel free to ignore or kick anything to follow up. Will review tests an onward later.

1. +++ b/core/modules/comment/comment.module
   @@ -64,21 +65,39 @@
   +const COMMENT_ACCESS_DENY = 'deny';
   

   Why is this 'deny' and not 'DENY'?

2. +++ b/core/modules/comment/comment.module
   @@ -382,19 +368,25 @@ function comment_permission() {
   + * @todo entity access for other entity types?
   

   Is this pending or will be fixed in followup? Perhaps an issue id if it is a followup.

3. +++ b/core/modules/comment/comment.module
   @@ -684,8 +663,13 @@ function comment_node_page_additions(EntityInterface $node) {
   +function comment_add(EntityInterface $entity, $field_name = 'comment', $pid = NULL) {
   

   I think we can remove this function. @dawehner and I removed the usage of this function in comment_reply conversion. If we can't get rid of this then maybe move it to manager. We can do it in followup as well.

4. +++ b/core/modules/comment/comment.module
   @@ -938,234 +932,165 @@ function comment_view_multiple($comments, $view_mode = 'full', $langcode = NULL)
   +  global $user;
   

   ahhh

5. +++ b/core/modules/comment/comment.routing.yml
   @@ -28,12 +28,13 @@ comment.confirm_delete:
   -    _entity_access: 'node.view'
   +    _access: 'TRUE'
   

   We should add @todo here or change it to entity.view.

6. +++ b/core/modules/comment/comment.services.yml
   @@ -3,4 +3,14 @@ services:
   +    arguments: ['@string_translation', '@plugin.manager.entity']
   ...
   +    arguments: ['@field.info', '@plugin.manager.entity']
   

   entity.manager now.

7. +++ b/core/modules/comment/comment.tokens.inc
   @@ -177,8 +184,8 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
   +            $replacements[$original] = $sanitize ? filter_xss($parent->subject->value) : $parent->subject->value;
   
   @@ -190,17 +197,36 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
   +            $replacements[$original] = $sanitize ? filter_xss($title) : $title;
   

   \Drupal\Component\Utility\Xss::filter() now

8. +++ b/core/modules/comment/lib/Drupal/comment/CommentBreadcrumbBuilder.php
   @@ -24,24 +25,40 @@ class CommentBreadcrumbBuilder implements BreadcrumbBuilderInterface {
   +      && isset($attributes['entity_type'])
   +      && isset($attributes['entity_id'])
   +      && isset($attributes['field_name'])
   

   I don't get the reason why we need to do this if the route is comment.reply then we'll get these param any way.

9. +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
   @@ -22,13 +22,15 @@ class CommentFormController extends EntityFormControllerNG {
        global $user;
   ...
   +    $entity = entity_load($comment->entity_type->value, $comment->entity_id->value);
   +    $instance = field_info_instance($comment->entity_type->value, $comment->field_name->value, $entity->bundle());
   
   @@ -182,8 +184,9 @@ public function form(array $form, array &$form_state) {
   +    $entity = entity_load($comment->entity_type->value, $comment->entity_id->value);
   +    $instance = field_info_instance($comment->entity_type->value, $comment->field_name->value, $entity->bundle());
   
   @@ -320,10 +323,15 @@ public function preview(array $form, array &$form_state) {
   +    $entity = entity_load($form_state['values']['entity_type'], $form_state['values']['entity_id']);
   ...
   +    $instance = field_info_instance($entity->entityType(), $field_name, $entity->bundle());
   +    $items = field_get_items($entity, $field_name);
   

   This is out of scope but can we please at least use \Drupal instead of wrapper functions.

10. +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
    @@ -22,13 +22,15 @@ class CommentFormController extends EntityFormControllerNG {
    +    $form['#theme'] = array('comment_form__' . $comment->entity_type->value . '__' . $entity->bundle() . '__' . $comment->field_name->value, 'comment_form');
    

    Please convert to multi line.

11. +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
    @@ -320,10 +323,15 @@ public function preview(array $form, array &$form_state) {
    +    if (user_access('post comments') && (user_access('administer comments') || $status['status'] == COMMENT_OPEN)) {
    

    hmmm user_acess

12. +++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.php
    @@ -54,25 +51,36 @@ protected function attachLoad(&$records, $load_revision = FALSE) {
    +    global $user;
    

    currentUser service.

13. +++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.php
    @@ -130,15 +154,18 @@ public function commentApprove(Request $request, CommentInterface $comment) {
    +    if ($entity = entity_load($comment->entity_type->value, $comment->entity_id->value)) {
    

    Injection.

14. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php
    @@ -0,0 +1,238 @@
    +      // @todo Remove in D9.
    

    Drupal 9 and issue id.

15. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/EntityLink.php
    @@ -0,0 +1,74 @@
    +      '#title' => t('Show teaser-style link'),
    ...
    +      '#description' => t('Show the comment link in the form used on standard entity teasers, rather than the full entity form.'),
    ...
    +      $this->build = entity_view_multiple($entities, $this->options['teaser'] ? 'teaser' : 'full');
    

    Injection.

Anyone minds if I try a patch here not related to the comment issue? I hope not, `cause I'm posting it anyway :)

@larowlan: Thanks so much for your work on this patch! I know it's been a rough road having to spend so much time re-rolling this massive patch. But thank you for carrying this burden and I really hope this gets committed soon!

and another re-roll

should fix

fixed broken test

Render caching is per-role (by default at least, as introduced in #1605290: Enable entity render caching with cache tag support), so I don't see how that could happen? Am I missing something?

Not sure what the need / use case is, but overriding applyDefaultValue() looks a bit weird. Typically field types will want to override getDefaultValue() instead - override "what the default value should be" rather than "how/when it gets assigned in an entity".

@larowlan, we discussed this topic in Prague today and unfortunately it's a thing that entity render caching doesn't support *yet* (but it needs to) and we outlined a plan for it. My suggestion is to comment out that test for now, but definitely keep it in the patch because it will be very useful to have once we get the other pieces in place.

@yched suggested in IRC:

yched> andypost: that method needs to live in the Field class, not the FieldItem class
yched> andypost: so you need a CommentField extends ConfigField, defined as a 'list_class' in the @FieldType annotation of CommentItem
andypost> yched, so I need to define a special class for status Field?
yched> and then CommentField::getDefaultValue() overrides ConfigField::getDefaultValue()
yched> andypost: yes, if your field type needs special handling of default values, those are handled at the Field level, not FieldItem
andypost> incrediable dx
yched> andypost: nope, "default value" involves multiple deltas
yched> andypost: so they live at the level of the object that is "a list of items"
yched> andypost: only makes sense...

but this does not works because EntityNG::getTranslatedField() does not apply default values when no data saved for entity.
The scenario:
1) visit admin/config/people/accounts/fields and add some Comments field
2) do not save field and instance settings (so no defaults are saved to field and instance)
3) visit user/1/edit no default value for comment field selected for user!

@larowlan The logic for widget should be following:
1) try to get status from $items (actual field values if any)
2) if no data, fallback to field defaults (but currently the

diff --git core/modules/comment/lib/Drupal/comment/Plugin/field/widget/CommentWidget.php core/modules/comment/lib/Drupal/comment/Plugin/field/widget/CommentWidget.php
index 191813b..accab7a 100644
--- core/modules/comment/lib/Drupal/comment/Plugin/field/widget/CommentWidget.php
+++ core/modules/comment/lib/Drupal/comment/Plugin/field/widget/CommentWidget.php
@@ -31,16 +31,18 @@ class CommentWidget extends WidgetBase {
   public function formElement(FieldInterface $items, $delta, array $element, array &$form, array &$form_state) {
     $field = $this->fieldDefinition;
     $entity = $items->getParent();
-    $default_value = $field->getFieldDefaultValue($entity);
-    if (!isset($default_value->status)) {
-      $default_value = (object) reset($field->default_value);
+    $default_value = $items->status;
+    if (!isset($default_value)) {
+      $default_value = $field->getFieldDefaultValue($entity);
+      $default_value = $default_value[$delta]['status'];
     }

So here's a fixes for default values:

1. approach1.txt - as suggested by yched
2. approach2.txt - does the same

Actually, if the goal is to set an initial value for the "default_value" property in newly created FieldInstance definitions, then hook_field_instance_create() might cleaner place than messing with either getDefaultValue() or applyDefaultValue() ?

@yched this is a half of the problem, the second part is absense of data when field added to existing entities - that's #93-94 mostly about.
So nice solution would be to use hook_field_instance_create() to provide defaults for field instance and once there's no data in entity have a fallback to instance defaults!

@larowlan done a lot of clean-up/comments fixes let's test

One of my main goals now during DrupalCon is to get back into this issue again and review and test it thoroughly. I'll also try hard to lobby for this getting committed with some follow-ups, as soon as possible. It's a big patch and very heavy to keep up with.

I only got started reviewing, but here's a start with some nit-picking:

+++ b/core/modules/comment/comment.admin.inc
@@ -84,41 +84,53 @@ function comment_admin_overview($form, &$form_state, $arg) {
   // Ensure all nodes are statically cached so that we do not have to load them
   // individually when getting their labels below.
-  node_load_multiple($nids);
-  $comments = comment_load_multiple($cids);
+  $comments = entity_load_multiple('comment', $cids);

This comment probably doesn't make sense any more.

+++ b/core/modules/comment/comment.install
@@ -36,48 +39,81 @@ function comment_uninstall() {
+  $entity_info = Drupal::service('entity.manager')->getDefinitions();

This should be \Drupal::.

+++ b/core/modules/comment/comment.install
@@ -36,48 +39,81 @@ function comment_uninstall() {
+  Drupal::state()->set('comment.maintain_entity_statistics', TRUE);

Should also be \Drupal::.

+++ b/core/modules/comment/comment.module
@@ -91,7 +101,7 @@ function comment_help($path, $arg) {
+      $output .= '<dd>' . t("Comment functionality can be attached to any Drupal entity, eg a content <a href='@content-type'> type</a> and the behavior can be customised to suit. Each entity can have its own default comment settings configured as: <em>Open</em> to allow new comments, <em>Hidden</em> to hide existing comments and prevent new comments, or <em>Closed</em> to view existing comments, but prevent new comments. These defaults will apply to all new content created (changes to the settings on existing content must be done manually). Other comment settings can also be customized per content type and entity, and can be overridden for any given item of content. When a comment has no replies, it remains editable by its author, as long as the author has a user account and is logged in.", array('@content-type' => url('admin/structure/types'))) . '</dd>';

The "eg" shortening is missing a trailing dot, should be "eg.". And there's a space within the second link tag there.

+++ b/core/modules/comment/comment.module
@@ -104,18 +114,41 @@ function comment_help($path, $arg) {
 /**
+ * Menu callback for loading the actual name of a comment field.
+ *
+ * @param string $field_id
+ *   The name of the field to load.
+ *
+ * @return string|false
+ *   The field name of the comment field entity or FALSE if $field_name
+ *   is not a comment field.
+ *
+ * @todo remove as unused.
+ */
+function comment_field_name_load($field_name) {
+  $fields = entity_load_multiple('field_entity', array(
+    'name' => $field_name,
+    'type' => 'comment',
+  ));
+  if (!empty($fields) && ($field = reset($fields))) {
+    return $field->getFieldName();
+  }
+
+  return FALSE;
+}

Let's remove this then :)

+++ b/core/modules/comment/comment.module
@@ -234,16 +275,8 @@ function comment_menu() {
+  // @todo Revisit after this becomes a view - https://drupal.org/node/1986606.
   $items['admin/content']['description'] = 'Administer content and comments.';

This is a view now. Is there anything we need to change?

+++ b/core/modules/comment/comment.module
@@ -379,19 +357,25 @@ function comment_permission() {
+ *
+ * @todo Replace with a view https://drupal.org/node/1938062
  */
 function comment_get_recent($number = 10) {

Let's post a follow-up for this and remove the comment.

@dixon_ thanx for review, fixed 4bd944c
Also fixed Render controller to use \Drupal::currentUser() without injection because of #2076411: Remove the request scope from the current user service

re @andypost #97
So, two things here:
- making sure that newly created field instances get a 'default_value' property: this is what hook_field_instance_create() is for.
- handling the case of "when creating a new comment field, preexisting entities do not have a value" : CommentWidget::formElement() is not the place to address that. What you want is "a comment field can never be seen as 'empty', it should always be seen as at least 'has one value, COMMENT_OPEN'." Discussed this with @Berdir, this should be done by overriding CommentField::__get() so that it returns a value even if there is none.

Yes, the unfortunate thing about that is that you will also have to override get(), as both could be called.

@Berdir: would it be easier if we override __set() instead ?

Finally pushed http://goo.gl/3sO7Jp

I'm running a bit back and forth on the conference here, so I'm sorry for breaking up the review so much. But here's another chunk of nit-picking:

diff --git a/core/includes/bootstrap.inc b/core/includes/bootstrap.inc
old mode 100644
new mode 100755

I don't think you intended to add this to the patch? :)

+++ b/core/modules/comment/comment.install
@@ -388,7 +423,329 @@ function comment_update_8003(&$sandbox) {
+    // Comment module may be disabled so we will need to inject the schema to
+    // avoid attempting to create a Field of an unknown type.
+    if (!\Drupal::moduleHandler()->moduleExists('comment')) {
+      // We can't use the autoloader because the comment namespace isn't
+      // registered.
+      // @todo Remove once https://drupal.org/node/1199946 is in.
+      require_once __DIR__ . '/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php';
+    }

Disabled modules are in, I believe. Should we remove this then?

+++ b/core/modules/comment/comment.install
@@ -5,30 +5,33 @@
+  // @todo Remove when disabled modules go away.
   \Drupal::entityManager()->addNamespaces(new ArrayIterator(array(
     'Drupal\comment' => DRUPAL_ROOT . '/core/modules/comment/lib',
   )));

Remove this as well then?

+++ b/core/modules/comment/comment.module
@@ -496,121 +465,142 @@ function theme_comment_block($variables) {
+ * @todo Make node links as comment_links formatter
+ *   http://drupal.org/node/1901110

Let's remove this @todo since there's an issue for this already.

+++ b/core/modules/comment/comment.module
@@ -935,234 +899,166 @@ function comment_view_multiple($comments, $view_mode = 'full', $langcode = NULL)
- * Implements hook_form_FORM_ID_alter().
+ * Implements hook_form_FORM_ID_alter() for field_ui_field_instance_edit_form().

I've never seen this doc/comment pattern before. Maybe it's enough with just Implements hook_form_FORM_ID_alter().. I think that's descriptive enough.

+++ b/core/modules/comment/comment.module
@@ -935,234 +899,166 @@ function comment_view_multiple($comments, $view_mode = 'full', $langcode = NULL)
- * Form submission handler for node_type_form().
+ * Implements hook_form_FORM_ID_alter() for field_ui_field_overview_form().

Same here and on more places, seems a bit verbose IMO.

+++ b/core/modules/comment/comment.module
@@ -1301,31 +1220,59 @@ function comment_load($cid, $reset = FALSE) {
+ *
+ * @todo Move to a method on comment manager service
+ *   - https://drupal.org/node/2097123.

Let's remove this @todo since there's an issue already.

+++ b/core/modules/comment/comment.module
@@ -1488,9 +1454,9 @@ function comment_prepare_author(Comment $comment) {
   $comment = $variables['elements']['#comment'];
-  $node = $variables['elements']['#node'];
+  $comment_entity = entity_load($comment->entity_type->value, $comment->entity_id->value);
   $variables['comment'] = $comment;
-  $variables['node'] = $node;
+  $variables['comment_entity'] = $comment_entity;

I think the $comment_entity variable name is a bit confusing here. One could easily think it's the comment entity itself.
Elsewhere in the code I've seen $commented_entity which I think is a lot better.

+++ b/core/modules/comment/comment.module
@@ -1667,8 +1638,8 @@ function theme_comment_post_forbidden($variables) {
   // Provide contextual information.
-  $variables['node'] = $variables['content']['#node'];
-  $variables['display_mode'] = variable_get('comment_default_mode_' . $variables['node']->getType(), COMMENT_MODE_THREADED);
+  $variables['entity'] = $variables['content']['#entity'];
+  $variables['display_mode'] = $variables['content']['#display_mode'];

To be consistent with the previous point we should use $commented_entity here instead? One could easily think this is an actual comment entity.

Let's test #118

Here's more reviews based on the latest patch:

+++ b/core/modules/comment/comment.tokens.inc
@@ -15,14 +15,14 @@ function comment_token_info() {
+  // @todo Make this work per field. See http://drupal.org/node/2031903
+  $entity['comment-count'] = array(

Let's remove this since we have an issue for this already.

+++ b/core/modules/comment/comment.tokens.inc
@@ -220,17 +248,25 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
+          $fields = array_keys(Drupal::service('comment.manager')->getFields($entity->entityType()));

Should use \Drupal::.

+++ b/core/modules/comment/comment.views.inc
@@ -317,11 +312,74 @@ function comment_views_data() {
+  $entities_info = Drupal::entityManager()->getDefinitions();
+
+  // Provide a relationship for each entity type except comment.
+  foreach ($entities_info as $type => $entity_info) {
+    if ($type == 'comment' || empty($entity_info['fieldable']) || !isset($entity_info['base_table'])) {
+      continue;
+    }
+    if ($fields = Drupal::service('comment.manager')->getFields($type)) {

\Drupal:: on two places here as well.

+++ b/core/modules/comment/comment.views.inc
@@ -372,18 +430,35 @@ function comment_views_data() {
+    // This relationship does not use the 'field id' column, if the entity has
+    // multiple comment-fields, then this might introduce duplicates, in which
+    // case the site-builder should enable aggregation and SUM the comment_count

Should we document this in the 'help' key to make this obvious to the end-user?

+++ b/core/modules/comment/comment.views.inc
@@ -372,18 +430,35 @@ function comment_views_data() {
+    // field. We cannot create a relationships from the base table to
+    // {comment_entity_statistics} for each field as multiple joins between
+    // the same two tables is not supported.

Grammar error. It should be "We cannot create a relationship"

+++ b/core/modules/comment/comment.views.inc
@@ -478,76 +553,130 @@ function comment_views_data() {
+    'help' => t('The Entity type to which the comment is a reply to.'),

Should not have capital letter 'E' here.

+++ b/core/modules/comment/comment.views.inc
@@ -478,76 +553,130 @@ function comment_views_data() {
+    'help' => t('The Field id from which the comment originated.'),

Should not have capital letter 'F' here.

+++ b/core/modules/comment/comment.views.inc
@@ -478,76 +553,130 @@ function comment_views_data() {
+  // Provide a integration for each entity type except comment.
+  foreach (Drupal::entityManager()->getDefinitions() as $entity_type => $entity_info) {
+    if ($entity_type == 'comment' || empty($entity_info['fieldable']) || !isset($entity_info['base_table'])) {
+      continue;
+    }
+    $fields = Drupal::service('comment.manager')->getFields($entity_type);

Should be \Drupal:: on two places here.

+++ b/core/modules/comment/lib/Drupal/comment/CommentFieldName.php
@@ -0,0 +1,41 @@
+/**
+ * @file
+ * Contains \Drupal\comment\CommentNewItem.
+ */

No, it doesn't ;)

+++ b/core/modules/comment/lib/Drupal/comment/CommentFieldNameValue.php
@@ -0,0 +1,53 @@
+/**
+ * @file
+ * Contains \Drupal\comment\CommentNewValue.
+ */

Not this either ;)

+++ b/core/modules/comment/lib/Drupal/comment/CommentFieldNameValue.php
@@ -0,0 +1,53 @@
+  /**
+   * Implements \Drupal\Core\TypedData\TypedDataInterface::getValue().
+   */

Let's do {@inheritdoc} here and all other places on these classes.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.php
@@ -0,0 +1,206 @@
+    // @todo Decide on better UX http://drupal.org/node/1901110

Let's remove this since issue already exists.

Fixed broken tests - added @todo to properly implement render cache per-user CommentUserTest
@dixon is there an issue for that?

Addressed review #119 & #122

Finally fixed default values as @berdir suggested

diff --git a/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentIte
index ea1455d..be0e1d8 100644
--- a/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php
+++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php
@@ -183,6 +183,20 @@ public function instanceSettingsForm(array $form, array &$form_state) {
   /**
    * {@inheritdoc}
    */
+  public function __get($name) {
+    if ($name == 'status' && !isset($this->values[$name])) {
+      // Get default value from field instance when no data saved in entity.
+      $field_default_values = $this->getFieldDefinition()->getFieldDefaultValue($this->getEntity());
+      return $field_default_values[0]['status'];
+    }
+    else {
+      return parent::__get($name);
+    }
+  }
+
+  /**
+   * {@inheritdoc}
+   */
   public function isEmpty() {
     // There is always a value for this field, it is one of COMMENT_OPEN,
     // COMMENT_CLOSED or COMMENT_HIDDEN.

I think it's the __get() of the CommentField class that should be overriden, not the one of CommentItem.
When the *field* is empty on an existing entity, we want it to say "no, pretend I'm not empty, I have one FieldItem, here it is".

Added lost title callback that still needed to display breadcrumbs properly.
Simplified code for admin controller.

@yched suppose we need __get() for CommentItem because the *field should never be empty* as isEmpty() method tells.

"field should never be empty" is something that deals with the Field (list of FieldItem objects) level, not each separate FieldItem level.
--> CommentField::__get() is the one you want to work on.

@yched yes, that's why we do not implement CommentField for 'status' property|item of the CommentItem (Field)
While all fields are 'list of items' and no way to implement single value field #1869574: Support single valued Entity fields we should hide UI to change cardinality and using $items->status to work with first item only. I'd like to point #1751210: Convert URL alias form element into a field and field widget that got this trouble too

EDIT also probably related #1988492: Avoid having empty field items by default

Dumping some notes and quick nit-picks here before hitting bed.

I believe the code is in very good shape, I'm mostly running into docs and todos that needs cleanup.

+++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
@@ -17,29 +24,72 @@
+    // comments on entities.

Should use capital letter 'C'.

+++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
@@ -0,0 +1,225 @@
+    // Make sure field doesn't already exist.

"Make sure the field..." sounds better, IMO.

+++ b/core/modules/comment/lib/Drupal/comment/CommentRenderController.php
@@ -63,8 +126,10 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
+          // @todo deprecate comment_links(). https://drupal.org/node/1975962
+          '#links' => comment_links($entity, $commented_entity, $entity->field_name->value),

Let's remove this @todo since it has an issue.

+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.php
@@ -53,25 +50,35 @@ protected function attachLoad(&$records, $load_revision = FALSE) {
+    // maintenance of the {comment_entity_statistics} table.

Should have leading capital letter 'M'.

+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.php
@@ -79,21 +86,30 @@ public function updateNodeStatistics($nid) {
+          // Use the created date of the entity if it's set,
+          // or default to REQUEST_TIME.
+          'last_comment_timestamp' => ($entity instanceof EntityChangedInterface) ? $entity->getChangedTime() : REQUEST_TIME,

The comment line break seems a bit early here.

+++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
@@ -56,7 +58,7 @@ class Comment extends EntityNG implements CommentInterface {
-   * @todo Rename to 'id'.
+   * @todo Rename to 'id'. https://drupal.org/node/2031935

This @todo has an issue, so lets remove the comment.

+++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
@@ -70,20 +72,34 @@ class Comment extends EntityNG implements CommentInterface {
-   * @todo: Rename to 'parent_id'.
+   * @todo: Rename to 'parent_id'. https://drupal.org/node/2031931

This @todo also has an issue. Let's remove it.

Old 'title callback' #125 should be removed because we use path-based breadcrumbs https://drupal.org/node/2098323

Here's a continuation of the review. I'm gonna review the last bits tomorrow at the sprint. Then, we also need to tackle the render cache thingie...

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/formatter/CommentDefaultFormatter.php
@@ -0,0 +1,160 @@
+    if ($status != COMMENT_HIDDEN && empty($entity->in_preview) &&
+      // Comments are added to the search results and search index by
+      // comment_node_update_index() instead of by this formatter, so don't
+      // return anything if the view mode is search_index or search_result.
+      !in_array($this->viewMode, array('search_result', 'search_index'))) {
+      $comment_settings = $this->getFieldSettings();

Are we sure this is the best way to do this? Wouldn't it be better to set it to hidden by default for these view modes?

It might be out of scope. But maybe we can remove most of the logic in comment_node_update_index() in favor of doing it properly through the formatter?

That said, I'm ok for leaving it like it is for now. We can treat that as an optimization later, since it would only move around some code and not break any behavior really.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/field/formatter/CommentDefaultFormatter.php
@@ -0,0 +1,160 @@
+      // Append comment form if the comments are open and the form
+      // is set to display below the entity.

This comment is breaking line a bit too early.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/argument/UserUid.php
@@ -85,16 +85,25 @@ protected function defaultActions($which = NULL) {
+    // Load the table information to get the entity information to finally
+    // be able to join to filter by the original entity type this join is
+    // attached to.
+    if ($this->table != 'comment') {
+      $subselect = $this->database->select('comment', 'c');
+      $subselect->addField('c', 'cid');
+      $subselect->condition('c.uid', $this->argument);
+
+      $entity_id = $this->definition['entity_id'];
+      $entity_type = $this->definition['entity_type'];
+      $subselect->where("c.entity_id = $this->tableAlias.$entity_id");
+      $subselect->condition('c.entity_type', $entity_type);

I'm not sure where we are "loading" any table information. And we're not really joining per either (although subselect could definitely be seen as a type of join of course).

Maybe it's more straight forward to just say "Use the table definition to correctly add this user id condition."

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/LinkDelete.php
@@ -26,10 +26,10 @@ public function access() {
+    $comment = $this->get_entity($values);

This should be $this->getEntity($values).

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentFieldsTest.php
@@ -68,6 +69,20 @@ function testCommentInstallAfterContentModule() {
+    // Drop default comment field added in CommentTestBase::setup().
+    // @todo WTF#1497374
+    entity_load('field_entity', 'node.comment')->delete();
+    if ($field = Field::fieldInfo()->getField('node', 'comment_node_forum')) {
+      $field->delete();
+    }

Uh, what's up with this? :)

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentInterfaceTest.php
@@ -89,23 +89,40 @@ function testCommentInterface() {
+    // Deliberately use the wrong url to test

Missing trailing sentence dot.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentNonNodeTest.php
@@ -0,0 +1,387 @@
+    $this->assertNoFieldChecked('edit-default-value-input-comment-0-status-2');
+    ¶
+    // Add a new comment field.

Indentation error.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTokenReplaceTest.php
@@ -100,11 +100,13 @@ function testCommentTokenReplacement() {
+    // Also test the Deprecated legacy token.

Should not have capital 'D' here.

+++ b/core/modules/file/lib/Drupal/file/Tests/FileFieldWidgetTest.php
@@ -202,6 +202,11 @@ function testMultiValuedWidget() {
+    // @todo Add method to roles list_class to exclude built in roles and use
+    //   User::getRoles() instead - https://drupal.org/node/2096717.

Let's remove since we have an issue for it.

fixed all from #137 and a bit of clean-up, can't push now so just interdiff and patch

+ if ($status != COMMENT_HIDDEN && empty($entity->in_preview) &&
+ // Comments are added to the search results and search index by
+ // comment_node_update_index() instead of by this formatter, so don't
+ // return anything if the view mode is search_index or search_result.
+ !in_array($this->viewMode, array('search_result', 'search_index'))) {

We can't use disabled entity display for render now ;( so that's the only way now

// Deliberately use the wrong url to test

Second line is too long and contains controller and method to test

Uncomment tests, also the whole interdiff also addresses #136

Filed #2099105: Clean-up render cache when permission changes
Also added fix for render cache when instance settings changed

Ok, made it through the whole patch finally. Here are my, for now, final remarks. I took the freedom to push these minor changes to the sandbox.

I'll update the main issue in a moment when #140 comes back green.

+++ b/core/modules/node/node.api.php
@@ -509,6 +511,7 @@ function hook_node_create(\Drupal\Core\Entity\EntityInterface $node) {
+  $types_we_want_to_process = Drupal::config('my_types')->get('types');
   if (count(array_intersect($types_we_want_to_process, $types))) {
     // Gather our extra data for each of these nodes.
     $result = db_query('SELECT nid, foo FROM {mytable} WHERE nid IN(:nids)', array(':nids' => array_keys($nodes)));
@@ -604,8 +607,8 @@ function hook_node_access(\Drupal\node\NodeInterface $node, $op, $account, $lang

@@ -604,8 +607,8 @@ function hook_node_access(\Drupal\node\NodeInterface $node, $op, $account, $lang
  * @ingroup node_api_hooks
  */
 function hook_node_prepare_form(\Drupal\node\NodeInterface $node, $form_display, $operation, array &$form_state) {
-  if (!isset($node->comment->value)) {
-    $node->comment = variable_get('comment_' . $node->getType(), COMMENT_NODE_OPEN);
+  if (!isset($node->my_rating)) {
+    $node->my_rating = Drupal::config("my_rating_{$node->bundle()}")->get('enabled');
   }

Should be \Drupal:: on both places.

+++ b/core/modules/rdf/rdf.module
@@ -308,32 +310,41 @@ function rdf_preprocess_node(&$variables) {
+  if (Drupal::moduleHandler()->moduleExists('comment')) {
+    if (!empty($comment_count_mapping['properties'])) {
+      $fields = array_keys(Drupal::service('comment.manager')->getFields('node'));

Should be \Drupal:: on two places here as well.

+++ b/core/profiles/standard/standard.install
@@ -24,8 +24,8 @@ function standard_install() {
+  Drupal::service('comment.manager')->addDefaultField('node', 'article', 'comment', COMMENT_OPEN);

Same here.

Clean merge HEAD after #1953528: Store 'per-bundle view mode settings' in CMI, get rid of field_bundle_settings() & its variable

No more needed!