Updated: Comment #0

Problem/Motivation

#731724: Convert comment settings into a field to make them work with CMI and non-node entities Will add a new CommentManager service. comment_num_new() belongs on that service.

Proposed resolution

Move to a method and deprecate the original function.

Remaining tasks

Wait for #731724: Convert comment settings into a field to make them work with CMI and non-node entities
Patch

User interface changes

None

API changes

comment_num_new() is deprecated.

Follow-up from #731724: Convert comment settings into a field to make them work with CMI and non-node entities.
#2090139: Tests for: Warning: Illegal string offset 'callable' in rdf_rdfa_attributes() (line 181 of core/modules/rdf/rdf.module).

Files: 
CommentFileSizeAuthor
#55 2097123-newComments-55.patch16.45 KBmgifford
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
#53 2097123-newComments-53.patch16.43 KBmgifford
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
#48 2097123-newComments-48.patch17.9 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 63,081 pass(es).
[ View ]
#48 interdiff.txt7.79 KBandypost
#44 2097123-newComments-44.patch16.17 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,366 pass(es).
[ View ]

Comments

Priority:Critical» Major
Status:Postponed» Active

StatusFileSize
new8.1 KB
FAILED: [[SimpleTest]]: [MySQL] 59,237 pass(es), 5 fail(s), and 86 exception(s).
[ View ]

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.

Status:Active» Needs review

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

Status:Needs review» Needs work

The last submitted patch, d8-deprecate-comment_num_new-2097123-2.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.67 KB
FAILED: [[SimpleTest]]: [MySQL] 59,436 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new5.77 KB

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.

Status:Needs review» Needs work

The last submitted patch, d8-deprecate-comment_num_new-2097123-5.patch, failed testing.

@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

The last submitted patch, d8-deprecate-comment_num_new-2097123-5.patch, failed testing.

Title:Deprecate comment_num_new() in favour of method on CommentManagerUse \Drupal instead of Drupal to make IDEs and static code analyse tools happy
Component:comment.module» other
Category:task» bug
Priority:Major» Normal

Title:Use \Drupal instead of Drupal to make IDEs and static code analyse tools happyDeprecate comment_num_new() in favour of method on CommentManager
Component:other» comment.module
Category:bug» task
Priority:Normal» Major

