Move {comment_entity_statistics} to proper service

Makes sense

working on this

First crack.
Adds an interface.
Not sure what to do with forum and tracker that query it direct.
Best I could muster would be some sort of queryAddCommentStatistics method that takes a query object, an array of fields, an array of joins and an array of expressions.
But that sounds really hacky, we have a query alter engine for just that already.
Thoughts?

entity type changes went in

yeah select() != insert()

meh

read/write/delete makes sense.
Suppose initializeRecord() should check for comment fields and throw exception when no comment fields found.

what about write -> updated
initializeRecord -> create
then we have CRUD, consistent

2. Agreed

3. But the service can be swapped, this is the closest thing to a service implementing a hook we have at the moment

We already ignore non-existant fields in the initializeRecord (now ::create) method

// Skip fields that entity does not have.
      if (!$entity->hasField($field_name)) {
        continue;
      }

Seems alright. We can add some awesome unit tests

Just a note about hook_ranking... This is an ancient hook for Node Search that allows modules to say "This is a way of ranking node search results".

The hook is invoked at configuration time to find the name of the module's ranking(s) (in this case, the ranking is by number of comments on the node), and then at search time, to figure out what to join to the search query in order to rank result nodes by number of comments.

The core NodeSearch plugin for searching nodes does use a SQL query to do the search, so it needs to be able to join to a table of some sort in order to rank nodes by number of comments. We don't have an issue at this time in the Search module to redo how rankings are done (convert from a hook to plugins or something like that)... so we're probably stuck with needing to join to a query.

12: comment-statistics-service.12.patch queued for re-testing.

reroll

Another note: The comment ranking stuff for Node search is not actually working at the moment in Core. See
#893302: Search ranking based on the factor "number of comments" & "No of Page Views" is broken

That issue is RTBC... I would advise waiting on this patch until that one has gone in, because different code will need to be put into the ranking stuff in this patch.

Thanks, following that one now

18: comment-statistics-service.17.patch queued for re-testing.

meh

