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.
Comment | File | Size | Author |
---|---|---|---|
#16 | interdiff-2280861-13-16.patch | 2.25 KB | roderik |
#16 | 2280861-16-comment-statistics-followup.patch | 10.87 KB | roderik |
Comments
Comment #1
roderikComment #2
andypostlooks great
Comment #4
roderikTrivial reroll: s/slave/replica/ in old deleted code. So this could be immediately RTBC again.
...BUT...
Comment #5
roderik...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?
Comment #6
roderikRerolled, 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 ;-)
Comment #7
pcambraPlain reroll of this, some of the changes related to the date have made it already into core.
Comment #8
andypostI think the comment should be adjusted, no reason to remove
any reason to remove the comment?
Comment #9
pcambraYou're right, setting back the comment.
Comment #10
roderikYes, 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
Comment #11
andypostI mean to fix comment, because it's not clear how this could throw exception,
previous thread lock was not released?
Comment #12
roderikAh, OK. Good idea, I have no strong opinion on it.
Tried to add things without duplicating information from the error message.
Comment #13
roderikNitpick: in hindsight, don't like the grammar.
Comment #14
andypostAwesome!
Comment #15
alexpott@roderik general sweep up operations are difficult to review. Patch with lots of unrelated changes
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.
Very unrelated change.
Comment #16
roderik1. Thanks! Took over the suggested text literally.
2. deleted
More sweep up has been tucked into #2318875: Redo CommentStatisticsInterface already ;)
Comment #18
dawehnerDid you considered to typehint $entities as array?
Comment #19
roderikThanks for the suggestion, I missed that! I'm going to set this to RTBC now because
Comment #20
alexpottCommitted 36675ea and pushed to 8.0.x. Thanks!
Comment #23
gisleOfficial tag is "API clean-up" - https://www.drupal.org/node/1207020