Comment views field handlers need to be replaced with field/entity aware handlers

Here's the first pass at this. It doesn't handle the changes related to comment_depth as of yet. If comment_depth is switched to 'field' instead of the custom formatter the output in views indeed doesn't look right as suggested in the proposed resolution. Next step providing this patch passes would be to switch the Depth class to derive from the Field API formatter.

That patch will fail testing as per issue #2090115 which moved the physical location of the install yml files from where they were when I rolled the patch. I'll re-roll and re-submit.

Same patch as before reflecting the move of those yml files to the optional subdirectory in issue #2090115.

There's a separate issue for "name" #2455131: Field comment_field_data.field_name should be using Field API formatter

Some work on it.

Let's fix stuff.

Alright.

Per #2393339-57: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality, tagging for critical office hours, but if there's a reason to not have this particular child issue in that list, please untag it.

The issue summary says the scope of this issue includes comment_username and comment_depth, but those conversions aren't in the patch yet. Meanwhile, #2454163: Replace comment_username handler with generic views handler is open as a separate issue without a patch and not yet prioritized as critical. Should we close that as a duplicate or does it make sense for that to be split out?

Should we close that as a duplicate or does it make sense for that to be split out?

I think it makes sense to have it split up, given that this issue doesn't need yet a new handler yet.

I adapted the comment_depth one, given that I see no usecase in exposing that as a formatter to be honest.

We should be able to remove the handlers now.

Pretty close here, only nits and one question about a deletion

1. +++ /dev/null
   @@ -1,122 +0,0 @@
   -<?php
   

   Less code++

2. +++ b/core/modules/comment/src/Plugin/views/field/Depth.php
   @@ -17,15 +17,20 @@
   +      $item['rendered']['#markup'] =  count(explode('.', $comment_thread)) - 1;
   

   nitpick two spaces after the =

   note to self: this should be a method on CommentInterface cause we do it a few times in Comment class too.

3. +++ /dev/null
   @@ -1,39 +0,0 @@
   -  public function render(ResultRow $values) {
   

   Removing this file seems unrelated - if we remove it, we need a formatter to replace it? Or is this totally unused because the 'comment' column on {node} is loooong dead?

4. +++ b/core/modules/comment/src/Tests/Views/DefaultViewRecentCommentsTest.php
   @@ -114,19 +114,20 @@ protected function setUp() {
   +    $user = $this->drupalCreateUser(['access comments']);
   

   Love that we had tests that validated insecure access to comments.

5. +++ b/core/modules/system/src/Tests/Cache/PageCacheTagsIntegrationTest.php
   @@ -105,6 +105,12 @@ function testPageCacheTags() {
   +    // Render the view block adds the languages cache context.
   

   %s/Render/Rendering?

6. +++ b/core/modules/system/src/Tests/Cache/PageCacheTagsIntegrationTest.php
   @@ -105,6 +105,12 @@ function testPageCacheTags() {
   +    // As the languages is the parent of the languages::LanguageInterface::TYPE_INTERFACE
   

   nitpick >80

fixed the nits. question from 19.3 still unanswered.

Removing this file seems unrelated - if we remove it, we need a formatter to replace it? Or is this totally unused because the 'comment' column on {node} is loooong dead?

Yeah, that handler is not used at all anymore in views.

Thank you @yesct for fixing the review points!

Let's fix the failures.

Tried to manually test on simplytest.me, didn't apply - re-roll

Shots from manual testing, comments on left, view on right

+++ /dev/null
@@ -1,39 +0,0 @@
-class NodeComment extends FieldPluginBase {

But there's still the same named filter plugin. But it has nothing about node

But there's still the same named filter plugin. But it has nothing about node

Well, we just care about field handlers here.

Looks like we need to update CommentViewsFieldAccessTest.

Sure.

Testbot please test.

Fixed those test failures.

@alexpott, @effulgentsia, @xjm and I discussed this on a D8 critical triage call today. Since comments are a content entity, like nodes and taxonomy terms, I think the expectation of users would be that access control would be respected in any given display, and if this wasn't fixed prior to release would likely result in an SA. Therefore, tagging as a triaged critical.

Ready to go

Applied again ... #2450153: Add a default_formatter to UUIDs fields conflicted with it.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed dc47451 and pushed to 8.0.x. Thanks!