Add filters to the comments administration pages

+++ b/core/modules/comment/comment.post_update.php
@@ -36,3 +36,25 @@ function comment_post_update_enable_comment_admin_view() {
+  $entity_type_manager = \Drupal::entityTypeManager();
...
+  $entity_type_manager

there's no reason for ETM variable cos it used only once

Congratulations on getting the conversion in! I'm wondering whether we should do some simple testing of the exposed filters?

+++ b/core/modules/comment/comment.post_update.php
@@ -36,3 +36,31 @@ function comment_post_update_enable_comment_admin_view() {
+  if ($view = $view_storage->load('comment')) {
+    // If we have an existing view then update that.
+    $new_view
+      ->set('uuid', $view->uuid())
+      ->enforceIsNew(FALSE);
+  }
+  $new_view->save();

Do we consider it as fine to override a view someone might have adapted? I guess its fine, given we have never promised it?

Do we have any way to handle/detect if people made modifications to the View before we load the new version? Cause this way we just discard any updates that people have done.

I think a lot more people will benefit from adding the filters but this could theoretically lead to loss of data. And we can't really mark UI exposed config as @internal.

Edit: cross post with @dawehner, guess we thought the same thing :)

If we would stay in 8.4.x, we could just override it, we wouldn't even need to provide an update path, given that we don't need to support one.

Let's assume this won't make it to 8.4.x, but rather just in 8.5. IN that case we could use the config hash stored in _core to determine whether things got changed.

We can totally drop upgrade path for it if it won't make it to 8.4.x.

Let's a good idea as well. Given that we don't want an update path in 8.4.x, but then also want to drop one afterwards, let's just no worry about it anymore 🎆

Nice work with the test!

Looks good to me!

Nice test coverage!

This looks great! Nice to see the test coverage as well.

1. 

   I was going to say that the language field should only be displayed on a multilingual site "like for nodes"... but then discovered that admin/content has a language filter even on a monolingual site in HEAD, with all the confusing/overwhelming options that nodes do. So I guess that's not in scope here, but I'd want to see a followup or two to (a) only display the language filter when content might exist in more than one language and (b) not show confusing, irrelevant options. Issues for these might already exist.

2. The headers should be sentence case rather than title case. ("Author name" rather than "Author Name" and "Comment type" rather than "Comment Type". (Compare to admin/content.)

   

3. I'm also not sure we should ship with "Comment type" as a default filter, since (unlike content types) multiple comment types are not the 80% usecase, and Standard itself only ships with one comment type.

4. +++ b/core/modules/comment/comment.post_update.php
   @@ -36,3 +36,31 @@ function comment_post_update_enable_comment_admin_view() {
   +/**
   + * Update the comment admin view.
   + */
   +function comment_post_update_update_comment_admin_view() {
   +  $module_handler = \Drupal::moduleHandler();
   

   I don't think we should have this in the patch; the view has only been added in 8.4.x which has not shipped a release yet. Unless I've missed something?

5. I noticed another maybe-out-of-scope thing, which is the unhelpful label "Update Options" on the bulk update field. (I think this is in the view in HEAD too.) admin/content has "Action" as the label, which is more informative.

Okay, out-of-scope point 1 is a whole can of worms. Relating some of the related issues.

I wonder if we could improve the situation by only displaying these exposed filters if more than one kind of the thing exists on the site? That would also fix point 3. That's magic and an extra query, so probably should be a followup, and we go with just doing the same meh thing that nodes do at the moment. So let's make that a followup issue (for both this view and admin/content maybe), and just go with my suggestion of removing the comment type filter for now.

IMO comment type is useless
Right now it used just to point to which entity type the comment field is bound to.
But 80% case is nodes
If someone will decide to add comments to products or taxonomy able to edit default view and customize...
To have a documentation page with few sentences about that makes sense otoh better to add it to change record

I still think we should remove the comment type filter. (A content type filter from the node relationship would me more useful than a comment type filter, even. Edit: Not saying we should add that since it would make the view node-specific; just offering it as an example.) If someone wants to have multiple comment types on their site, it's very easy for them to add a comment type filter to the view (that's the point of it being a view, after all). :)

I don't think it's tricky to add later either if we decide it is worthwhile?

I guess a usability maintainer could weigh in on whether we should include that filter.

I agree with @xjm, it seems like we would be optimizing for a use case thats just not the 80%, and people who do that likely know how to adjust a view.

If possible we can do some special logic to hide it if it's only one?

Then we should not show it, lets aim for the 80% usecase.

IMO good to go

Awesome test coverage! Just some really tiny nits:

1. +++ b/core/modules/comment/config/optional/views.view.comment.yml
   @@ -600,6 +665,128 @@ display:
   +            identifier: combine
   

   +++ b/core/modules/comment/config/optional/views.view.comment.yml
   @@ -733,6 +938,128 @@ display:
   +            identifier: combine
   

   Maybe use 'author_name' here? Much clearer in the URL when searching (would also need an update in the test then afterwards)

2. +++ b/core/modules/comment/tests/src/Kernel/Views/CommentAdminViewTest.php
   @@ -0,0 +1,204 @@
   +    // Unpublish the comment to test the Unapproved comments tab.
   

   comment => comments

3. +++ b/core/modules/comment/tests/src/Kernel/Views/CommentAdminViewTest.php
   @@ -0,0 +1,204 @@
   +    // Test the combine filter using  username.
   

   one space too much between using and username.

Nice. Updated the screenshot to the most recent version of the patch.

1. +++ b/core/modules/comment/config/optional/views.view.comment.yml
   @@ -641,10 +828,27 @@ display:
   +      relationships:
   +        uid:
   +          id: uid
   

   So this does add a relationship to the existing view, which we'd typically want to profile per https://www.drupal.org/core/gates#performance. However, I looked over #1986606: Convert the comments administration screen to a view and I don't see any documentation of profiling there either. :P I'll check with catch.

2. +++ b/core/modules/comment/tests/src/Kernel/Views/CommentAdminViewTest.php
   @@ -0,0 +1,204 @@
   +    $this->doTestFilters('page_1');
   ...
   +    $this->doTestFilters('page_2');
   

   Does this mean that we didn't give semantic display names to the tabs of the comment view? We should fix that if so. (Out of scope here but we'd want to backport that if possible before beta.)

3. +++ b/core/modules/comment/tests/src/Kernel/Views/CommentAdminViewTest.php
   @@ -0,0 +1,204 @@
   +   * Runs tests comment admin view display.
   

   Nit: This is not really a sentence.

4. +++ b/core/modules/comment/tests/src/Kernel/Views/CommentAdminViewTest.php
   @@ -0,0 +1,204 @@
   +    // Assert the expose filters on the admin page.
   

   Nit: exposed filters.

5. +++ b/core/modules/comment/tests/src/Kernel/Views/CommentAdminViewTest.php
   @@ -0,0 +1,204 @@
   +    $this->assertEquals(1, count($elements), 'Only anonymous comment is visible.');
   +    $executable->destroy();
   ...
   +    $this->assertEquals(1, count($elements), 'Only admin comment is visible.');
   +    $executable->destroy();
   ...
   +    $this->assertEquals(1, count($elements), 'Only anonymous comment is visible.');
   ...
   +    $this->assertEquals(1, count($elements), 'Only admin comment is visible.');
   

   This isn't actually asserting that the anonymous comment is visible; it's asserting that one comment of some sort is visible. We should add assertText()s and assertNoText()s for the comment we expect to have displayed.

6. +++ b/core/modules/comment/tests/src/Kernel/Views/CommentAdminViewTest.php
   @@ -0,0 +1,204 @@
   +    // Test the language filter.
   +    $executable->setExposedInput(['langcode' => '***LANGUAGE_site_default***']);
   +    $build = $executable->preview($display_id);
   +    $this->setRawContent($renderer->renderRoot($build));
   +    $this->verbose($this->getRawContent());
   +
   +    $elements = $this->cssSelect('input[type="checkbox"]');
   +    $this->assertEquals(2, count($elements), 'Both comments are visible.');
   +    $executable->destroy();
   

   Should we test with another language also?

For #32.2.

I will work on analyzing the query changes using EXPLAIN to see performance impact. First will need to generate many users and comments since the join is on the user table.

Fixed #32.3 and #32.4

Tim Plunkett said I don't need an interdiff. Leaving it at needs work because the other issues still need to be fixed.

#2901778: Provide correct display name and title for comment admin view displays

I've created a follow-up issue for #32.2.

I was having some development environment issues and wasn't able to get to finish performance testing today.

Sorry. I didn't mean to change the status.

The 4 possible queries running here are

1. No filters used.
   • No index scan used on left join to field_users_data. Note that MySQL EXPLAIN in the above related issue also does this, and this is not necessarily a bad thing.
2. Filter on author name using the combine plugin (concatenating two columns and doing a like).
   • No index scan used on left join to field_users_data.
3. Filter on subject
   • Index scan (uid, default langcode, langcode) used on merge right join to field_users_data.
4. Filter on author name and subject.
   • Index scan (uid, default langcode, langcode) used on merge right join to field_users_data.

In PostgreSQL 9.6 the query planner did well for everything except #1 and #2 and used the index on the users_field_data table. However for some reason it did not choose the index scan when only the author name filter was done. Not using the index increased the "cost" of the query significantly. If I only filtered on the comment author name, then the index WAS used. It's possible the query planner made the correct decision because as the number of rows increase it gets more expensive to run the index. This probably happened on my VM with 4gb memory using 1 vcpu with 10000 users and 10000 nodes with max of 50 comments where the name I searched for returned about 1-50 rows (after limit). This was also done after auto vacuum as I got the data in there yesterday and have done subsequent restarts of the VM since then.

MySQL should also have similar behavior with choosing indexes, and it looks like EXPLAIN in #1986606: Convert the comments administration screen to a view.

I did not see any significant issues with the query, and I think views is doing a good job here.

As @andypost noted in #1986606: Convert the comments administration screen to a view there may be render performance issues, but that's probably more to do with views in general rather than adding in filters? If anything filters would improve render performance by reducing the number of comment entities to load individually.

Thanks @mradcliffe, nice detailed summary.

Thanks @mradcliffe, appreciate the profiling and analysis

larowlan did a blackfire comparison and it looks like there's a small performance improvement in rendering (which has he pointed out, makes sense given Views has had a lot of render caching attention compared to comment admin controller by now). Thanks for the testing!

The above addresses all my feedback. RTBCing (which means I can't commit it), but if someone else can review/confirm @jibran's changes then I could commit it also.

The last round of changes (last two interdiffs) look good to me.

Alright, committed and pushed to 8.5.x and backported to 8.4.x since this view is new in 8.4.x anyway. Thanks everyone!