Expand CommentInterface to provide methods

Working on this.

Here is a first version of the patch, only refactoring the CommentInterface and the Comment class implementing it.

PHP syntax issues should be fixed now.

Renamed the patch file to be a correct description.

Fixed issues from #5.

Still working on this, just a quick update. There were some inconsistencies on the naming of the getters/setters that are now fixed. This will fix most of the tests, but still lots of things that need to be reviewed.

Still working on this, just a quick update. There were some inconsistencies on the naming of the getters/setters that are now fixed. This will fix most of the tests, but still lots of things that need to be reviewed.

Still working on this, just a quick update. There were some inconsistencies on the naming of the getters/setters that are now fixed. This will fix most of the tests, but still lots of things that need to be reviewed.

Sorry for the duplicated comments. drupal.org was acting funny...

I missed a couple functions renaming. That should fix a few more tests, but still no green light.

Changed to needs review so test runs against patch.

New patch. Still not completly green, but getting close.

Ok, this might be green.

I think that's it.

+++ b/core/modules/comment/lib/Drupal/comment/CommentInterface.phpundefined
@@ -15,6 +15,216 @@
+   * Returns the parent comment entity if this is a reply to a comment.
+   *
+   * @return \Drupal\comment\CommentInterface|null
+   *   A Comment Entity of the parent comment or null if there is no parent.
...
+   * @return \Drupal\comment\CommentInterface|null
+   *   The class instance that this method is called on.
+   */
+  public function setAuthorName($name);

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.phpundefined
@@ -52,160 +52,192 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function setAuthorName($name) {
+    $this->set('name', $name);
+    return $this;
+  }

Why are setters, like setAuthorName, returning the type or null?

I think sometimes |null makes sense, but when it's return $this, it's just the class?

See @Berdir's comment in #2028037-11: Expand FeedInterface with methods

I see in this issue in #5
it was said to make the return \Drupal\comment\CommentInterface|null but I think that is only in the case where the description was like "if this is a reply to a comment" like for getParent and getParentId. And the null is what is returned if it is not a reply to a comment.

Unassigned

Looking at this now - patch needs a reroll.

Taking myself off this one - say when the comment patch goes in and I'll come back again if I can.

@ceng, it went in last night!

Picking this up now.

Hi, new patch ready for review. I can't create an interdiff, probably because its too different now?

1. Renamed *Node() to *Entity() now comments fields on any entity
2. Added a getFieldId() and getFieldName() and the setters
3. Replaced all instances of direct access to the Comment Entity properties with the get or set methods which exist in the comment module including it's tests
4. Question: There are three reads of a property called $comment->is_anonymous in the D8 codebase but no writes to that property. It is therefore always NULL - can it be removed?
5. Question: There is code which reads and writes to a property called $comment->comment_body - should this have a get and set method? Maybe one for a follow up issue?
6. Question: There is code which reads and writes to a property called $comment->is_preview - should this have a get and set method? Maybe one for a follow up issue?
7. Question: The $comment->getPublished() method can return NULL at present since it returns the value of the $status variable which has a tri-state, it can be COMMENT_PUBLISHED, COMMENT_NOT_PUBISHED or NULL (meaning unknown, this is checked for repeatedly in the code). I think the function should be returning TRUE or FALSE though this will require some rewriting of where it is used. One for a follow up issue?

There are three reads of a property called $comment->is_anonymous in the D8 codebase but no writes to that property. It is therefore always NULL - can it be removed?

Pretty sure there's a #value field in the comment form-controller called is_anonymous, so in this instance its a property from submitted form values.

There is code which reads and writes to a property called $comment->comment_body - should this have a get and set method? Maybe one for a follow up issue?

This is a field, we can switch it to ->get('comment_body') or leave as is

There is code which reads and writes to a property called $comment->is_preview - should this have a get and set method? Maybe one for a follow up issue?

There is no property for this, but its set when the 'preview' button is clicked. We should look at how node does it or add a in_preview property with get/set

The $comment->getPublished() method can return NULL at present since it returns the value of the $status variable which has a tri-state, it can be COMMENT_PUBLISHED, COMMENT_NOT_PUBISHED or NULL (meaning unknown, this is checked for repeatedly in the code). I think the function should be returning TRUE or FALSE though this will require some rewriting of where it is used. One for a follow up issue?

I agree, lets make it TRUE or FALSE only

