Remove (untested, unused, broken) comment_get_recent()

I think it could be a method on a CommentManager service, honestly. But, if we continue down this path:

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/Block/RecentCommentsBlock.phpundefined
@@ -62,9 +99,27 @@ public function blockSubmit($form, &$form_state) {
+      '#entities' => entity_load_multiple('comment', $comments),

Inject the storage controller, please

Not fussy either way
On the manager would make it more useful to contrib/easier to reuse.
For forum I've tried to get all of the SQL queries (other than hooks) on the manager to make it easier for other backends to override in one place so that would fit here too.
But either way is better than what we have now.

If I wanted to get 10 comment object for the most recent, what does that have to do with a block?

BlockManager::getRecentComments(AccountInterface $account);

I think its easier to call a method than copy paste the contents of a class, but I'll probably never need this, I don't care.
But I still think it has nothing to do with blocks.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/Block/RecentCommentsBlock.phpundefined
@@ -62,9 +111,27 @@ public function blockSubmit($form, &$form_state) {
+    $query = $this->database->select('comment', 'c');
+    $query->innerJoin('node_field_data', 'n', 'n.nid = c.nid');
+    $query->addTag('node_access');
+    $query->addMetaData('base_table', 'comment');
+    $comments = $query
+      ->fields('c', array('cid'))
+      ->condition('c.status', COMMENT_PUBLISHED)
+      ->condition('n.status', NODE_PUBLISHED)
+      // @todo This should be actually filtering on the desired node status field
+      //   language and just fall back to the default language.
+      ->condition('n.default_langcode', 1)
+      // Additionally order by cid to ensure that comments returned in the exact
+      // order posted.
+      ->orderBy('c.cid', 'DESC')
+      ->range(0, $this->configuration['block_count'])
+      ->execute()
+      ->fetchAllKeyed(0, 0);

Is there a reason to not use EQ or just skip all that and just do the views bit?

I interpretate #1938062-82: Convert the recent_comments block to a view as a won't fix for this issue.

@dawehner, I don't think my comment has anything to do with this issue?

It makes sense to me to not remove that function in that issue, and discuss it specifically here instead, as to whether we think it's a worthwhile procedural helper or not. I agree that the function is generic and needn't have anything to do with the block.

My two cents is that this is not serious enough to merit a BC break after API freeze. Instead, I think we should:

• Stop using comment_get_recent() inside OO code. This dovetails nicely with #1938062: Convert the recent_comments block to a view, but neither issue is dependent on the other.
• Gut the current comment_get_recent() and replace its innards with an EFQ.
• Decide whether or not to mark comment_get_recent(), which is then merely a BC procedural wrapper, as @deprecated.

This needs to additionally oder by comment.updated and only if they are the same by comment.cid. Otherwise editing comments will not make them jump to the top of the list.

Shameless advertisement for a different issue: #1938062: Convert the recent_comments block to a view

1. +++ b/core/modules/comment/comment.module
   @@ -390,28 +390,11 @@ function comment_permission() {
    /**
   @@ -491,7 +474,7 @@ function theme_comment_block($variables) {
   
   @@ -491,7 +474,7 @@ function theme_comment_block($variables) {
      $items = array();
      $number = $variables['number'];
      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->changed->value))) . '</span>';
      }
    
   

   This feels kind of out of scope ? I would rather suggest to use the service directly

2. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
   @@ -0,0 +1,64 @@
   +    $query = $this->queryFactory->get('comment');
   ...
   +      ->addTag('node_access')
   

   Wait, we do add node_access on a comment entity query?

3. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
   @@ -0,0 +1,64 @@
   +    $ids =$query
   

   Missing space before $query

4. +++ b/core/modules/comment/lib/Drupal/comment/CommentManagerInterface.php
   @@ -0,0 +1,27 @@
   +   * @param integer $count
   

   ... this should be int not integer.

5. +++ b/core/modules/comment/lib/Drupal/comment/CommentManagerInterface.php
   @@ -0,0 +1,27 @@
   +   * @return array
   

   What about using \Drupal\Core\CommentInterface[] to typehint it?

+++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
@@ -0,0 +1,64 @@
+      ->addTag('node_access')

There is no reason to not add multiple tags. ... nevertheless I think it is wrong to add one, given that this is supposed to be an api function.

#20: drupal8.comment-module.2054993-20.patch queued for re-testing.

24: drupal8.comment-module.2054993-24.patch queued for re-testing.

Now that this function is completely unused after #1938062: Convert the recent_comments block to a view, do we really still need it? If we do, I think we should introduce some test coverage for it as well.

I have to agree, this function is really unrealistic to be used at all and not part of some fundamental API.

If I'm reading correctly, this should have been removed with #1938062: Convert the recent_comments block to a view but just got missed. If so I'm fine with just nuking it. Not committing right now though, but unassigning.

@catch
Sadly it was decided to not do it: #1938062-85: Convert the recent_comments block to a view

That sounds like API change approval from @catch.

For my part, +1, kill it. We just split this off to avoid blocking the conversion of the block on whether or not we could kill this. Conversion landed, so kill. :)

Oh. Accurate title.

Sorry for all the noise.

Committed and pushed to 8.x.