Simplify what CommentLinkBuilder is doing - consider removing a lot of the functionality

Having worked on #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user, I remember what an incredible PITA this was. Fully agreed on this being more complex than it should be.

Thoughts:

1. we literally have if ($this->moduleHandler->moduleExists('history')) { in there. This is simply ridiculous. We could easily move this into the History module, thanks to hook_node_links_alter()
2. it could help a lot of if we didn't hardcode certain logic to certain view modes, but if we made that configurable?

Moving history stuff to history module is good - we didn't have a separate history module until relatively recently.

If history module has to check for comment module I think we can live with that, better than vice versa. Formatter might save doing even that.

Generally cleaning this up agreed on doing that and fine in terms of beta changes policy at https://www.drupal.org/node/2368133 - this makes things better for contrib, reduces fragility in core, only 'disruption' is likely to be gutting big chunks out of this function.

Generally agreed on simplifying here. Its predecessor in D6/7 was my go-to example of excessive complexity in the past. :-) My main question is that "punt this to a formatter" seems like a hand-wavy answer. A field can only have one formatter per view mode at a time; if there's 50 possible links that are reasonable to be there, moving the big blob of logic from this function to a formatter doesn't really improve matters, does it? How would we avoid just moving the ugly around?

Is there anything else we could factor out nicely, like the history links?

My main question is that "punt this to a formatter" seems like a hand-wavy answer.

Agreed on this — it's not clear to me either how that is going to fix things.

Could you explain that some more, larowlan?

This looks nice for WIP.

I like it UI-wise and managing-complexity-wise. Each plugin is instantiated only once and not per commented entity, which is good, otherwise this'd cost us a lot performance-wise.
It's possible that this approach is going to cost us some performance (using the plugin system is another serious abstraction layer), but I suspect this is going to be fine/acceptable.

EDIT: I had looked at the UI from the screenshots. Apparently this requires selecting a different comment field formatter on the commented entity (i.e. choose "Comment links" instead of "Comment list" at /admin/structure/types/manage/article/display). I'd expect this to make the "Links" field on the comment entity itself configurable (i.e. at /admin/structure/comment/manage/comment/display)?

+1 to continue along this path. Ideally with profiling before it gets committed, on a page with 10 commentable nodes, which is the typical expensive case.

#19 looks great!
I'm also think about performance impact

Architecturally I like this direction. Good use of plugins! I also agree with Wim that, from a UX perspective, I would expect to configure this on the comment, not on the node/user/whatever. (Comments can have view modes; I could want to have the links different in different comment view modes, allowing for, say, a "most recent comments" block that doesn't have links but does have body excerpts while full comments have all links.)

+++ b/core/modules/comment/src/Links/CommentLinkPluginBase.php
@@ -0,0 +1,65 @@
+abstract class CommentLinkPluginBase extends PluginBase implements CommentLinkPluginInterface, ContainerFactoryPluginInterface {

Side note: I would love it if we could start using traits for these instead of base classes. :-P (I know, I know, consistency. But a base class can use a trait, too.)

Oh! OK. Do the on-comment links work this way as well? (Seems like it would make sense for all "links on entities" to work in a similar fashion, no?)

Hm. Well, let's forge ahead here and maybe we can make a DX argument for the other later. :-) (If not, eh, there are more versions.)

@Crell there's #1875974: Abstract 'component type' specific code out of EntityDisplay to unify the components of entities

Thought I had commented here earlier - +1 for unraveling this rats' nest, which has given us issues like #2588547: CommentLinkBuilder causes fatal error when comment field not named 'comment', #2593869: Using "New comments" field in a view breaks if history module not enabled, #2594201: No choice of comment field to use for "New comments" view field...

Seems like as we're past the 8.0 point we should pick up #20-22 again for consistency

It would help me tremendously if there was a concise before/after behaviour description. I don't quite understand if the *ed things in the issue summary will all be removed from core altogether, or simply moved to alternate, more modern implementations. If there's no functionality loss/UX regression, and this is all under-the-hood refactoring, then there's no need for product manager sign-off.

@webchick I think the proposal is to actually remove some of the links from core altogether, but agreed it could use a better issue summary (or a mockup/patch that just hacks them out and screenshots).

removing tag for now, the requests in #31 and #32 still stand.

Was going to tag stale-issue-content but this seems like it's probably still relevant.

yes, and while comment's module fate is unclear we lack of opinions