vote 1 commentedEntity

+1 to getCommentedEntity()

With threaded comments, comments can sometimes be on other comments yet getCommentedEntity would return e.g. the parent node, right? So not sure getCommentedEntity() works well in that case.

Thanks all for your comments - I'm going to add all these items into this patch then rather than creating any follow up issues.

+++ b/core/modules/comment/comment.module
@@ -1577,7 +1579,7 @@ function template_preprocess_comment(&$variables) {
-    $variables['status'] = ($comment->status->value == COMMENT_NOT_PUBLISHED) ? 'unpublished' : 'published';
+    $variables['status'] = $comment->isPublished() == COMMENT_NOT_PUBLISHED ? 'unpublished' : 'published';

According to this logic, if the status is NULL then it would be published? +1 on isPublished() to return bool btw.

in PHP, this is actually TRUE ... (0 == NULL), so the logic is right, despite reading badly! Will work out the boolean return.

1. I don't think getEntityType() and ..Id() as method names work. That's extremely close to entityType() but something else.

I've gone for getCommentedEntity(), getCommentedEntityId() and getCommentedEntityType() as suggested.

2. Note that https://drupal.org/node/2078387 also adds getAuthorId(), getAuthor() and setAuthorId() to comments and does some conversions.

I've left these modifications - will probably have to remove when this patch goes in?

3. We shouldn't add a setChangedTime() method. There are some cases where it is set, but it's not really a public API.

OK, I've removed this and set back to setting the value directly. What about setCreatedTime()?

4. According to this logic, if the status is NULL then it would be published? +1 on isPublished() to return bool btw.

isPublished() returns a bool now - I've updated the places it was used in the code and checked the workflows and all seems well (tests seem to be passing too)

5. I think this kind of logic should be hidden in getAuthorName(), see getUsername() on users. Then we don't have to set somewhere above.

I copied the same process as getUsername() in User Entity as suggested so it happens in the Comment class and and updated the external versions.

6. So we have a name and an author token which is more or less the same?

I've dropped the name token and updated the tests.

7. No need for the !empty() check, if ($parent_id = $comment->getParentId()) is fine. !empty() is a left-over of the stdClass $entity object times. We could even add a getParentComment() method?

I removed getParentId() and added a hasParentComment() method instead since getParentId() was mostly used for determining if there was a parent at all. This makes the code easier to read. To get the parent comment itself there is now a getParentComment() as suggested.

8. similar for this, not yet sure how to name it, but there should be a method to get the entity.

As per 1 above, added getCommentedEntity*() methods.

9. Ah, you do have a method, just not used yet there :)

Made use of the getCommentedEntity() function in the appropriate places.

10. the field definition of entity_id is a lie. No idea why it got in like this, but it's wrong. entity_id needs to be simple integer_field and getEntity needs to manually load the entity based on the other two methods.

entity_id is now an integer field defined in the Comment::baseFieldDefinitions() function. getCommentedEntity() loads the entity as suggested.

11. Hint: If you add the new methods at the end or near the end of the class, then the diff might be much easier to read as it should first remove all properties and then add the new methods instead of this mix. Try it out.

Woah, pro tip FTW!

This leaves the $comment->is_anonymous and $comment->is_preview variables still to tidy up. I think this patch is good to go if these are moved out to follow up issues, the patch is getting quite big with a number of changes. Or I can add to this one if everyone is happy with that.

Not too sure about removing the token. That's basically an API change and one that is ugly to take care of because we don't know where that token was used. Maybe better to keep and add a @todo that says it's identical to author and should be dropped.

Put name back in, its the same code as author

Huh, why is that necessary? get('changed')->value should work fine? (changed->value too, but I prefer get() in those methods).

It isn't necessary and I've taken it out.

Test fail because of the hook_comment_* - needed to go through and update all implementations of the hook_comment_* in node, tracker, rdf and forum modules.

For the 'name' token we have a separate issue #920056: [comment:name] duplicates [comment:author], and the latter should use format_username()

Rerolled patch attached

Patch in comment #42 did not apply.

Changing to Needs work.

Please ignore this file.

reroll

Should fix the threadLock test

grumble grumble

passes locally, but clearly testbot is different kettle

Re-roll after removal of comment_alphadecimal_to_int() etc went in
I'm happy this is ready

66: comment-interface-2028025.67.patch queued for re-testing.

