This code is split out from #2068331-7: Convert comment SQL queries to the Entity Query API (#79 ish) ...

... but I consider this a standalone followup to finish off #2101183: Move {comment_entity_statistics} to proper service. It just needed #2259209: Fix CommentStatistics::read() committed first.

1)
With CommentStatistics being a service, we don't need CommentStorage::updateEntityStatistics() to exist. *Decouple*

2)
Added service container to a test to make it pass.

3)
There still was an SQL query that needed to use the service, in Tracker module. Fix this.
(Note IMHO the 'filtering on desired language' comment is not so relevant here. The whole CommentStatistics service either should or should not be updated to be able to store different per-language statistics... Either way it's not up to this specific query.)

3a)
The SQL query tried to use a replica server. So add an option to CommentStatisticsInterface::read() to accommodate that.

4)
Related and unrelated comments/small cleanups.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

roderik’s picture

Issue summary: View changes
andypost’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +API cleanup

looks great

Status: Reviewed & tested by the community » Needs work

The last submitted patch, comment-statistics-followup.patch, failed testing.

roderik’s picture

Status: Needs work » Needs review
FileSize
8.18 KB

Trivial reroll: s/slave/replica/ in old deleted code. So this could be immediately RTBC again.

...BUT...

roderik’s picture

...the 'replica' (use slave server) option is something I removed in the above patch. I guess this option was only there for the call from tracker.module, not for other uses of CommentStatistics classes.
(Since: #615526: Use slave servers for tracker module)

So in order to keep that, we could add an extra option to the read() method, maybe?

roderik’s picture

Rerolled, and added #5 to the issue summary.

The after-reroll interdiff contains:

Related to that last point, there's something I want to run by y'all. But if this one gets reviewed quickly, I'll leave that for a new issue, so this one won't get cluttered more ;-)

pcambra’s picture

Plain reroll of this, some of the changes related to the date have made it already into core.

andypost’s picture

I think the comment should be adjusted, no reason to remove

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -76,8 +76,6 @@ public function preSave(EntityStorageInterface $storage) {
-          // As preSave() is protected, this can only happen when this class
-          // is extended in a faulty manner.

any reason to remove the comment?

pcambra’s picture

You're right, setting back the comment.

roderik’s picture

Yes, there is reason to remove the comment: it's about preSave() being protected - but preSave() isn't protected anymore.

according to 'git blame', someone changed it to public- but forgot to remove the comment. Has nothing to do with the rest of the patch, so I understand the confusion. (This patch turned into a general sweep-up operation.)

requesting further review on the basis of #7

andypost’s picture

I mean to fix comment, because it's not clear how this could throw exception,

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -76,6 +76,8 @@ public function preSave(EntityStorageInterface $storage) {
       if (empty($thread)) {
         if ($this->threadLock) {
+          // As preSave() is protected, this can only happen when this class
+          // is extended in a faulty manner.
           throw new \LogicException('preSave is called again without calling postSave() or releaseThreadLock()');

previous thread lock was not released?

roderik’s picture

Ah, OK. Good idea, I have no strong opinion on it.

Tried to add things without duplicating information from the error message.

roderik’s picture

Nitpick: in hindsight, don't like the grammar.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Awesome!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

@roderik general sweep up operations are difficult to review. Patch with lots of unrelated changes

  1. +++ b/core/modules/comment/src/CommentStatisticsInterface.php
    @@ -32,11 +32,15 @@ public function getRankingInfo();
    +   * @param boolean $accurate
    +   *   An indicator of whether the results must be completely up to date.
    +   *   If set to FALSE, the function may return slightly out of date information
    +   *   while using less processing time.
    

    The statement of using less processing time is not necessarily true. A better param doc would be something like: (optional) Indicates if results must be completely up to date. If set to FALSE, a replica database will used if available. Defaults to TRUE.

  2. +++ b/core/modules/tracker/tracker.pages.inc
    @@ -78,7 +74,7 @@ function tracker_page($account = NULL) {
    -          $comments .= l(format_plural($new, '1 new', '@count new'), 'node/' . $node->id(), array('fragment' => 'new'));
    +          $comments .= l(\Drupal::translation()->formatPlural($new, '1 new', '@count new'), 'node/' . $node->id(), array('fragment' => 'new'));
    

    Very unrelated change.

roderik’s picture

Status: Needs work » Needs review
FileSize
10.87 KB
2.25 KB

1. Thanks! Took over the suggested text literally.

2. deleted

More sweep up has been tucked into #2318875: Redo CommentStatisticsInterface already ;)

Status: Needs review » Needs work

The last submitted patch, 16: interdiff-2280861-13-16.patch, failed testing.

dawehner’s picture

+++ b/core/modules/comment/src/CommentStatisticsInterface.php
@@ -32,11 +32,14 @@ public function getRankingInfo();
+  public function read($entities, $entity_type, $accurate = TRUE);

Did you considered to typehint $entities as array?

roderik’s picture

Status: Needs work » Reviewed & tested by the community

Thanks for the suggestion, I missed that! I'm going to set this to RTBC now because

  • no code change since last RTBC, only comments change according to suggestion
  • there was no test failure (just me using wrong extension for interdiff)
  • I'll do the typehint in #2318875: Redo CommentStatisticsInterface, it fits there
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 36675ea and pushed to 8.0.x. Thanks!

  • alexpott committed 36675ea on 8.0.x
    Issue #2280861 by roderik, pcambra: CommentStatistics service followup.
    

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

gisle’s picture

Issue tags: -API cleanup +API clean-up

Official tag is "API clean-up" - https://www.drupal.org/node/1207020