Problem/Motivation

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

Steps to reproduce

Proposed resolution

Deprecate route comment.new_comments_node_links and move to history.new_comments_node_links

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#9 3542528-nr-bot_8lozaowj.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3542528

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

quietone created an issue. See original summary.

quietone opened merge request !13058

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

mstrelan’s picture

mstrelan commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Seems like a good deprecation to me. Thanks for pointing out core/modules/comment/tests/src/Functional/CommentNewIndicatorTest.php as that was going to be my only comment.

Rest LGTM.

catch’s picture

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

I think we could probably use route aliasing on the deprecated comment route as well, per https://www.drupal.org/node/3317784?

mstrelan’s picture

mstrelan commented

I'm not sure about #6, because I think a route alias should have the same path, or at least it does for router_test.deprecated.

Also not sure it works if history module is not installed and the route doesn't exist, although the controller throws a 404 in that case anyway.

I did notice that the deprecation format was incorrect though, so I've fixed that up.

smustgrave’s picture

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

That work @catch?

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3542528-nr-bot_8lozaowj.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

mstrelan’s picture

mstrelan commented
Status: Needs work » Reviewed & tested by the community

Clean rebase, not sure why the bot complained. Restoring RTBC as I didn't do anything else.

$ git fetch origin
$ git rebase origin/11.x
Successfully rebased and updated refs/heads/3542528-deprecate-route-comment.newcommentsnodelinks.
astonvictor’s picture

astonvictor commented

works for me. thanks for your work
+1 RTBC

catch’s picture

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

#7 is fair enough, also this isn't a route that anyone is likely to reference so not really worth trying to figure out anything complex.

Committed/pushed to 11.x, thanks!

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

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

Maintainers, please credit people who helped resolve this issue.

  • catch committed 4a4c358e on 11.x 
    Issue #3542528 by quietone, mstrelan: Deprecate route comment....

catch closed merge request !13058

mstrelan’s picture

mstrelan commented

Published the CR

Status: Fixed » Closed (fixed)

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