Problem/Motivation

#2097123: Deprecate comment_num_new() in favour of method on CommentManager deprecated comment_num_new()

Proposed resolution

Remove it when the time is right.

Remaining tasks

User interface changes

No.

API changes

Removal of comment_num_new()

Files: 
CommentFileSizeAuthor
#8 interdiff.txt1.49 KBMKorostoff
#6 drupal8-remove-comment-num-new-2296839-6.patch2.88 KBMKorostoff
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,255 pass(es). View
#4 Working As Expected.png140.89 KBMKorostoff
#3 drupal8-remove-comment-num-new-2296839-3.patch1.4 KBer.pushpinderrana
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,921 pass(es). View

Comments

YesCT’s picture

Status: Active » Postponed

technically postponed until the other is committed I guess.

marcingy’s picture

Status: Postponed » Active
Issue tags: +Novice

Other issue is in so unpostponing,.

er.pushpinderrana’s picture

Status: Active » Needs review
FileSize
1.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,921 pass(es). View

Please review.

MKorostoff’s picture

FileSize
140.89 KB

Confirming that the patch in #3 applies cleanly for me, and that the "new comment count" continues to function as expected when commenting on articles.

To test this I did the following

  1. Read the patch to confirm that the function is indeed deleted
  2. Applied the patch, and confirmed that drupal continued booting
  3. Created an article, added comments, and viewed the article teaser as a different user to confirm that the comment count continues functioning
  4. Ran Drupal\comment\Tests\CommentLinksTest in the browser to confirm that all tests still pass.

However, one small item I noticed that could use a bit of additional work: in core/modules/comment/src/Tests/CommentLinksTest.php on lines 234 and 246 there are docblocks which reference comment_num_new(). The overall test remains relevant and functional, but this documentation should be updated to reflect the new function name. I'll update and post a new patch in a moment.

MKorostoff’s picture

Status: Needs review » Needs work
MKorostoff’s picture

FileSize
2.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,255 pass(es). View

This patch 1) removes the deprecated function comment_num_new() and 2) replaces all docblock references to comment_num_new() with the new method \Drupal::service('comment.manager')->getCountNewComments()

MKorostoff’s picture

Status: Needs work » Needs review
MKorostoff’s picture

FileSize
1.49 KB

Here's an interdiff for the above patch

YesCT’s picture

I wonder if that comment is still accurate.

MKorostoff’s picture

Yup, that docblock is indeed still accurate.

Here's the behavior that docblock is describing:

  1. User Alice lacks access to comments.
  2. User Bob makes a comment on node/123
  3. User Alice views node/123. User Alice does not see user Bob's comment. User Alice closes the tab.
  4. User Alice receives a new user role, and is now able to access comments.
  5. User Alice views the teaser for node/123
  6. User Alice should not see "1 new comment," because Bob's comment was marked as read the last time she visited node/123 (despite the fact that she never saw Bob's comment in reality).

I've tested this behavior by creating dummy user accounts and dummy content, and it is indeed preserved in getCountNewComments()

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

ok. I read all the lines of the patch.

did
ag "comment_num_new" core/*

this is getting them all as far as I can see, and the edited comments seem fine (no out of scope changes, and wrapped for the longer reference)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 46a9a49 and pushed to 8.x. Thanks!

  • alexpott committed 46a9a49 on
    Issue #2296839 by MKorostoff, er.pushpinderrana | YesCT: Remove...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.