Status:Needs work» Needs review
StatusFileSize
new9.45 KB
FAILED: [[SimpleTest]]: [MySQL] 59,331 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new3.04 KB

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

  1. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -241,4 +241,64 @@ public function getFieldUIPageTitle($field_name) {
    +   * @param int $entity_id

    I think this should take an array of entity ids, like we did for history_read_multiple(). There are several instances where we're calling this method in a foreach loop which could be more performant in a single query.

  2. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -241,4 +241,64 @@ public function getFieldUIPageTitle($field_name) {
    +    $user = \Drupal::currentUser();

    This should be injected

  3. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -241,4 +241,64 @@ public function getFieldUIPageTitle($field_name) {
    +    if ($user->isAuthenticated() && \Drupal::moduleHandler()->moduleExists('history')) {

    So should module handler

  4. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -241,4 +241,64 @@ public function getFieldUIPageTitle($field_name) {
    +          $timestamp = history_read($entity_id);

    Do we have a history service yet? If not we should put history_read behind a method so we have ability to unit-test

  5. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -241,4 +241,64 @@ public function getFieldUIPageTitle($field_name) {
    +      $query = db_select('comment', 'c');

    We should inject the database connection

  6. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentFieldsTest.php
    @@ -69,7 +69,7 @@ function testCommentInstallAfterContentModule() {
    +    if ($field = $this->container->get('field.info')->getField('node', 'comment_forum')) {
    +++ b/core/modules/forum/forum.module
    @@ -791,7 +791,7 @@ function template_preprocess_forum_topic_list(&$variables) {
    +        $variables['topics'][$id]->new_url = url('node/' . $topic->id(), array('query' => comment_new_page_count($topic->comment_count, $topic->new_replies, $topic, 'comment_forum'), 'fragment' => 'new'));

    Good catch. So far this is the only issue from the 450kb comment-field patch. I can live with that.

  1. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -241,4 +241,64 @@ public function getFieldUIPageTitle($field_name) {
    +   * Returns the number of new comments for an user and the specified entity.

    'for the current user' might push it past 80 chars, so 'for a user'

  2. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -241,4 +241,64 @@ public function getFieldUIPageTitle($field_name) {
    +   * @todo Help, this uses procedural code! We've to get rid of that.

    Remove this line

Status:Needs review» Needs work

The last submitted patch, drupal8.comment-module.2097123-12.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new13.4 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new6.57 KB

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.

Status:Needs review» Needs work

The last submitted patch, drupal8.comment-module.2097123-16.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new14.21 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

let's debug bot

Status:Needs review» Needs work

The last submitted patch, drupal8.comment-module.2097123-17-test.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new13.96 KB
FAILED: [[SimpleTest]]: [MySQL] 59,192 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new3.42 KB

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

Status:Needs review» Needs work

The last submitted patch, drupal8.comment-module.2097123-20.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new15.48 KB
FAILED: [[SimpleTest]]: [MySQL] 58,267 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.33 KB

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

Status:Needs review» Needs work

The last submitted patch, drupal8.comment-module.2097123-22.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new16.12 KB
PASSED: [[SimpleTest]]: [MySQL] 58,674 pass(es).
[ View ]
new1.82 KB

Disregard the comment regarding fetching in a single query (aka taking array of ids), this can't be done because the $timestamp is different in each case.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.php
@@ -330,7 +330,7 @@ public function renderNewCommentsNodeLinks(Request $request) {
+      $new = \Drupal::service('comment.manager')->countNewComments($node->id(), 'node');

Can this be injected?

Other than that looks good to go

StatusFileSize
new16.66 KB
FAILED: [[SimpleTest]]: [MySQL] 58,317 pass(es), 22 fail(s), and 9 exception(s).
[ View ]
new5.78 KB

StatusFileSize
new16.66 KB
PASSED: [[SimpleTest]]: [MySQL] 58,341 pass(es).
[ View ]
new636 bytes

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.

StatusFileSize
new15.72 KB
PASSED: [[SimpleTest]]: [MySQL] 58,765 pass(es).
[ View ]
new4.87 KB

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?

StatusFileSize
new15.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.comment-module.2097123-31.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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:

StatusFileSize
new4.35 KB

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.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes
Issue tags:-beta blocker

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

Status:Needs review» Needs work

The last submitted patch, 31: drupal8.comment-module.2097123-31.patch, failed 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

Status:Needs work» Needs review
StatusFileSize
new16.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2097123-newComments-39.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll with EntityQuery and @todo reference for #2081585: Introduce HistoryRepository service (no reason to inject module handler in favor of history manager

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

Status:Needs review» Needs work

The last submitted patch, 39: 2097123-newComments-39.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new16.13 KB
PASSED: [[SimpleTest]]: [MySQL] 59,389 pass(es).
[ View ]

re-roll

+++ b/core/modules/forum/tests/Drupal/forum/Tests/ForumManagerTest.php
@@ -9,6 +9,11 @@
+// @todo Remove once the constants are replaced with constants on classes.

Can we get a url added for this todo?

Other than that, RTBC

StatusFileSize
new549 bytes
new16.17 KB
PASSED: [[SimpleTest]]: [MySQL] 59,366 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Ready, thanks!

Priority:Major» Normal

Agreed that this is not major.

Status:Reviewed & tested by the community» Needs review

+++ 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?

StatusFileSize
new7.79 KB
new17.9 KB
PASSED: [[SimpleTest]]: [MySQL] 63,081 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

Great cleanup to see the wrapper gone from ForumManager, getting there slowly :)

Status:Reviewed & tested by the community» Needs work
  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?

Seems this should be postponed on #2081585: Introduce HistoryRepository service

Status:Needs work» Needs review
StatusFileSize
new16.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 53: 2097123-newComments-53.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new16.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

hmm...

Status:Needs review» Needs work

The last submitted patch, 55: 2097123-newComments-55.patch, failed testing.

Issue tags:+Needs reroll

Issue tags:+TUNIS_2014_MARCH

Assigned:Unassigned» ahmed25

Assigned:ahmed25» Unassigned
Issue tags:-Needs reroll

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