66: comment-interface-2028025.67.patch queued for re-testing.

1. +++ b/core/modules/comment/comment.module
   @@ -1084,7 +1084,7 @@ function comment_user_cancel($edit, $account, $method) {
   -        $comment->status->value = 0;
   +        $comment->setPublished(COMMENT_NOT_PUBLISHED);
   

   did I see somewhere else we are not using the constants, but using true and false?

2. +++ b/core/modules/comment/comment.module
   @@ -1274,26 +1274,28 @@ function comment_get_display_page($cid, $instance) {
   +      $account = user_load_by_name($author_name);
   ...
   -    else {
   -      $comment->name->value = \Drupal::config('user.settings')->get('anonymous');
   +    elseif (empty($author_name)) {
   +      $comment->setAuthorName(\Drupal::config('user.settings')->get('anonymous'));
   

   I looked at this logic change from else to elseif, and got confused by the whole first bit of comment_preview(). It's like it loads based on the name and then sets the name..

   We shouldn't refactor comments to make more sense in this issue, so as long as it is correct, I guess it is fine.

   Here is the logic in context:

   /**
    * Generates a comment preview.
    *
    * @param \Drupal\comment\CommentInterface $comment
    *   The comment entity to preview.
    *
    * @return array
    *   An array as expected by drupal_render().
    */
   function comment_preview(CommentInterface $comment, array &$form_state) {
     $preview_build = array();
     $entity = entity_load($comment->getCommentedEntityType(), $comment->getCommentedEntityId());
   
     if (!form_get_errors($form_state)) {
       // Attach the user and time information.
       $author_name = $comment->getAuthorName();
       if (!empty($author_name)) {
         $account = user_load_by_name($author_name);
       }
       elseif (\Drupal::currentUser()->isAuthenticated() && empty($comment->is_anonymous)) {
         $account = \Drupal::currentUser();
       }
   
       if (!empty($account) && $account->isAuthenticated()) {
         $comment->setAuthorId($account->id());
         $comment->setAuthorName(check_plain($account->getUsername()));
       }
       elseif (empty($author_name)) {
         $comment->setAuthorName(\Drupal::config('user.settings')->get('anonymous'));
       }
   

3. +++ b/core/modules/comment/comment.tokens.inc
   @@ -136,32 +136,22 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
            case 'mail':
   -          if ($comment->uid->target_id != 0) {
   -            $mail = $comment->uid->entity->getEmail();
   -          }
   -          else {
   -            $mail = $comment->mail->value;
   -          }
   +          $mail = $comment->getAuthorEmail();
   
   +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
   @@ -454,8 +303,214 @@ public static function baseFieldDefinitions($entity_type) {
   +  public function getAuthorEmail() {
   +    $mail = $this->get('mail')->value;
   +
   +    if ($this->get('uid')->target_id != 0) {
   +      $mail = $this->get('uid')->entity->getEmail();
   +    }
   +
   +    return $mail;
   

   why was the logic rewritten like this instead of the original if/else?

   So code inspection in an IDE does not warn that the return value might not be set? (chx in irc says code inspection is smart enough, so that wasn't the reason)

4. +++ b/core/modules/comment/comment.tokens.inc
   @@ -136,32 +136,22 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
   +          $replacements[$original] = $sanitize ? filter_xss($comment->getSubject()) : $comment->getSubject();
   
   @@ -179,28 +169,30 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
   +          $name = $comment->getAuthorName();
   +          $replacements[$original] = $sanitize ? filter_xss($name) : $name;
   

   sometimes getX() is called once, and then the var used. more often in its ... ? filter_xss($c->getX()) : $c->getX(); with two calls to the get. We could later do a follow-up to make it consistent if that would be useful.

5. +++ b/core/modules/comment/comment.tokens.inc
   @@ -179,28 +169,30 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
   -        // Default values for the chained tokens handled below.
   ...
   +        case 'name':
   

   adding issue for this todo. #920056: [comment:name] duplicates [comment:author], and the latter should use format_username()

6. +++ b/core/modules/comment/lib/Drupal/comment/CommentInterface.php
   @@ -26,6 +26,257 @@
   +   * Returns the ID of the comment field the comment is attached to.
   +   *
   +   * @return int
   +   *   The field ID of the field the comment is attached to.
   +   */
   +  public function getFieldId();
   

   huh. comments get attached to specific comment fields on the entities they are attached to? (larowlan says yes) Can we have more than one comment field on an entity? (larowlan says yes) Do we "enable commenting" by adding a comment field? (answering my own question: add the field, then edit settings on that field to enable commenting.) So. That leads me to think that the getCommentedEntity() returns the entity that field is on. and comments on different fields might be attached to the same entity. Maybe I'll draw myself a picture.

7. +++ b/core/modules/comment/lib/Drupal/comment/CommentInterface.php
   @@ -26,6 +26,257 @@
   +   * Set the field ID for which this commment is attached.
   +   *
   +   * @param string $field_id
   +   *   The field identifier
   

   a) commment is mis-spelled. I fixed it.

   b) is the field ID a string? My guess is it would be an int, like getFieldId(). But larowlan, via irc, says. it might be getFieldId that the type is wrong on. "FieldId is of the form {entity_type}__{field_name}" I'll leave it as string for now. Maybe we need to change the return type of getFieldId(). Seems Entity/Field has $id which is a string, but a different format. OK. so talked more to larowlan. Idea for followup is: since bundle() might be the same as getFieldId(), getFieldId() might not be needed and we could repurpose it "to map node__comment to node.comment, and make it useful, and a place to document this."

   c) missing period. I added it.

8. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
   @@ -222,23 +73,21 @@ public function id() {
   -        if ($this->pid->target_id == 0) {
   +        if (! $this->hasParentComment()) {
              // This is a comment with no parent comment (depth 0): we start
              // by retrieving the maximum thread level.
   

   no parent is no longer determined by checking if (depth) is 0. I almost changed the comment about the if... but

   I read the next bit of code about max depth, but I could not decide if noting something with no parent had a depth of 0 would be useful. I left it.

Not done yet. Posting what I have so far so I dont lose it.

(@cend I think it's ok to assign it to me, it's been a while. :) I have some changes I'll post after I go through the rest.)

oops. meant to save this comment I had open in another window earlier.
--
I started reviewing this. It is a lot to read through.

in #35 by @ceng

This leaves the $comment->is_anonymous and $comment->is_preview variables still to tidy up. I think this patch is good to go if these are moved out to follow up issues, the patch is getting quite big with a number of changes.

grepping for is_anonymous and is_preview I dont see is_preview at all, and in
core/modules/comment/lib/Drupal/comment/CommentFormController.php

213:    $form['is_anonymous'] = array(
280:      if ($form_state['values']['name'] && !$form_state['values']['is_anonymous'] && !$account) {
284:    elseif ($form_state['values']['is_anonymous']) {
322:    if (!$comment->is_anonymous && !empty($author_name) && ($account = user_load_by_name($author_name))) {
327:    if ($comment->is_anonymous && (!isset($author_name) || $author_name === '')) {

These were also discussed in #26, #27, and @Berdir says in #28

If that's the case, then I doubt that's set. The NG (now ContentEntity) form controller only sets explicitly defined fields, everything else is ignored. You have to override and extend buildEntity() to set more stuff, see NodeFormController.

The patch has no instances of is_anonymous and is_preview, so it wasn't this patch that took it out. Maybe it was another issue.

I'll create an issue as was mentioned and we can sort it out there. Here it is #2170439: $comment->is_anonymous and $comment->is_preview variable should be removed or converted to be explicit properties with getter and setter menthods

I'll keep going with the review. I have a few small changes so far.
--
also started updating issue summary.

1. +++ b/core/modules/comment/lib/Drupal/comment/CommentInterface.php
   @@ -26,6 +26,257 @@
   +   * Determine if this comment is a reply to another comment.
   ...
   +   * Set the field ID for which this commment is attached.
   

   I changed a few first line function summaries to be third person verbs.

2. +++ b/core/modules/comment/lib/Drupal/comment/CommentInterface.php
   @@ -26,6 +26,257 @@
   +   * Returns the title of the comment.
   +   *
   +   * @return string
   +   *   The title of the comment.
   +   */
   +  public function getSubject();
   

   read a little strange that the docs mentioned title and not subject. larowlan suggested checking what the column is called in {comments} table in comment_schema, which is in comment.install It is 'subject'. So changing the docs.

3. +++ b/core/modules/comment/lib/Drupal/comment/CommentViewBuilder.php
   @@ -273,8 +273,8 @@ protected function alterBuild(array &$build, EntityInterface $comment, EntityVie
   -      $commented_entity = $this->entityManager->getStorageController($comment->entity_type->value)->load($comment->entity_id->value);
   -      $instance = $this->fieldInfo->getInstance($commented_entity->entityType(), $commented_entity->bundle(), $comment->field_name->value);
   +      $commented_entity = $this->entityManager->getStorageController($comment->getCommentedEntityType())->load($comment->getCommentedEntityId());
   +      $instance = $this->fieldInfo->getInstance($commented_entity->entityType(), $commented_entity->bundle(), $comment->getFieldName());
   

   This made me wonder if $commented_entity->entityType() should become $comment->getCommentedEntityType(). I decided it wasn't necessary and I didn't change it.

4. +++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.php
   @@ -236,8 +236,8 @@ public function getReplyForm(Request $request, $entity_type, $entity_id, $field_
   -        // Check if the parent comment is published and belongs to the entity.
   -        if (($comment->status->value == CommentInterface::NOT_PUBLISHED) || ($comment->entity_id->value != $entity->id())) {
   +        // Check if the parent comment is published and belongs to the current nid.
   +        if (!$comment->isPublished() || ($comment->getCommentedEntityId() != $entity->id())) {
   

   changing that comment from entity to current nid seems out of place. I dont think it is node specific. changing comment back.

5. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
   @@ -281,24 +130,24 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
   -      if (empty($this->changed->value)) {
   -        $this->changed->value = $this->created->value;
   +      if (is_null($this->getChangedTime())) {
   +        $this->set('changed', $this->getCreatedTime());
   

   seems like this should be:
   $this->setChangedTime($this->getCreatedTime()); Oh, but I see in comment #5 @berdir mentioned something related. and this is in preSave(). Ok.

I stopped at CommentActionTest

posting the patch so far, and taking a break.

coming back to try and finish review.

trying to clarify what is *not* being done in this issue, and why. please edit the summary and fill out the why where the reason is missing.

1. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentInterfaceTest.php
   @@ -75,24 +75,24 @@ function testCommentInterface() {
   -    $comment = $this->postComment(NULL, $comment->comment_body->value, $comment->subject->value, array('name' => $this->web_user->getUsername()));
   -    $this->assertTrue($comment->name->value == $this->web_user->getUsername() && $comment->uid->target_id == $this->web_user->id(), 'Comment author successfully changed to a registered user.');
   +    $comment = $this->postComment(NULL, $comment->comment_body->value, $comment->getSubject(), array('name' => $this->web_user->getUsername()));
   +    $this->assertTrue($comment->getAuthorName() == $this->web_user->getUsername() && $comment->uid->target_id == $this->web_user->id(), 'Comment author successfully changed to a registered user.');
   

   I think $comment->uid->target_id in the assert should be $comment->getAuthorId()

2. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentStatisticsTest.php
   @@ -106,7 +106,7 @@ function testCommentNodeCommentStatistics() {
   -    $this->assertEqual($node->get('comment')->last_comment_name, $comment_loaded->name->value, 'The value of node last_comment_name is the name of the anonymous user.');
   +    $this->assertEqual($node->get('comment')->last_comment_name, $comment_loaded->getAuthorName(), 'The value of node last_comment_name is the name of the anonymous user.');
        $this->assertEqual($node->get('comment')->last_comment_uid, 0, 'The value of node last_comment_uid is zero.');
        $this->assertEqual($node->get('comment')->comment_count, 2, 'The value of node comment_count is 2.');
   

   seems strange to be doing these direct access to properties. but I see them set in comment.module (around line 912).

phew. read the whole thing.

next, I wanted to try the hint for patch producers in the meta which says "Ideally the visibility of the properties which are getters and setters in this case can force people to use the property directly and instead of using it directly, use it as a method so while working on the patch you can make the visibility as protected and change them back to public before RTBC"

But that is assuming there are properties. And here, looks like ->set(' ') and ->get(' ') are used not properties that could be set to protected.

So I guess I cannot make a patch that can "check" we have gotten all the replacements and not missed any. ... maybe a grep could be done?

might as well guess at something...
$ ag "comment\-\>" core

A:
comment.module template_preprocess_comment()

1481:  if (!$comment->uid->target_id) {
1486:    if ($commented_entity->hasField('uid') && $comment->uid->target_id == $commented_entity->get('uid')->value) {

should use getAuthorId() ?

as mentioned previously #2078387: Add an EntityOwnerInterface might do them? but maybe we should change there here too, and then at least they are all done, and the which ever issue gets in first can go, and the other can be rerolled to account for that.

B:
in CommentFormController form()

139:      $date = !empty($comment->date) ? $comment->date : DrupalDateTime::createFromTimestamp($comment->created->value);

should that be getCreatedTime() ?

C:
in CommentFormController submit()

323:      $comment->uid->target_id = $account->id();

should use getAuthorId() ?

D:
in CommentFormController submit()

339:      $comment->subject = Unicode::truncate(trim(String::decodeEntities(strip_tags($comment_text))), 29, TRUE);

should use setSubject() ?

E:
in CommentViewBuilder buildContent()

113:    // $comment->entity_id->value as we can load them in bulk per type.

should that comment be replaced with getCommentedEntity() ?
Like (line wrap would need correcting):

    // Load entities in bulk. This is more performant than using
    // $comment->getCommentedEntity() as we can load them in bulk per type.

or maybe
// Load entities in bulk. This is more performant than using
// $comment->entity_id->value or $comment->getCommentedEntity() as we can load them in bulk per type.

F:
in CommentAdminOverview in buildForm()

185:      $commented_entity_ids[$comment->entity_type->value][] = $comment->entity_id->value;

should that use getCommentedEntityType() and getCommentedEntityId() ?

G:
in core/modules/comment/lib/Drupal/comment/Plugin/views/field/Link.php

117:      $entity_id = $comment->entity_id;
118:      $entity_type = $comment->entity_type;
(for context) 119:      $entity = $this->entityManager->getStorageController($entity_type)->load($entity_id);

Are these (entity_id and entity_type) going to cause a problem?

H:
in CommentCSSTest

135:        $this->assertIdentical(1, count($this->xpath('//*[contains(@class, "comment")]/*[@data-comment-timestamp="' . $comment->changed->value . '"]')), 'data-comment-timestamp attribute is set on comment');

Should that be getChangedTime() ?

I:
In CommentLinksTest

169:    if ($this->node->comment->status != $info['comments']) {
239:            if ($this->loggedInUser && isset($this->comment) && !isset($this->comment->seen)) {
241:              $this->comment->seen = TRUE;
254:        $this->comment->seen = TRUE;

->status, if that were ->status->value, I would say it should be ->isPublished() But I'm not sure.

->seen ? huh? is that just in this test, and so can be ignored?

J:
in CommentNewIndicatorTest

114:    $this->assertIdentical(1, count($this->xpath('//*[@data-history-node-last-comment-timestamp="' . $comment->changed->value .  '"]')), 'data-history-node-last-comment-timestamp attribute is set to the correct value.');

K:
in core/modules/rdf/lib/Drupal/rdf/Tests/CommentAttributesTest.php

249:      'value' => $comment->subject->value,
257:      'value' => date('c', $comment->created->value),
264:      'value' => date('c', $comment->created->value),
278:    if ($comment->uid->value > 0) {
279:      $author_uri = url('user/' . $comment->uid->value, array('absolute' => TRUE));
307:    if ($comment->uid->value == 0) {

Looks like these were missed?

L:
in rdf.module

494:    if ($comment->pid->target_id != 0) {

Looks like this should be getParentComment()->id()

M:
in EntityCrudHookTest

191:    $comment->subject->value = 'New subject';

is this direct set of subject ok? CommentTokenReplaceTest uses setSubject(). Maybe this should also?

N:
in tracker.module

291:  if ($latest_comment && $latest_comment->changed > $changed) {
292:    $changed = $latest_comment->changed;

Should that use getChangedTime() ?

Not sure if @Berdir had a different place to put this hunk... but it didn't go there. :)

edit: I also diffed the patches 74 and 80 and noticed these couple niffty improvements

1.

< +  $entity = entity_load($comment->getCommentedEntityType(), $comment->getCommentedEntityId());
> +  $entity = $comment->getCommentedEntity();

berdir says:
saves code and avoids a call out to that function
so unit tests or so would be easier/possible

2.

> -  if ($entity->getEntityTypeId() == 'comment') {
> +  if ($entity instanceof CommentInterface) {

I looked at the interdiffs in #85 and #87 And they look great.

Issues to open for later might be
#78 D (a minor should live else where),
#78 G (needs fix and test coverage)
added to issue summary.

Updated issue summary especially regarding possible issues to be opened (not necessarily "follow-ups", but to be opened.)

Looking really good. What's next? One more final look by.. ? @larowlan

[edit:] I didn't delete those. See #2191619: New on-page comment form is deleting all hidden files :(

There's meta issue to clean-up tests #1847540: [META] Clean up comment module tests and decouple from node

Updated summary about permalink #2113323: Rename Comment::permalink() to not be ambiguous with ::uri()
Issue is RTBC and needs change notice draft, suppose @larowlan will fire rtbc and set 'avoid commit conflicts'

@Berdir, ah yes. :)

tiny issue summary clarifications.

I'll start a draft change record. (following https://drupal.org/contributor-tasks/draft-change-record)
[update]
Actually "New Entity Field API based upon the Typed Data API" https://drupal.org/node/1806650 covers this and the meta is already listed there.
@Berdir says we dont need to clutter that with each one of the sub issues.
Looked for an example in one of the others for what to do. In #2028037-53: Expand FeedInterface with methods @webchick says

We'll need to scour existing change notices to make sure they reflect the methods introduced here.

Maybe what we can do here is list the change records that we edit.
"Comment settings are now a field. Comments allowed on any entity type." https://drupal.org/node/2100015 might be one.

1. +++ b/core/modules/comment/comment.module
   @@ -1293,13 +1294,14 @@ function comment_preview(CommentInterface $comment, array &$form_state) {
   +      $comment->setAuthorName(check_plain($account->getUsername()));
   
   +++ b/core/modules/comment/comment.tokens.inc
   @@ -137,32 +137,27 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
   +          $replacements[$original] = $sanitize ? check_plain($comment->getHostname()) : $comment->getHostname();
   ...
   +          $replacements[$original] = $sanitize ? filter_xss($comment->getSubject()) : $comment->getSubject();
   
   @@ -180,28 +175,31 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
   +          $replacements[$original] = $sanitize ? filter_xss($name) : $name;
   ...
   +            $replacements[$original] = $sanitize ? filter_xss($parent->getSubject()) : $parent->getSubject();
   

   would have been nice to use String::checkPlain and the corresponding XSS util while touching these lines, but unless we need another reroll, leave as is

2. +++ b/core/modules/comment/comment.module
   @@ -1480,7 +1482,7 @@ function template_preprocess_comment(&$variables) {
   +    $variables['status'] = $comment->isPublished() ? 'published' : 'unpublished';
   

   out of scope: missing t() here, please fix if re-rolling

3. +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
   @@ -79,8 +79,8 @@ protected function init(array &$form_state) {
   +    $entity = $this->entityManager->getStorageController($comment->getCommentedEntityTypeId())->load($comment->getCommentedEntityId());
   
   @@ -232,8 +232,8 @@ public function form(array $form, array &$form_state) {
   +    $entity = $this->entityManager->getStorageController($comment->getCommentedEntityTypeId())->load($comment->getCommentedEntityId());
   

   Can this use CommentInterface::getCommentedEntity now?

4. +++ b/core/modules/comment/lib/Drupal/comment/CommentFormController.php
   @@ -320,28 +320,29 @@ public function submit(array $form, array &$form_state) {
   +    if (!$comment->is_anonymous && !empty($author_name) && ($account = user_load_by_name($author_name))) {
   

   Mental note: we need to inject the User storage controller here (needs follow up).

5. +++ b/core/modules/comment/lib/Drupal/comment/CommentInterface.php
   @@ -27,6 +27,231 @@
   +   * @return \Drupal\Core\Entity\EntityInterface
   ...
   +   * @return \Drupal\comment\CommentInterface
   ...
   +   * @return \Drupal\comment\CommentInterface
   ...
   +   * @return \Drupal\comment\CommentInterface
   ...
   +   * @return \Drupal\comment\CommentInterface
   ...
   +   * @return \Drupal\comment\CommentInterface
   ...
   +   * @return \Drupal\comment\CommentInterface
   ...
   +   * @return \Drupal\comment\CommentInterface
   ...
   +   * @return \Drupal\comment\CommentInterface
   

   I believe new coding standard is these should just say @return $this with no description/other comments

6. +++ b/core/modules/comment/lib/Drupal/comment/CommentInterface.php
   @@ -27,6 +27,231 @@
   +  /**
   +   * Returns the field ID of the comment field the comment is attached to.
   +   *
   +   * @return string
   +   *   The field identifier of the field the comment is attached to.
   +   */
   +  public function getFieldId();
   

   i don't think this is any different to CommentInterface::bundle(), I think we can replace all calls with ::bundle() instead and remove the duplication?

7. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
   @@ -60,156 +60,7 @@ class Comment extends ContentEntityBase implements CommentInterface {
   -   * The comment ID.
   -   *
   -   * @var \Drupal\Core\Field\FieldItemListInterface
   -   */
   -  public $cid;
   -
   -  /**
   -   * The comment UUID.
   -   *
   -   * @var \Drupal\Core\Field\FieldItemListInterface
   -   */
   -  public $uuid;
   -
   -  /**
   -   * The parent comment ID if this is a reply to another comment.
   -   *
   -   * @var \Drupal\Core\Field\FieldItemListInterface
   -   */
   -  public $pid;
   -
   -  /**
   -   * The entity ID for the entity to which this comment is attached.
   -   *
   -   * @var \Drupal\Core\Field\FieldItemListInterface
   -   */
   -  public $entity_id;
   -
   

   love it!

8. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
   @@ -221,23 +72,21 @@ public function id() {
   +        if (! $this->hasParentComment()) {
   

   extra space?

9. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
   @@ -280,24 +129,24 @@ public function preSave(EntityStorageControllerInterface $storage_controller) {
   -      $this->hostname->value = \Drupal::request()->getClientIP();
   
   @@ -452,8 +301,192 @@ public static function baseFieldDefinitions($entity_type) {
   +    return entity_load($entity_type, $entity_id);
   

   Mental note: we really need to be able to inject into Entity classes. entity_load (puke)

10. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
    @@ -343,7 +192,7 @@ public static function postDelete(EntityStorageControllerInterface $storage_cont
    -    $entity = entity_load($this->get('entity_type')->value, $this->get('entity_id')->value);
    +    $entity = $this->getCommentedEntity();
    

    schmick!

11. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/Link.php
    @@ -114,8 +115,8 @@ protected function renderLink($data, ResultRow $values) {
    -      $entity_id = $comment->entity_id;
    -      $entity_type = $comment->entity_type;
    +      $entity_id = $comment->getCommentedEntityId();
    +      $entity_type = $comment->getCommentedEntityTypeId();
           $entity = $this->entityManager->getStorageController($entity_type)->load($entity_id);
    

    Can be simplified to CommentInterface::getCommentedEntity()?

Assuming bot agrees

comment-interface-2028025.94.patch no longer applies.

error: patch failed: core/modules/tracker/tracker.module:206
error: core/modules/tracker/tracker.module: patch does not apply

looks like this passed the testbot. so setting to needs review.

Draft change records are now required before commit. I searched for CommentInterface in the existing change records and none of the matches for that will need an update.

+++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
@@ -60,156 +60,7 @@ class Comment extends ContentEntityBase implements CommentInterface {
-  public $cid;
...
-  public $uuid;
...
-  public $pid;
...
-  public $entity_id;
...
-  public $entity_type;
...
-  public $field_id;
...
-  public $langcode;
...
-  public $subject;
...
-  public $uid;
...
-  public $name;
...
-  public $mail;
...
-  public $homepage;
...
-  public $hostname;
...
-  public $created;
...
-  public $changed;
...
-  public $status;
...
-  public $thread;

So I think these are all the deleted thingers that we'd need to search the change records for?

https://drupal.org/node/2100015 only references comment count, which is not changed per the summary, so that one is still okay I think.

97: comment-interface-2028025.97.patch queued for re-testing.

@alexpott asked me about #102 and whether I deliberately didn't set the issue NW. To clarify, I was replying to @YesCT's comments in #92. https://drupal.org/node/1806650 is the change record for the meta and is nicely written to cover all these changes, linking off to the API documentation, so this issue already is covered as far as change records go. (Though, that change record needs to have a reference to this issue added.)

It would be good for someone to look around a little in comment change records for references to the removed comment member variables in #102, but based on my review of what's currently there, I think that need not block commit and the issue is still RTBC.

Filed related critical #2197723: Comment::getFieldId() should return actual field ID

The DX win means that this should be major. Can someone add this issue to https://drupal.org/node/1806650?

Committed f3732c6 and pushed to 8.x. Thanks!

added reference to https://drupal.org/node/1806650