Deprecate comment_num_new() in favour of method on CommentManager

First attempt.
There are still calls to procedural code in the method, is it in the scope of this issue to change this as well?
And I don't know if $user as parameter is the right approach.
So the whole thing doesn't feel sound yet.

Let's test the patch

1. +++ b/core/modules/comment/comment.module
   @@ -1250,66 +1250,6 @@ function comment_load($cid, $reset = FALSE) {
   -function comment_num_new($entity_id, $entity_type, $field_name = NULL, $timestamp = 0) {
   
   +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
   @@ -241,4 +242,65 @@ public function getFieldUIPageTitle($field_name) {
   +  public function countNewComments(UserInterface $user, $entity_id, $entity_type, $field_name = NULL, $timestamp = 0) {
   

   Please @deprecated

2. +++ b/core/modules/comment/comment.module
   @@ -1250,66 +1250,6 @@ function comment_load($cid, $reset = FALSE) {
   -function comment_num_new($entity_id, $entity_type, $field_name = NULL, $timestamp = 0) {
   
   +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
   @@ -241,4 +242,65 @@ public function getFieldUIPageTitle($field_name) {
   +   * @param \Drupal\user\UserInterface $user
   +   *   The user for which to count the new comments.
   ...
   +  public function countNewComments(UserInterface $user, $entity_id, $entity_type, $field_name = NULL, $timestamp = 0) {
   

   $user makes no sense as first argument

Addressed feedback in #3.

• Restored legacy function and added @deprecated
• Removed $user parameter, really doesn't make sense as long as we don't have a way to pass it on.

Btw. I don't get how ForumManager::numberNew() ever worked, it looks like this method called comment_num_new() always with wrong parameters.

@das-peter Awesome finding about forum!
Not sure why tokens fail, will check tonight

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
@@ -312,7 +312,7 @@ protected function getTopicOrder($sortby) {
   protected function numberNew($nid, $timestamp) {
-    return comment_num_new($nid, $timestamp);
+    return \Drupal::service('comment.manager')->countNewComments($nid, 'node', NULL, $timestamp);

yep, seems we have no test for that.
Also forum adds own comment field so it makes sense to use the name here

#5: d8-deprecate-comment_num_new-2097123-5.patch queued for re-testing.

I can't reproduce failures locally...

Also filed related
#2101155: Add CommentManagerInterface
#2101183: Move {comment_entity_statistics} to proper service

Found 2 occurrences of broken forum field usage, now tests should be fine

Patch fixes injections, multiple IDs needs a bit more work,

Also found Warning: call_user_func() expects parameter 1 to be a valid callback, function 'd' not found or invalid function name in rdf_rdfa_attributes() (line 183 of core/modules/rdf/rdf.module). but that's not patch related, thrown on forum page

+ $user = \Drupal::currentUser();
This should be injected

Done, but still not sure about injection of common services.

Do we have a history service yet?

No, that's for @todo Remove once http://drupal.org/node/1029708 lands.

let's debug bot

Current user is request scope limited so we can't pass it to services

Proper fix for tests:
1) ForumManagerTest test needs to mock comment manager
2) CommentTokenReplaceTest should check new comments for user that never read comments

We need to fix simpletest #2102539: Current user service should be updated in drupalLogin() and drupalLogout()

Let's name it right, also includes hunk from #2102539-2: Current user service should be updated in drupalLogin() and drupalLogout()

And last hunk

1. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
   @@ -241,4 +260,65 @@ public function getFieldUIPageTitle($field_name) {
   +  public function getCountNewComments($entity_id, $entity_type, $field_name = NULL, $timestamp = 0, AccountInterface $account = NULL) {
   +    if (!isset($account)) {
   +      $account = \Drupal::currentUser();
   +    }
   

   Is there a specific reason why we don't inject the currentUser?

2. +++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.php
   @@ -330,7 +330,7 @@ public function renderNewCommentsNodeLinks(Request $request) {
   -      $new = comment_num_new($node->id(), 'node');
   +      $new = $this->commentManager->getCountNewComments($node->id(), 'node');
   

   It would be maybe helpful to have swap the order of the arguments so we kind of have similar signature as with things like entity_load in D7.

Order of arguments makes a lot of sense!

Is there a specific reason why we don't inject the currentUser?

We pass $account as argument so calling code should pass it right, it makes no sense to make manager depends on service that will be limited with request scope #2102027: Add back the request scope to the current user service

We pass $account as argument so calling code should pass it right, it makes no sense to make manager depends on service that will be limited with request scope #2102027: Add back the request scope to the current user service

Well, then we maybe should require the account variable?

I don't think requiring the account is a good idea DX-wise, since most of the time this is called it will be for the current account, and it would put additional burden on the developer to always have to retrieve the current user.

I couldn't make sense of the current user / request scope issue, but looking at the bootstrap, the request is initialized right after the DRUPAL_BOOTSTRAP_CODE phase, which means it is the very first thing happening after the system becomes usable. I can't imagine many cases where it would be necessary to retrieve a comment count earlier than this bootstrap phase :) Trying to do so will throw fatal errors anyway.

Did some work on this:

• Reroll was necessary because #2080577: Remove Unused local variable $instance from /core/modules/comment/lib/Drupal/comment/Tests/CommentTokenReplaceTest.php got in.
• Updated documentation.
• Some small cleanups.

Here's the interdiff.

Probably we should implement this method in storage controller or postpone on #2068331: Convert comment SQL queries to the Entity Query API

Can someone explain why this is a major beta blocker? Also, postponing on #2068331: Convert comment SQL queries to the Entity Query API sounds reasonable.

Updated issue summary.

Yes I don't think this is a beta blocker, not sure it should block release either since there's no API change necessary.

31: drupal8.comment-module.2097123-31.patch queued for re-testing.

+++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
@@ -199,4 +222,64 @@ public function getFieldUIPageTitle($field_name) {
+      $query = $this->connection->select('comment', 'c');
+      $query->addExpression('COUNT(cid)');

this query should be converted to EntityQuery

Re-roll with EntityQuery and @todo reference for #2081585: [META] Modernize the History module (no reason to inject module handler in favor of history manager

39: 2097123-newComments-39.patch queued for re-testing.

re-roll

- filed #2169361: Convert COMMENT_HIDDEN & COMMENT_CLOSED & COMMENT_OPEN to a constant on the comment field interface that needs discussion
- added link

Agreed that this is not major.

+++ b/core/modules/comment/lib/Drupal/comment/CommentManagerInterface.php
@@ -86,4 +88,24 @@ public function addBodyField($entity_type, $field_name);
+  public function getCountNewComments($entity_type, $entity_id, $field_name = NULL, $timestamp = 0, AccountInterface $account = NULL);

Sorry one more question here. Why not just pass the $entity object instead of type/id?

New patch changes the argument to entity, also removes wrapper in forum manager that was a simple wrapper function and not defined in ForumManagerInterface so no API change here.

Also @larowlan's feedback is welcome

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
@@ -314,7 +325,7 @@ protected function getTopicOrder($sortby) {
   protected function numberNew($nid, $timestamp) {
-    return comment_num_new($nid, $timestamp);
+    return $this->commentManager->getCountNewComments('node', $nid, 'comment_forum', $timestamp);

Because this will need to change forum manager.

1. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
   @@ -296,6 +307,48 @@ public function forbiddenMessage(EntityInterface $entity, $field_name) {
   +    if (\Drupal::currentUser()->isAuthenticated() && \Drupal::moduleHandler()->moduleExists('history')) {
   

   Let's replace \Drupal::currentUser() with $this->currentUser

2. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
   @@ -296,6 +307,48 @@ public function forbiddenMessage(EntityInterface $entity, $field_name) {
   +  public function getCountNewComments(EntityInterface $entity, $field_name = NULL, $timestamp = 0, AccountInterface $account = NULL) {
   

   What is the purpose of the $account parameter?

See #2180109: Change the current_user service to a proxy and #2182055: Comment module causes Circular reference error on a REST request, Comment manager seems to get the current user but it is problematic...

Seems this should be postponed on #2081585: [META] Modernize the History module

re-roll... I tried to address $this->currentUser from #50.

hmm...

I just checked this it doesn't need a reroll.

Fix #50, $account was supposed to be passed, but there's a current user service for.

Also removed leftover COMMENT_OPEN constant

And a bit more fixes

62: 2097123-newComments-62.patch queued for re-testing.

And one more place

1. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
   @@ -316,6 +327,48 @@ public function forbiddenMessage(EntityInterface $entity, $field_name) {
   +    if ($this->currentUser->isAuthenticated() && \Drupal::moduleHandler()->moduleExists('history')) {
   

   I guess we also want to inject the module handler.

2. +++ b/core/modules/forum/tests/Drupal/forum/Tests/ForumManagerTest.php
   @@ -74,12 +74,17 @@ public function testGetIndex() {
   +    $comment_manager = $this->getMockBuilder('\Drupal\comment\CommentManager')
   +      ->disableOriginalConstructor()
   +      ->getMock();
   +
   

   Can't we just mock the interface?

1) fixed, but module handler will not needed after #2081585: [META] Modernize the History module
2) fixed

Also I start to think- do new method needs access check for current user...

missed module handler

fixed interdiff

proper patch & interdiff

I have a feeling we have a system for dealing with @deprecated things.
Is it, commit the patch like this, and then do a follow-up issue to actually remove it?

Yeah, the system is at #2158871: [Policy, no patch] Clearly denote when @deprecated code is slated to be removed which includes a followup-meta. Reworded @deprecated to fit this.

Also: needed reroll for #2182055: Comment module causes Circular reference error on a REST request and #2079797: Provide a trait for $this->t() and $this->formatPlural().
Unfortunately I don't know what to do with the 'currentUser' class variable which has been removed. Do we pass the current account into getCountNewComments() now?

Attached part of the before-after reroll diff FYI. (Not included there: comment.module. in there, the call to getCountNewComments() was changed.)

Correct version of what I wanted to upload FYI. (I guess it's not customary to do before-after-reroll diffs, they're confusing.)

Hah, brain freeze. Forgot to check existing calls to getCountNewComments() before I changed its signature.

Rephrase:
I do not know how to reroll this properly; protected $currentUser wasn't removed for nothing. I placed the reference to \Drupal::currentUser() back in the rerolled patch, to make tests pass, but I'm pretty sure this is not wanted :/

(Maybe #2188287: Split CommentManager in two will make this easier? I don't know.)

Reroll after PSR-4.

Fixed few minor things.

More a comment than anything do we have an issue to fix up comment_new_page_count I am assuming yes!!

Here we go: #2281475: Deprecate comment_new_page_count() and move it functionality into Comment storage controller

Actually.... this is not OK. DB code should go into CommentStorage.

You're right.

Actually, no. This uses an EntityQuery, which is storage agnostic, so it's fine in CommentManager.

I'm not setting back RTBC, only because I don't understand what changed between comment #20 and now. #20 says you can't inject current_user into services.

(If this has indeed changed, just set back RTBC.)

Hm... you're right. current_user is properly injected as of #80.

Reroll.

This was a straight reroll.

Reroll. (Effectively: one replacement from a field_id expression to field_name.)

Committed and pushed to 8.x. Thanks!

test reverted patch

Fix broken test

Looks good

+1 for RTBC.

read through this and noticed some nits. just fixed them. should still be rtbc.

also made #2296839: Remove deprecated comment_num_new()

#100 added comment to the list of module dependencies. It had to do this because #2287223: Use KernelTestBase for config schema tests where possible changed BlockConfigSchemaTest to be a kernel test and kernel tests do not auto enable dependencies of modules.

Awesome! back to rtbc

I dont think this changes anything related to accessibility. that tag probably was inherited from the parent issue.

+++ b/core/modules/comment/src/CommentViewBuilder.php
@@ -327,7 +327,8 @@ protected function alterBuild(array &$build, EntityInterface $comment, EntityVie
+    $new = \Drupal::service('comment.manager')
+      ->getCountNewComments(entity_load($context['entity_type'], $context['entity_id']));

This should be injected no?

Nope - it's a static function - thanks @slashrsm

Committed 6e70062 and pushed to 8.x. Thanks!