Convert comment SQL queries to the Entity Query API

Working on this.

This patch is not finished. It includes first batch of conversions. Posting here mostly for my reference and to check with testbot if agrees with it.

#2: 2068331_2.patch queued for re-testing.

This should fix tests. No new conversions yet.

Some more conversions.

Just confirming that I'm still working on this. I am almost there, just need to fix few more tests.

Wow... this was one hell of a re-reoll. Comments field patch created a total mess here.

This should finally make tests pass. Some more conversions needed.

This should be the last set of relevant conversions.

#25: 2068331_25.patch queued for re-testing.

This should now be ready for review.

The patch is awesome! there's a some questions about moving part of functions to storage controller, probably most of them should live in CommentManager - not sure which one better!?

Contrib could swap manager and storage to more performant implementation so:
1) once storage changed for example to mongo then better to override the storage controller but other places should be fixed anyway
2) once manager changed then some functionality could be changed (number of items for example)

So I think better to discuss the issue a bit more and maybe split to parts to sort implementations to proper places.

Also I linked related issues to changed parts

1. +++ b/core/modules/comment/comment.module
   @@ -267,9 +267,7 @@ function comment_menu_alter(&$items) {
    function comment_count_unpublished() {
   -  $count = db_query('SELECT COUNT(cid) FROM {comment} WHERE status = :status', array(
   -    ':status' => COMMENT_NOT_PUBLISHED,
   -  ))->fetchField();
   +  $count = \Drupal::entityQuery('comment')->condition('status', COMMENT_NOT_PUBLISHED)->count()->execute();
   

   Related issue #2111357: Get rid of comment_count_unpublished() in favor of CommentStorage method

2. +++ b/core/modules/comment/comment.module
   @@ -361,30 +359,7 @@ function comment_permission() {
    function comment_get_recent($number = 10) {
   ...
   +  return \Drupal::entityManager()->getStorageController('comment')->loadRecent($number);
   
   @@ -474,7 +413,7 @@ function theme_comment_block($variables) {
      foreach (comment_get_recent($number) as $comment) {
   -    $items[] = l($comment->subject, 'comment/' . $comment->cid, array('fragment' => 'comment-' . $comment->cid)) . '&nbsp;<span>' . t('@time ago', array('@time' => format_interval(REQUEST_TIME - $comment->changed))) . '</span>';
   +    $items[] = l($comment->subject->value, 'comment/' . $comment->id(), array('fragment' => 'comment-' . $comment->id())) . '&nbsp;<span>' . t('@time ago', array('@time' => format_interval(REQUEST_TIME - $comment->getChangedTime()))) . '</span>';
   

   Related #2054993: Remove (untested, unused, broken) comment_get_recent()

3. +++ b/core/modules/comment/comment.module
   @@ -418,44 +393,8 @@ function comment_new_page_count($num_comments, $new_replies, EntityInterface $en
   +    // Threaded comments.
   +    $pageno = \Drupal::entityManager()->getStorageController('comment')->getThreadedNewCommentPage($entity, $new_replies, $comments_per_page, $field_name);
   

   Suppose the CommentManager is better place for that.

4. +++ b/core/modules/comment/comment.module
   @@ -674,106 +613,10 @@ function comment_add(EntityInterface $entity, $field_name = 'comment', $pid = NU
   + *   An array of the commment objects.
     */
    function comment_get_thread(EntityInterface $entity, $field_name, $mode, $comments_per_page) {
   ...
   +  return \Drupal::entityManager()->getStorageController('comment')->loadThread($entity, $field_name, $mode, $comments_per_page);
   

   Let's deprecate|remove this one once the result is changed anyway

5. +++ b/core/modules/comment/comment.module
   @@ -1007,11 +850,7 @@ function comment_entity_load($entities, $entity_type) {
   +  $result = \Drupal::entityManager()->getStorageController('comment')->getEntityCommentStatistics(array_keys($entities), $entity_type);
   
   @@ -1032,48 +871,7 @@ function comment_entity_insert(EntityInterface $entity) {
   +    \Drupal::entityManager()->getStorageController('comment')->initCommentStatistics($entity, $fields);
   
   @@ -1081,17 +879,12 @@ function comment_entity_insert(EntityInterface $entity) {
   +  \Drupal::entityManager()->getStorageController('comment')->deleteCommentStatistics($entity);
   

   The related issues
   #2101183: Move {comment_entity_statistics} to proper service
   #148849: Refactor {comment_entity_statistics} into performant Field

6. +++ b/core/modules/comment/comment.module
   @@ -1134,8 +927,7 @@ function comment_node_update_index(EntityInterface $node, $langcode) {
   -      if ($node->get($field_name)->status && $cids = comment_get_thread($node, $field_name, $mode, $comments_per_page)) {
   -        $comments = comment_load_multiple($cids);
   +      if ($node->get($field_name)->status && $comments = comment_get_thread($node, $field_name, $mode, $comments_per_page)) {
   

   Let's do not use the wrapper once have method on storage controller

7. +++ b/core/modules/comment/comment.module
   @@ -1212,7 +1004,7 @@ function comment_user_cancel($edit, $account, $method) {
   -  $cids = db_query('SELECT c.cid FROM {comment} c WHERE uid = :uid', array(':uid' => $account->id()))->fetchCol();
   +  $cids = \Drupal::entityManager()->getStorageController('comment')->loadUsersComments($account);
   

   is there other usage of the method?

8. +++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.php
   @@ -154,4 +154,335 @@ public function getChildCids(array $comments) {
   +  public function loadRecent($number = 10) {
   ...
   +    $cids = $query
   +      ->orderBy('c.created', 'DESC')
   +      // Additionally order by cid to ensure that comments with the same timestamp
   +      // are returned in the exact order posted.
   +      ->orderBy('c.cid', 'DESC')
   

   Related #1059608: Fix comment_get_recent ordering

Discussed in IRC - msonnabaum> all database access should go in the storage class

https://gist.github.com/msonnabaum/b88c73dbd5a910ee1db3

Re-rolling... not sure anything need to be updated for #31.

#31 seems to only list related issues, etc. I think this should be ready for review. Please correct me if I'm wrong.

This doesn't seem too touch comment_admin_overview this may be intentional as I am assuming there is a routing patch out there but at the moment it has

$query = db_select('comment', 'c')
    ->extend('Drupal\Core\Database\Query\PagerSelectExtender')
    ->extend('Drupal\Core\Database\Query\TableSortExtender');

This doesn't seem too touch comment_admin_overview this may be intentional as I am assuming there is a routing patch out there but at the moment it has

$query = db_select('comment', 'c')
    ->extend('Drupal\Core\Database\Query\PagerSelectExtender')
    ->extend('Drupal\Core\Database\Query\TableSortExtender');

Yes... I left it like this intentionally. Mainly since I assumed it will be replaced by a view. I can definitely add it if needed.

The fallback should be still there, related conversion is #1978904: Convert comment_admin() to a Controller

Conversion included. It will conflict once #1978904: Convert comment_admin() to a Controller is converted, but should be pretty easy to re-roll.

40: 2068331_40.patch queued for re-testing.

Re-rolled the patch. Uploading the updated patch.

@piyuesh23 The tests failed.

PHP Fatal error:  Call to undefined method Drupal\\field\\Entity\\FieldInstance::getFieldSetting() in /core/modules/comment/lib/Drupal/comment/CommentStorageController.php on line 402

This error only occurs after I patched Drupal to with your changes.

I don't know but I think your calls to getFieldSetting() should be getSetting().
PS: It was not the getFieldSetting() code because when I changed that it just continued, but with a lot of errors.

This jumped out at me.

1. +++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.php
   @@ -150,4 +150,355 @@ public function getChildCids(array $comments) {
   +  public function loadCommetingUsers(EntityInterface $node, $entity_type = 'node') {
   

   This should be loadCommentingUsers, right?

45: 2068331_44.patch queued for re-testing.

Here's a re roll taking into account #46 & #48 plus several other changes to make the patch apply

Some minor changes to fix (most?) of the tests.

Thanks for the patch @pcambra
This patch fails
error: patch failed: core/modules/comment/comment.module:408
Use \Drupal\comment\Controller\CommentController::adminPage() instead of comment_admin()
Use buildForm to construct the form instead old style.

Thanks for noticing about the admin.inc file @sidharthap, I definitely think that's something coming from some merge conflict as the comment admin forms are in \Drupal\comment\Form\CommentAdminOverview. now.

Fixed some historical merge problems and removed that file, here's a new patch.

This one should be better.

1. +++ b/core/modules/comment/comment.module
   @@ -214,9 +214,7 @@ function comment_menu_links_defaults_alter(&$links) {
   +  $count = \Drupal::entityQuery('comment')->condition('status', CommentInterface::NOT_PUBLISHED)->count()->execute();
   
   @@ -309,6 +307,20 @@ function comment_permission() {
   +  return \Drupal::entityManager()->getStorageController('comment')->loadRecent($number);
   
   @@ -339,46 +351,8 @@ function comment_new_page_count($num_comments, $new_replies, EntityInterface $en
   +    $pageno = \Drupal::entityManager()->getStorageController('comment')->getThreadedNewCommentPage($entity, $new_replies, $comments_per_page, $field_name);
   
   @@ -879,11 +827,7 @@ function comment_entity_load($entities, $entity_type) {
   +  $result = \Drupal::entityManager()->getStorageController('comment')->getEntityCommentStatistics(array_keys($entities), $entity_type);
   

   Can we make these multi line wherever possible please? Readability++

2. +++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.php
   @@ -158,4 +158,307 @@ public function getChildCids(array $comments) {
   +      // @todo This should be actually filtering on the desired node status field
   ...
   +      // Additionally order by cid to ensure that comments with the same timestamp
   ...
   +        // @todo Use $entity->getAuthorId() after https://drupal.org/node/2078387
   ...
   +    // Count how many comments (c1) are before $cid (c2) in display order. This is
   ...
   +      // use the sorting code for comparison, but must remove the trailing slash.
   

   more then 80 chars.

Thanks @Jibran for the review

Here's a new patch with the formatting changes in #60 applied

I checked the patch and realized that it uses comment_get_thread() in comment_node_update_index() function. This function will be depricated according to the following issue: #2156089: Remove comment_get_thread() in favour of method on CommentStorage

Should we make it posponed until the mentioned issue is fixed?

Referring in part to #2156089-2: Remove comment_get_thread() in favour of method on CommentStorage: It looks to me like this method should be moved into CommentStorageController anyway, since there is no way this could be done in a database generic way.

In other words, the patch in this issue could solve #2156089 in one go.

I'm going to work a bit on this patch regardless, because I found some strange things (and pcambra is probably way busier than me, D8 Core beginner :) )

No need to look at this patch now.

Rerolled for #2101183: Move {comment_entity_statistics} to proper service and the renaming of storage controller interfaces. Also removed comment_get_recent() and theme_comment_block() which snuck back in by accident during a previous reroll.

Also removed some of the storage interface's methods which the previous patch introduced, and replaced them by EntityQuery where that made sense to me. I'm not done with that yet, though. To finish up, I'll have to sit down and think about what to do with some code in tracker.module. Probably move its remaining SQL queries into a service first -- much like #2101183 did for comment statistics.

So, more coming up in a next patch (and I'll then provide an interdiff from #61). But not for a few days.

*cough* sorry, no. This weekend looks promising though. I'll update here by saturday.

Status update! Wut, it's not saturday (19th) already, is it? :p

(Excuse of the week: got sucked deeper with every step, into a swamp of fixing contrib modules for a live website. I should never do urgent ongoing Core work; stuff like this will keep happening.)

If urgent: tell me, it helps. If not: I will try to parse #2068333: Convert node SQL queries to the Entity Query API (back to CNW again) today and this issue tomorrow.

OK. Pick your interdiff at will. Remarks and questions in semi random order:

1. The change in tracker.pages.inc is backed out / broken out into a separate followup patch, since the '/tracker' page breaks when using the comment.statistics service. I'll be reopening #2101183: Move {comment_entity_statistics} to proper service and we can apply the followup patch when that is fixed.
2. I'm getting lost on nailing down method signatures (arguments and return values) in this interface - specifically, on any 'rules' for when we want to force entities vs. IDs.
   • CommentStorageInterface::getCommentingUserIds() now wants an Entity passed. An entity_id + type would be less overhead. Then again, the place where it's called from (tracker_cron()) isn't used that much; see comments.
   • CommentStorageInterface::getCommentingUserIds() still returns (user) IDs instead of objects though, for performance reasons. (By the way, the existing & unchanged CommentStorageInterface::getChildCids() does too.)
   • CommentStorageInterface::getDisplayOrdinal() I kept at wanting an ID as an argument because I don't know how strongly we feel about making that into a Comment. If we do, I'd have to make a replacement for comment_get_display_page() too, but move that method into CommentManager(??) instead of CommentStorage - feel free to tell me to do that.
   • CommentStorageInterface::LoadThread() returns Comments now, which just seems logical (and patch #61 already kind of assumed that it would start doing so.)
3. I can't fully parse the discussion at #1938062-80: Convert the recent_comments block to a view and #2158871: [Policy, no patch] Clearly denote when @deprecated code is slated to be removed. I think the conclusion is that we want to keep functions in comment.module as much as possible, but mark them @deprecated Deprecated since Drupal 8.x-dev, to be removed in Drupal 8.0. So that's what I did for
   • comment_count_unpublished() (IMHO does not have wide enough use for a function call; I replaced the call in UnapprovedComments::getTitle() with the code itself.)?
   • comment_get_display_ordinal() (IMHO does not have wide enough use for a function call; related: question above about changing the call signature of CommentStorage::getDisplayOrdinal())
   • comment_get_thread() (Inefficient in new regime: returns IDs, discards the comment objects from CommentStorage::loadThread())
4. ...there's more functions that could be moved into CommentManager(?) but that's outside the scope of this issue. (comment_num_new(), comment_new_page_count(), comment_prepare_thread(), comment_get_display_page())

Oh, meh.

Oh. During code reshuffle, I deleted the existing CommentStatisticsInterface::updateEntityStatistics() but didn't delete the class method.

It may strictly not belong in this patch... but let's just delete this method here? There is only 'update' in here, not 'create', 'read' or 'delete'...

See above interdiffs and the comments in #69.

Change in #74 was incomplete: we can/should get rid of the whole dependency to the comment.statistics service + overridden createInstance()...

Unit test was failing because it missed the comment.statistics service. Should be fixed now. Also inserted/deleted some random comments.

You know what - this is just way too big.

I found a second already-existing issue (with an existing patch) so I am going to wait until some split-off parts are committed. Also, note I should just open separate issues for

• asking the getDisplayOrdinal() questions (because it will probably affect CommentManager too, not only CommentStorage - so it's part of #2096923: [meta] Clean-up CommentManager service and deprecate procedural helper functions)
• CommentStatistics related changes including the postponed tracker.pages.inc change (because really that's a followup to #2101183: Move {comment_entity_statistics} to proper service / #2259209: Fix CommentStatistics::read() now)

.

Can this be unpostponed now?

You can unpostpone if you want to review this whole thing in one go, but I figured it would be easier to do it in parts.

Getting each of the 3 "Related issues" committed will decrease the size and complication of this patch. I will go reroll those now. If you have some junior-contributors-like-me walking around, maybe they will want to review those?

I'm working / in Europe, but will be mostly available to reroll (and shrink) this one when I see activity in the other issues.

If, on the other hand, you want me to reroll this big one because you think getting it reviewed/committed in one go is the better way to approach this, please tell me.

Agree. Fixing this in parts seems easier. Let's fix related issues ASAP and then finish last part in here.

Nice.

Some summary (I'm off till tomorrow, after this):
#2156089: Remove comment_get_thread() in favour of method on CommentStorage I rerolled, needs review

#2262407: Deprecate comment_get_display_page/ordinal() you rerolled - waiting for comments I guess? I can RTBC your patch but not my own logic without someone saying they looked at my patch in detail?

#2097123: Deprecate comment_num_new() in favour of method on CommentManager you rerolled - myeah, I didn't really work on this, only tried to do a reroll which was imperfect, don't know the code yet. Will review when green, I guess.

(About the last fail: while rerolling something else, my eye spotted a FieldInfo class that was replaced by an EntityManager class recently? I don't know what it did, just making an offhand uninformed remark :)...)

Just opened #2280861: CommentStatistics service followup because I had been planning to do that. Should be review-able.

Do we have a sub-issue for comment_new_page_count()?

I couldn't find one #2290155: Deprecate comment_new_page_count in favour of commentManager/commentStorage created, I have assigned to myself and will look at in the next day or so but feel free to grab.

#2281475: Deprecate comment_new_page_count() and move it functionality into Comment storage controller is for comment_new_page_count, RTBC, mentioned in left sidebar.

@larowlan suppose once we move direct queries to entity queries we should separate storage dependent methods to CommentStorage and independent to CommentManager

(if people are checking this issue now, with 2 sprints going on: I will check this stuff and weed out all committed code, maybe leading to closing the issue, before saturday.)

Looks all child issues are closed

...which leads to the following leftovers from the previous patches:

• two tests which need their SQL statements converted
• one random comment (type hint) change in CommentStorageInterface
• deleting {comment} related queries from tracker.module (I rewrote most of it, now I know what I'm doing), and adding another method to CommentStorageInterface for that.

Note there is more to rewrite about tracker module, but I stopped once the {comment} sql was gone. tracker.page.inc conversion is for #2280861: CommentStatistics service followup, and the one {node_field_data} query remaining in tracker_cron(). is for... the future TrackerRepository service (no ticket opened yet), imho.

Doing Node::load() in one place and not the other, is weird.

Wrong 'reroll'.

(Also, now formatting the SQL just like a CommentStorage::getNewCommentPageNumber() did above.)

Oh by the way! This patch 'depends' on #2280861: CommentStatistics service followup at the moment!

The 3rd argument from

$latest_comment = \Drupal::service('comment.statistics')->read(array($node), 'node', FALSE);

...has no meaning at the moment; it's converting the 'target = replica' option that will be honored if #2280861 gets accepted.

dum da dum see #95 & #100

Yes, we need to wait for #2280861: CommentStatistics service followup
The patch looks good but I disagree to add new method to storage that will require a test

1. +++ b/core/modules/comment/src/CommentStorage.php
   @@ -221,6 +221,18 @@ public function getChildCids(array $comments) {
   +  public function getCommentingUserIds(EntityInterface $entity) {
   +    return $this->database->query('SELECT DISTINCT c.uid FROM {comment_field_data} c WHERE entity_id = :entity_id
   +                                    AND entity_type=:entity_type
   +                                    AND default_langcode = 1', array(
   +      ':entity_id' => $entity->id(),
   +      ':entity_type' => $entity->getEntityTypeId()
   +    ))->fetchCol();
   
   +++ b/core/modules/comment/src/CommentStorageInterface.php
   @@ -86,6 +86,17 @@ public function getDisplayOrdinal(CommentInterface $comment, $comment_mode, $div
   +   * Gets the ids of users that commented on a given parent entity.
   ...
   +  public function getCommentingUserIds(EntityInterface $entity);
   
   +++ b/core/modules/tracker/tracker.module
   @@ -81,25 +84,24 @@ function tracker_cron() {
   -      $query = db_select('comment_field_data', 'c', array('target' => 'replica'));
   -      // Force PostgreSQL to do an implicit cast by adding 0.
   -      $query->addExpression('0 + :changed', 'changed', array(':changed' => $changed));
   -      $query->addField('c', 'status', 'published');
   -      $query->addField('c', 'entity_id', 'nid');
   -      $query
   -        ->distinct()
   -        ->fields('c', array('uid'))
   -        ->condition('c.entity_id', $row->nid)
   -        ->condition('c.entity_type', 'node')
   -        ->condition('c.uid', $row->uid, '<>')
   -        ->condition('c.status', CommentInterface::PUBLISHED)
   -        ->condition('c.default_langcode', 1);
   +      $uids = \Drupal::entityManager()->getStorage('comment')->getCommentingUserIds($node);
   

   This is not strait conversion, also I see no reason in this method in storage because it has only one usage.
   And the tracker should be replaced with views! #1941830: Convert tracker listings to a view

2. +++ b/core/modules/comment/src/Tests/CommentLanguageTest.php
   @@ -112,17 +112,15 @@ function testCommentLanguage() {
            // Check that comment language matches the current content language.
   -        $cid = db_select('comment_field_data', 'c')
   -          ->fields('c', array('cid'))
   -          ->condition('entity_id', $node->id())
   +        $cids = \Drupal::entityQuery('comment')
   +          ->condition('entity_id', (int) $node->id())
   ...
              ->condition('default_langcode', 1)
   

   Not sure that approach right, but default_langcode makes no sense for EQ.
   Let's get @plach here to mention!
   Seems the test a bit outdated, but that out of scope

This is not strait conversion, also I see no reason in this method in storage because it has only one usage.

Is there any better way to convert? One of the reasons why we're converting is easy storage swapability. In order to achive that we need all DB-specific code in one place (== storage).

The better way is just simply replace db_select with the equivalent EQ, but as I pointed - queries are totally different

For the sake of simplicity; for now; we should move that query from tracker into comment 1:1 and leave a // @todo remove when https://www.drupal.org/node/1941830 is in.

In this interdiff:

• comment I'd forgotten
• since we're loading nodes in tracker_cron() already, we can do better in using those
• ...and I realised I shouldn't be calling Node::load() directly since that's hard coupling, right?
• bug in conversion: I forgot an if ($node->isPublished()) {

This is not strait conversion

I kind-of disagree, though it's not apparent at first sight.

The old SQL query gets all user IDs from comments (except the node->uid), and then inserts a number of rows into {tracker_user}: each user ID & 'static' values for all other columns.
INSERT INTO {tracker_user} SELECT uid, [static values] FROM {comment_field_data} WHERE [conditions]

The converted SQL query still does the same, but...

The intention of what we are changing now (if I am correct) is to remove the explicit dependency on {comment_field_data}, so that's what is done by packaging the {comment_field_data} query into a CommentStorage method.
This implies that the new query must be converted to a INSERT INTO {tracker_user} VALUES (uid1, [static values]), (uid2, [static values]), .... And that the [conditions] are moved outside the query (into code deciding which uids to insert).

IMHO it's as straight a conversion as I can make it, given the assumption we need to get rid of {comment_field_data}.

also I see no reason in this method in storage because it has only one usage.

I bounced back and forth on this. Is there another way in which we can get 'all users in a thread of comments'? This is a pretty generic need (theoretically, though nothing else uses it). I can use loadThread() because we don't really care about efficiency anyway... but then I'm a bit scared of the mandatory 'comment_filter' tag in that query.

...all of the above goes away if we make all this stuff into a TrackerRepository service and we don't care about that service using {comment_field_data} directly. But I think we do care - decoupling of thingies?

And the tracker should be replaced with views!

Cool. We're lucky; #1941830: Convert tracker listings to a view does not coincide with changes here. (Only with #2280861: CommentStatistics service followup.)

@chx: That implies "no can do" on #106. If you want to, though, I'm fine with an explicit decision to not convert SQL from tracker.module yet, and split the tracker.module part (and above discussion) into a new issue so we can call comment.module converted at least.

(It's gonna be added to #2068325: [META] Convert entity SQL queries to the Entity Query API though ;) )

I was typing while you were discussing :)

My point in a shorter reply:
@andypost

The better way is just simply replace db_select with the equivalent EQ,

But it's not a 'SELECT' query. It's an 'INSERT INTO {tracker_user} SELECT ...' query. How to convert that?

• fix test failure
• forgot #103.2. @andypost you're right, default_langcode doesn't make sense. Other than that I don't think anything's wrong with the query.

still fixing silly minor things in EQ

Overall great!

1. +++ b/core/modules/comment/src/Tests/CommentLanguageTest.php
   @@ -112,17 +112,14 @@ function testCommentLanguage() {
   +          ->condition('entity_id', (int) $node->id())
   

   tricky, comments can not work with non-int IDs, but not sure that's needed here. Probably for postgres only, and sometimes entities returns ID as string ("123")

2. +++ b/core/modules/tracker/tracker.module
   @@ -74,35 +79,35 @@ function tracker_cron() {
   -      $query = db_select('comment_field_data', 'c', array('target' => 'replica'));
   
   @@ -269,23 +274,18 @@ function _tracker_add($nid, $uid, $changed) {
   -  ), array('target' => 'replica'))->fetchObject();
   ...
   +  $latest_comment = \Drupal::service('comment.statistics')->read(array($node), 'node', FALSE);
   

   should wait for #2280861: CommentStatistics service followup

3. +++ b/core/modules/tracker/tracker.module
   @@ -74,35 +79,35 @@ function tracker_cron() {
   -      // Force PostgreSQL to do an implicit cast by adding 0.
   -      $query->addExpression('0 + :changed', 'changed', array(':changed' => $changed));
   

   previously I refer to this place, that is not properly converted, but now I see that "changed" is not used and calculated latter

4. +++ b/core/modules/tracker/tracker.module
   @@ -301,23 +301,24 @@ function _tracker_calculate_changed($nid) {
   -  // @todo This should be actually filtering on the desired language and just
   -  //   fall back to the default language.
   -  $node = db_query('SELECT nid, status, uid, changed FROM {node_field_data} WHERE nid = :nid AND default_langcode = 1 ORDER BY changed DESC, status DESC', array(':nid' => $nid))->fetchObject();
   +  $node = \Drupal::entityManager()->getStorage('node')->load($nid);
   

   Node::load($nid) but I'd preserve comment, there's issue somewhere in d8mi about negotiation...

114.1
I really didn't know what to do there :) don't know the implementation of this in the lower DB layers. I got it from comment_entity_predelete().

But now I see other places that don't do (int). So I will delete this. (If there's ever a bug, we will see it.)

114.4.1
Please teach me (I don't know where to find out except typing a novel on IRC):
I like Node::load($nid) better too, but doesn't it create a hard dependency on \Drupal\node\Node while we want this class to be swappable? Or don't we care about swappability in the case of 'base entity classes' or is there something I am not seeing?

114.4.2
The comment (generic-clone comment inserted ~30 times for every {node_field_data} occurrence during working on #1498674) makes no sense to me when {node_field_data} disappears. I inserted a function-level comment, does it make sense?

Oh and re. #103, I need guidance on writing tests for that CommentStorage::getCommentingUserIds();

Not the code specifics, but the overall picture. (Maybe guidance can be simply "put it in X.php".) Because I don't see yet if

• we're aiming for unit tests (there are no unit tests for the other methods, right?)
• we're aiming for functional tests (where to put those? Can I just stick it in an existing test whereever I see a logical scenario to piggyback on - and make up a new test if not?)

Re 114.4.1 oh never mind -- it's a static function, and I should have checked its definition. Still having a hard time keeping up to date with all these newfangled modern OO inventions ;-)

Only #115 outstanding (if you agree with the comment in #114)

Fixing patch apply, lets see how many test fails.

Changing status to need review.

chx suggested using QueryAggregate instead of the new interface function, which works wonders.

(I had to think for a minute, whether an SQL 'group by' query without an aggregate field would be legal everywhere. But indeed, why wouldn't it?)

The $do_insert in the interdiff is fixing a bug that had been in this patch forever.

Only one thing left imo...

+++ b/core/modules/comment/src/Tests/CommentLanguageTest.php
@@ -112,17 +112,14 @@ function testCommentLanguage() {
           ->range(0, 1)
...
+        $comment = Comment::load(reset($cids));

reset() is not needed here because of range()

Really? Return will still be an array so it is either loadMultiple() or reset().

I concur. reset is simpler.

Overall this looks great, one question though...

+++ b/core/modules/tracker/tracker.module
@@ -74,35 +80,47 @@ function tracker_cron() {
-      $query = db_select('comment_field_data', 'c', array('target' => 'replica'));
-      // Force PostgreSQL to do an implicit cast by adding 0.
-      $query->addExpression('0 + :changed', 'changed', array(':changed' => $changed));
-      $query->addField('c', 'status', 'published');
-      $query->addField('c', 'entity_id', 'nid');
-      $query
-        ->distinct()
-        ->fields('c', array('uid'))
-        ->condition('c.entity_id', $row->nid)
-        ->condition('c.entity_type', 'node')
-        ->condition('c.uid', $row->uid, '<>')
-        ->condition('c.status', CommentInterface::PUBLISHED)
-        ->condition('c.default_langcode', 1);
-
-      // Insert the user-level data for the commenters (except if a commenter
-      // is the node's author).
-      db_insert('tracker_user')
-        ->from($query)
-        ->execute();
+      if ($node->isPublished()) {
+        // Insert the user-level data for the commenters (except if a commenter
+        // is the node's author).
...
+      }

Why did this code get indented into a if ($node->isPublished()) block? That didn't seem to be there before.

Well that was a bit silly. Now that the need for a 'generalized' method has been replaced with a QueryAggregate, we can actually do a straight conversion of the existing query.

(Not to mention that the if ($node->isPublished()) block was a mistake to begin with...)

If #127 didn't fail despite #131 then we need more tests.

Absolutely. But since

• that forum_cron() code is just waiting to be refactored into a service
• this issue itself is about straight-converting SQL queries without changing functionality
• a test would add SQL queries (to tracker)

...could we put the test into a followup which tackles the rest of the tracker stuff/tests?
Created #2330577: Introduce TrackerRepository service.

I'm fine with follow-up, because tests are out of scope for conversion

Okay let's have a followup to add the missing test coverage.

Committed 8d594b5 and pushed to 8.0.x. Thanks!