Redo CommentStatisticsInterface

1. +++ b/core/modules/comment/comment.module
   @@ -194,6 +194,9 @@ function comment_field_instance_config_delete(FieldInstanceConfigInterface $inst
   +    // TODO delete comment statistics. After #2338457 gets fixed we may have a
   +    //      list of entity IDs here, to pass to CommentStatistics::delete().
   +    //      Until then: unimportant, since the above always deletes 0 comments.
   

   Lets leave this for the dedicated issue

2. +++ b/core/modules/comment/src/CommentStatistics.php
   @@ -68,35 +66,69 @@ public function __construct(Connection $database, AccountInterface $current_user
   +  public function delete(array $entity_ids, $entity_type, array $field_names = array()) {
   +    if ($entity_ids && $entity_type) {
   +      $query = $this->database->delete('comment_entity_statistics')
   +        ->condition('entity_id', $entity_ids, 'IN')
   +        ->condition('entity_type', $entity_type);
   +      if ($field_names) {
   +        $query->condition('field_name', $field_names, 'IN');
   +      }
   +      $query->execute();
   +    }
   

   this feels to me like we need both a delete (as in HEAD) and a deleteMultiple (as per this patch)

3. +++ b/core/modules/comment/src/CommentStatistics.php
   @@ -68,35 +66,69 @@ public function __construct(Connection $database, AccountInterface $current_user
   +      // This means $field_names are attached to this entity type, but we still
   +      // need to check this for the specific entity('s bundle).
   

   maybe merge this with the // Skip fields that entity does not have comment lower down?

4. +++ b/core/modules/comment/src/CommentStatistics.php
   @@ -178,84 +198,100 @@ public function getRankingInfo() {
   +      // This means $field_names are attached to this entity type, but we still
   +      // need to check this for the specific entity('s bundle).
   

   same?

5. +++ b/core/modules/comment/src/CommentStatisticsInterface.php
   @@ -28,34 +27,57 @@ public function getRankingInfo();
   +   *   The name(s) of the comment field(s) to read statistics for. If not
   +   *   specified / if there are multiple comment fields on the entity which
   +   *   have statistics, multiple records per entity are returned for each field.
   ...
   +   *   The name(s) of the comment field(s) whose statistics records to delete.
   +   *   If not specified, all records for the specified ids will be deleted.
   ...
   +   * @param array $field_names
   +   *   The name(s) of the comment field(s) whose statistics records to updated.
   +   *   If not specified, all records for the entity will be updated.
   
   @@ -73,13 +95,14 @@ public function update(CommentInterface $comment);
   +   * @param array $field_names
   +   *   The name(s) of the comment field(s) which are defined for the entity
   +   *   type. Separate statistics records will be inserted for each field name.
   

   Needs (optional) at start.

6. +++ b/core/modules/comment/tests/src/Unit/CommentStatisticsUnitTest.php
   @@ -91,6 +82,81 @@ protected function setUp() {
   +   * Sets up methods required to mock an iterator
   +   *
   +   * @param \PHPUnit_Framework_MockObject_MockObject $iteratorMock The mock to attach the iterator methods to
   +   * @param array $items The mock data we're going to use with the iterator
   +   * @return \PHPUnit_Framework_MockObject_MockObject The iterator mock
   +   */
   +  protected function mockIterator(\PHPUnit_Framework_MockObject_MockObject $iteratorMock, array $items) {
   

   This is awesome - should be part of UnitTestCase so other modules/tests can use it.

+++ b/core/modules/comment/src/CommentStatisticsInterface.php
@@ -58,15 +58,28 @@ public function read(array $entities, $entity_type, array $field_names = array()
-  public function delete(array $entity_ids, $entity_type, array $field_names = array());
+  public function deleteMultiple(array $ids, $entity_type, array $field_names = array());

if this changing now, lets move entity_type before IDs
Any reason for this method? no usage...

1. +++ b/core/modules/comment/src/CommentStatistics.php
   @@ -68,35 +66,83 @@ public function __construct(Connection $database, AccountInterface $current_user
   +      if ($ids) {
   

   This doesn't seem to be optional - so passing an empty array here (which will return FALSE) will delete all records? That seems wrong.

2. +++ b/core/modules/comment/src/CommentStatisticsInterface.php
   @@ -28,34 +27,70 @@ public function getRankingInfo();
   +  public function deleteMultiple($entity_type, array $ids, array $field_names = array());
   

   Not seeing any test coverage for this

Current code affected too, filed #2422443: Fix default value of author in \Drupal\comment\CommentStatistics::create()

+++ b/core/modules/comment/src/CommentStatistics.php
@@ -68,35 +66,82 @@ public function __construct(Connection $database, AccountInterface $current_user
+    // Get the user ID from the entity if it's set, or default to the
+    // currently logged in user.
+    $last_comment_uid = 0;
+    if ($entity instanceof EntityOwnerInterface) {
+      $last_comment_uid = $entity->getOwnerId();
+    }
+    if (!isset($last_comment_uid)) {
+      // Default to current user when entity does not implement
+      // EntityOwnerInterface or author is not set.
+      $last_comment_uid = $this->currentUser->id();
+    }

this is a current bug in code, just copy/paste.
$last_comment_id never get current user ID

Failed with
Undefined property: Drupal\comment\Tests\CommentStatisticsTest::$web_userDrupal\comment\Tests\CommentStatisticsTest->testCommentNodeCommentStatistics() Drupal\simpletest\TestBase->run(Array) simpletest_script_run_one_test('116', 'Drupal\comment\Tests\CommentStatisticsTest') Notice CommentStatisticsTest.php 122

+++ b/core/modules/comment/src/CommentStatistics.php
@@ -68,35 +66,82 @@ public function __construct(Connection $database, AccountInterface $current_user
+    $last_comment_uid = 0;

please remove this line

Anther round on review, mostly to follow 8.x.

1. +++ b/core/modules/comment/src/CommentStatistics.php
   @@ -68,35 +66,82 @@ public function __construct(Connection $database, AccountInterface $current_user
   +    if (!$field_names) {
   +      $map = $this->entityManager->getFieldMapByFieldType('comment');
   +      $entity_type = $entity->getEntityTypeId();
   +      if (!isset($map[$entity_type])) {
   +        return;
   +      }
   +      $field_names = array_keys($map[$entity_type]);
   
   @@ -180,84 +213,99 @@ public function getRankingInfo() {
   +    if (!$field_names) {
   +      $map = $this->entityManager->getFieldMapByFieldType('comment');
   +      $entity_type = $entity->getEntityTypeId();
   +      if (!isset($map[$entity_type])) {
   +        return;
   +      }
   +      $field_names = array_keys($map[$entity_type]);
   ...
   -    $this->entityManager->getStorage($comment->getCommentedEntityTypeId())->resetCache(array($comment->getCommentedEntityId()));
   +    $this->entityManager->getStorage($entity->getEntityTypeId())->resetCache(array($entity->id()));
   

   2 places uses same code - there's comment manager getFields() method for that

2. +++ b/core/modules/comment/src/CommentStatistics.php
   @@ -68,35 +66,82 @@ public function __construct(Connection $database, AccountInterface $current_user
   +    $last_comment_uid = 0;
   +    if ($entity instanceof EntityOwnerInterface) {
   +      $last_comment_uid = $entity->getOwnerId();
   +    }
   +    if (!isset($last_comment_uid)) {
   

   just needs ELSE

3. +++ b/core/modules/comment/src/CommentStatistics.php
   @@ -68,35 +66,82 @@ public function __construct(Connection $database, AccountInterface $current_user
   +    // Default to REQUEST_TIME when entity does not have a changed property.
   +    $last_comment_timestamp = REQUEST_TIME;
   

   Request time should be taken from server vars, to allow unit-testing https://www.drupal.org/node/2463059

queued to test with postgres
in general changes makes sense, API changes could be minified but anyway requires BC to be commited in 8.x minors

OTOH we need to solve related #2495087: comment_entity_storage_load() is too expensive on cold caches first, to make sure that we have optimal statistics loading, then decide which arguments are really needed for the service

Also we can add new implementation of service with different name and leave BC wrapper in old service

1. +++ b/core/modules/comment/src/CommentStatistics.php
   @@ -57,46 +62,96 @@ class CommentStatistics implements CommentStatisticsInterface {
   +  public function create(FieldableEntityInterface $entity, array $field_names = array()) {
   

   if we change interface then better to pass request object here (with fallback to \Drupal::request()

2. +++ b/core/modules/comment/src/CommentStatistics.php
   @@ -57,46 +62,96 @@ class CommentStatistics implements CommentStatisticsInterface {
   +    $last_comment_timestamp = $_SERVER['REQUEST_TIME'];
   +    // @todo Make comment statistics language aware and add some tests.
   +    if ($entity instanceof EntityChangedInterface) {
   

   it got rework in #2422443: Fix default value of author in \Drupal\comment\CommentStatistics::create()

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.