1. +++ b/core/modules/comment/comment.module
   @@ -922,54 +917,8 @@ function comment_entity_insert(EntityInterface $entity) {
   +    \Drupal::service('comment.statistics')->create($entity, $fields);
   

   +1 for not adding into again into the comment manager.

2. +++ b/core/modules/comment/lib/Drupal/comment/CommentStatistics.php
   @@ -0,0 +1,238 @@
   +
   +class CommentStatistics implements CommentStatisticsInterface {
   

   Soome kind of documentation would be cool

3. +++ b/core/modules/comment/lib/Drupal/comment/CommentStatistics.php
   @@ -0,0 +1,238 @@
   +      if (!isset($last_comment_uid)) {
   ...
   +      }
   ...
   +      if ($entity instanceof EntityOwnerInterface) {
   +        $last_comment_uid = $entity->getOwnerId();
   

   what about just using else? I would guess that the EntitOwnerInterface guaranties ensures an owner.

4. +++ b/core/modules/comment/lib/Drupal/comment/CommentStatisticsInterface.php
   @@ -0,0 +1,82 @@
   +   * @return object[]
   

   I guess they are stdClass es?

what about just using else? I would guess that the EntitOwnerInterface guaranties ensures an owner.

Still returns NULL for some implementations (entity_test), which {comment_statistics} schema doesn't allow. The fail above shows this (thats why my comment was 'meh' - HEAD does the same as this).
Fixed docs issues
Thanks for the review

A few docs things to fix, mostly on the new Interface:

a)

+   * @return \StdClass[]
+   *   Array of statistics records keyed by entity id.
+   */
+  public function read($entities, $entity_type);

In contrast to #26 item 4, if these really are stdClass objects, the right thing is @return object[].
https://drupal.org/node/1354#types

b)

+  /**
+   * Read comment statistics records for an array of entities.

All functions should start with a 3rd-person verb like "Reads" not "Read". Several are not.

c)

+   * @param \Drupal\Core\Entity\EntityInterface[] $entities
+   *   Array of entities on which commenting is enabled, keyed by id

Description should end in . and also "id" should be "ID" (id == ego, superego, etc.; ID == identifier).

d) typo:

+   * @return int
+   *   The maximum number of comments for and entity of the given type.

and -> an

e) And on the Controller:

+   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_info
+   *   An array of entity info for the entity type.

Is it an array or an interface or an array of interfaces? Type and docs do not agree.

f) Total nitpick on the Controller:

+  }
+
+
+  /**

Probably only want one blank line here? This is between createInstance() and buildQuery().

Fixes #27

We should write PHPCS sniffs for some of those items, as none were highlighted by my IDE, attention to detail++

The feedback from jhodgon got adressed.

Andypost questioned whether services should depend on the current user. I would argue at the moment that it is not a problem if they are, as even if it will be changed, we can change it.

In comment #21 I asked if this issue could wait until #893302: Search ranking based on the factor "number of comments" & "No of Page Views" is broken is taken care of... The code you've put into the statistics stuff is not actually working code. Could we postpone this or is it urgent?

That other issue had been at RTBC for ages until webchick put it back to "needs review" yesterday, and these two issues clash... I just don't really want to get the current non-working code put into a completely different spot in Core when there's another issue that is trying to make sure the code actually works (with tests) out there.

I did not intentionally delete those files. See
#2191619: New on-page comment form is deleting all hidden files :(
Terrible bug!!!!!!

Happy to wait

#893302: Search ranking based on the factor "number of comments" & "No of Page Views" is broken got committed.

However, I found a PostgreSQL bug in the statistics.module code that is very similar to the comment.module code, so I'm addressing both on #2195907: Search ranking for number of views fails in PostgreSQL, which has a working patch (I hope), which again touches the comment_ranking() and comment_cron() code that this patch is using too.

And by the way, since this issue is closer to being done, I won't ask again for this one to be postponed, I guess.

29: comment-stats.28.patch queued for re-testing.

re-roll after #2028025: Expand CommentInterface to provide methods

1. +++ b/core/modules/comment/lib/Drupal/comment/CommentStatistics.php
   @@ -0,0 +1,244 @@
   +        // Inverse law that maps the highest reply count on the site to 1 and 0 to 0.
   

   Comment overflows the 80 char limit.

2. +++ b/core/modules/comment/lib/Drupal/comment/CommentStatistics.php
   @@ -0,0 +1,244 @@
   +          'last_comment_name' => $last_reply->uid ? '' : $last_reply->name,
   

   Is this the wrong way around?

Just a note to reviewers/committers: this will conflict with #2195907: Search ranking for number of views fails in PostgreSQL, which has updated code that will need to get into the hook_cron and hook_ranking pieces of this patch (or the new methods they call) so that it will consistently work with PostgreSQL.

Setting this to Needs Work on the basis of #41.

Also, just a thought that the code in the hook_cron() would be easier to understand if it explicitly passed in 'node' as the entity type when calling the getMaximumCount() method. Without that, it is not obvious until you go to the interface/class docs (which might not be easy to find and/or even know what class it is if you are in comment.module) that it's getting the max count for nodes by default. Putting that in there would help self-document the code. Talking about here:

 function comment_cron() {
   // Store the maximum possible comments per thread (used for node search
   // ranking by reply count).
-  \Drupal::state()->set('comment.node_comment_statistics_scale', 1.0 / max(1, db_query('SELECT MAX(comment_count) FROM {comment_entity_statistics}')->fetchField()));
+  \Drupal::state()->set('comment.node_comment_statistics_scale', 1.0 / max(1, \Drupal::service('comment.statistics')->getMaximumCount()));
 }

making that last line say getMaximumCount('node'). Also just to point out that the line being removed did not specify an entity type, so technically this is a change in behavior; however, it's the right change in behavior for this purpose, since it's being used by the NodeSearch plugin for ranking search results on nodes.

Fixed 41.1
Re 41.2, we don't store the commenter's name if they're logged in, we have that in {users}.name - this is to store anonymous users's name, as entered on the comment form.

interdiff

Since we're leaning towards been explicit I don't see why we should even have the default on the interface?

public function getMaximumCount($entity_type = 'node');

Could we just remove it?

fixes 45

Looks good to me.

Ummmm... What is this:

   if (\Drupal::state()->get('comment.maintain_entity_statistics') &&

It seems like this is inconsistently used in the code. For instance, I only see it being called once, and if we are not maintaining comment entity statistics, then the comment ranking won't work, right? And wouldn't it be configuration rather than state anyway? Very confusing. Maybe out of scope for this patch, but ... it seems like if you're going to have this as a service then the service should be configured to be on/off consistently?

Its for upgrade paths only, in fact migrate now relies on it for its stubbing.

RE #50: a comment to that effect might be useful. :)

comment-statistics-service.46.patch no longer applies.

error: patch failed: core/modules/comment/comment.module:11
error: core/modules/comment/comment.module: patch does not apply

Straight reroll

Can we add a code comment as suggested in #49-51?

1. +++ b/core/modules/comment/comment.module
   @@ -926,54 +921,7 @@ function comment_entity_insert(EntityInterface $entity) {
      // maintenance of the {comment_entity_statistics} table.
      if (\Drupal::state()->get('comment.maintain_entity_statistics') &&
        $fields = \Drupal::service('comment.manager')->getFields($entity->getEntityTypeId())) {
   -    $query = db_insert('comment_entity_statistics')
   

   So, you want the comment here?

2. +++ b/core/modules/comment/lib/Drupal/comment/CommentStatistics.php
   @@ -0,0 +1,245 @@
   +  public function update(CommentInterface $comment) {
   +    // Allow bulk updates and inserts to temporarily disable the maintenance of
   +    // the {comment_entity_statistics} table.
   +    if (!$this->state->get('comment.maintain_entity_statistics')) {
   +      return;
   +    }
   

   should similar comment go here? Or is the comment already explaining the same?

RE #57, yes. In comment #49, I noticed this line of code and had no idea what it meant. larowland explained it in comment #50, and it seemed to me that a comment in the code explaining it might be sensible, since it wasn't obvious what it was about. And yes, in both places I think.

Regarding that second spot, this seems to contradict the answer I got in #50. Let's figure out what it's actually for and document it please?

There is already this comment in both places:

  // Allow bulk updates and inserts to temporarily disable the
  // maintenance of the {comment_entity_statistics} table.

Do we really need anything more than that? This already exists in HEAD.

Re-roll.

This now blocks #2205215: {comment} and {comment_entity_statistics} only support integer entity ids (which is critical because it fixes some SQL issues now that entity-ids can be strings) which also provides the scope to make this a UI setting per field.

Can we leave the comment as is and make it configurable per-field over there?

RE #60, OK.

We had a PostgreSQL issue in the comment rankings for search and its patch was just committed:
#2195907: Search ranking for number of views fails in PostgreSQL
(I made note previously on both issues that they would conflict)...

So
a) This patch probably needs a reroll.
b) This patch needs the new code for the ranking stuff currently in the newly-updated comment_ranking() put into
the new class's getRankingInfo() method.

Some of the ranking changes match #2205215: {comment} and {comment_entity_statistics} only support integer entity ids changes - nice!
Re-roll

Thanks for the quick reroll! The search ranking stuff looks correct to me. Back to RTBC. I'm assuming that was all you changed in this reroll (aside from any other context stuff that needed rerolling).

comment-statistics-service.64.patch no longer applies.

error: patch failed: core/modules/comment/lib/Drupal/comment/CommentStorageController.php:47
error: core/modules/comment/lib/Drupal/comment/CommentStorageController.php: patch does not apply

reroll, it was just comments, coding standards changes

comment-statistics-service.67.patch no longer applies.

error: patch failed: core/modules/comment/comment.module:11
error: core/modules/comment/comment.module: patch does not apply

72: comment-statistics-service.72.patch queued for re-testing.

72: comment-statistics-service.72.patch queued for re-testing.

Committed/pushed to 8.x, thanks!

Reopening issue, because the error I found is purely related to this one, and because I'd like to ask you something.

The read() method does not say what the interface documentation says it does. It does not return an array but a Database\Statement (iterable) object. Fix attached. Also attached for clarity / how I caught this: a patch that should be part of #2068331: Convert comment SQL queries to the Entity Query API but isn't yet, because its last changed line generates a "Fatal error: Cannot use object of type Drupal\Core\Database\Statement as array"

So, question. Does this fact mean there should be a (unit?) test for this?
(Or instead, is there already (going to be) some magical thing that tests whether all output types of all classes match the documentation?)

Please do not reopen fixed issues. Instead, open a new issue, and mark it as "related" to this issue. Then come back to this issue and add a comment about your new issue.

OK, will do in future. #2259209: Fix CommentStatistics::read()

FYI: followup that cleans up some existing queries/code to use the CommentStatistics service: for review at #2280861: CommentStatistics service followup