Problem/Motivation

Part of #3524379: [meta] Remove usage of history module from comment module

This will need to move to the history module. Theoretically it could have other usage outside of history module, but according to the interface docs that shouldn't be possible:

   *   The number of new comments or FALSE if the user is not authenticated or
   *   if the History module is not installed.

In core this is currently used by:

  • \Drupal\comment\CommentLinkBuilder::buildCommentedEntityLinks
  • \Drupal\comment\Controller\CommentController::renderNewCommentsNodeLinks
  • \Drupal\comment\Hook\CommentTokensHooks::tokens

Steps to reproduce

Proposed resolution

  • Move implementation to new HistoryManager service in history module
  • Update CommentLinkBuilder::buildCommentedEntityLinks (or HistoryCommentLinkBuilder after #3542540: Deprecate comment libraries and move to history module) to use the new implementation
  • Move comment-count-new token from comment to history
  • Refactor new_comment_count part of \Drupal\Tests\comment\Functional\CommentFieldsTest::testCommentFieldLinksNonDefaultName
  • Refactor [entity:comment-count-new] part of \Drupal\Tests\comment\Functional\CommentTokenReplaceTest::testCommentTokenReplacement done in #3546095: Remove history module from CommentTestBase::$modules

CommentController::renderNewCommentsNodeLinks is handled in #3542528: Deprecate route comment.new_comments_node_links. We can update it here to use the history version of getCountNewComments.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3543035

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mstrelan created an issue. See original summary.

mstrelan’s picture

mstrelan commented
Parent issue: » #3524379: [meta] Remove usage of history module from comment module
mstrelan’s picture

mstrelan commented
Issue summary: View changes

mstrelan opened merge request !13082

mstrelan’s picture

mstrelan commented
Status: Active » Needs work
mstrelan’s picture

mstrelan commented
Title: Deprecate CommentManagerInterface::getCountNewComments » PP-1: Deprecate CommentManagerInterface::getCountNewComments
Status: Needs work » Postponed

Postponed on #3542540: Deprecate comment libraries and move to history module due as this depends on changes to CommentLinkBuilder

mstrelan’s picture

mstrelan commented
Title: PP-1: Deprecate CommentManagerInterface::getCountNewComments » [PP-1] Deprecate CommentManagerInterface::getCountNewComments
Issue summary: View changes
mstrelan’s picture

mstrelan commented
Title: [PP-1] Deprecate CommentManagerInterface::getCountNewComments » Deprecate CommentManagerInterface::getCountNewComments
Status: Postponed » Needs work
mstrelan’s picture

mstrelan commented
Title: Deprecate CommentManagerInterface::getCountNewComments » [PP-1] Deprecate CommentManagerInterface::getCountNewComments
Status: Needs work » Postponed

Rebased for the changes in #3542540: Deprecate comment libraries and move to history module but this is now postponed on #3542528: Deprecate route comment.new_comments_node_links

mstrelan’s picture

mstrelan commented
Title: [PP-1] Deprecate CommentManagerInterface::getCountNewComments » Deprecate CommentManagerInterface::getCountNewComments
Status: Postponed » Needs work

Blocker is in

mstrelan’s picture

mstrelan commented
Issue summary: View changes
Status: Needs work » Needs review
dcam’s picture

dcam commented
Status: Needs review » Needs work

I agreed with the idea of using the procedural service call in HistoryController. So I'm setting this back to Needs Work because of this.

Aside from that, it looks good to me. I could get nit-picky about code style in HistoryTokensHooks::tokens(), but I won't. I know it's because the code was copied faithfully from Comment. And I think it's beneficial for reviewers because it's easy to tell that there's nothing missing.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Updated to use \Drupal::service. I agree with #12, it can always be cleaned up in contrib later.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

I don't think there would be BC issues since this is autowired and probably no one is extending this, but switched it over anyway.

Me either. But who wants to deal with that one site that has it overridden and shows up to find out who broke their stuff?

Feedback has been addressed. I verified that all code was copied from Comment to History correctly. Happy to RTBC this now.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

One comment on the MR.

deepakkm made their first commit to this issue’s fork.

deepakkm’s picture

deepakkm commented
Status: Needs work » Needs review
catch’s picture

catch
he/him
Primary language English
commented

One stray line of the @todo was still in there, but removed that via gitlab suggestions. Back to RTBC.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Assuming #18 meant to RTBC this one?

  • catch committed 2e4c28af on 11.3.x 
    task: #3543035 Deprecate CommentManagerInterface::getCountNewComments...

  • catch committed 30a2de10 on 11.x 
    task: #3543035 Deprecate CommentManagerInterface::getCountNewComments...

catch closed merge request !13082

catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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