Updated #347

Problem/Motivation

Part of #1823450: [Meta] Convert core listings to Views. This also helps solve problems for #1978904: Convert comment_admin() to a Controller and #1946348: Convert all of confirm_form() in comment.admin.inc to the new form interface which currently needs special handling for confirm delete forms.

Proposed resolution

Converting the admin/content/comment* page to a view with operations would keep it more inline with the rest of core listing pages.

Remaining tasks

Review.
Manual testing.

User interface changes

Before update messages


After update messages


Before comment lisiting


After comment lisiting


Before empty text


After empty text


Before action text/choices


After action text/choices


API changes

  • Not exactly a API change but if Views module is enable then comment admin page is replaced by a view.
  • Adds StringFormatter::getEntityUrl() so that comment entity can be linked to permalink URL.
  • Adds comment delete action.
  • Adds post-update hook to enable the comment admin view and comment delete action.
  • Adds comment_permalink formatter and changes default comment subject formatter to comment_permalink.
  • Adds commented_entity views field plugin to use the static cache of entity storage controller.
  • Removes commented entity relationship from comment views data as it is a multiple entity reference field and we can't add relationship to all commented entities.
  • Updated comment ConfirmDeleteMultiple form to make it multilingual aware with tests.
  • Adds comment bulk form views field plugin.
  • Removes admin controller class.
  • And adds a lot of tests.
CommentFileSizeAuthor
#362 convert_the_comments-1986606-362.patch88.57 KBjibran
#352 convert_the_comments-1986606-352.patch88.57 KBjibran
#352 interdiff-b248d8.txt5.73 KBjibran
#350 convert_the_comments-1986606-350.patch84.88 KBjibran
#350 interdiff-42c1fd.txt8.47 KBjibran
#347 screenshot-d8.dev-2017-07-12-00-34-00.png63.3 KBjibran
#347 screenshot-d8.dev-2017-07-12-00-30-49.png62.19 KBjibran
#347 screenshot-d8.dev-2017-07-12-00-23-50.png78.54 KBjibran
#347 screenshot-d8.dev-2017-07-12-00-23-05.png78.25 KBjibran
#347 screenshot-d8.dev-2017-07-12-00-23-37.png56.49 KBjibran
#347 screenshot-d8.dev-2017-07-12-00-22-45.png54.78 KBjibran
#347 screenshot-dcd5l.ply_.st-2017-07-12-00-35-13.png78.27 KBjibran
#347 screenshot-dcd5l.ply_.st-2017-07-12-00-34-44.png82.61 KBjibran
#347 screenshot-dcd5l.ply_.st-2017-07-12-00-22-09.png79.49 KBjibran
#347 screenshot-dcd5l.ply_.st-2017-07-12-00-16-02.png78.81 KBjibran
#347 screenshot-dcd5l.ply_.st-2017-07-12-00-13-03.png76.7 KBjibran
#347 screenshot-dcd5l.ply_.st-2017-07-12-00-12-30.png76.55 KBjibran
#347 Screenshot from 2017-07-12 00-38-58.png11.82 KBjibran
#347 Screenshot from 2017-07-12 00-38-32.png11.79 KBjibran
#347 Screenshot from 2017-07-12 00-13-44.png21.2 KBjibran
#347 Screenshot from 2017-07-12 00-13-30.png20.22 KBjibran
#347 convert_the_comments-1986606-347.patch83.04 KBjibran
#347 interdiff-cd1cde.txt1.89 KBjibran
#346 convert_the_comments-1986606-346.patch82.95 KBjibran
#346 interdiff-7ea53d.txt2.78 KBjibran
#345 convert_the_comments-1986606-345.patch80.58 KBjibran
#345 interdiff-ea2d01.txt713 bytesjibran
#343 convert_the_comments-1986606-343.patch80.58 KBjibran
#343 interdiff-b37866.txt7.92 KBjibran
#340 convert_the_comments-1986606-340.patch80.61 KBjibran
#340 interdiff-c780e5.txt16.53 KBjibran
#335 convert_the_comments-1986606-335.patch79.82 KBvprocessor
#335 convert_the_comments-1986606-335.interdiff.txt454 bytesvprocessor
#332 convert_the_comments-1986606-332.patch79.64 KBvprocessor
#323 convert_the_comments-1986606-322.patch80.42 KBjibran
#322 interdiff.txt4.12 KBjibran
#319 convert_the_comments-1986606-319.patch78.93 KBjibran
#319 interdiff.txt1.67 KBjibran
#318 convert_the_comments-1986606-318.patch78.71 KBjibran
#318 interdiff.txt8.26 KBjibran
#317 convert_the_comments-1986606-317.patch75.35 KBjibran
#317 interdiff.txt3.59 KBjibran
#315 convert_the_comments-1986606-315.patch73.86 KBjibran
#315 interdiff.txt10.47 KBjibran
#311 convert_the_comments-1986606-307.patch73.1 KBandypost
#311 interdiff.txt680 bytesandypost
#306 convert_the_comments-1986606-306.patch73.1 KBandypost
#306 interdiff.txt8.26 KBandypost
#303 convert_the_comments-1986606-303.patch74.95 KBandypost
#303 interdiff.txt7.56 KBandypost
#301 convert_the_comments-1986606-300.patch74.13 KBxjm
#300 convert_the_comments-1986606-300.txt74.13 KBjibran
#300 interdiff.txt1.33 KBjibran
#299 convert_the_comments-1986606-299-interdiff.txt4.05 KBmbovan
#299 convert_the_comments-1986606-299.patch74.05 KBmbovan
#297 convert_the_comments-1986606-297.patch73.59 KBjibran
#297 interdiff.txt1.14 KBjibran
#295 convert_the_comments-1986606-295.patch72.7 KBjibran
#295 interdiff.txt14.74 KBjibran
#292 convert_the_comments-1986606-292.patch79.01 KBjibran
#292 interdiff.txt3.95 KBjibran
#291 convert_the_comments-1986606-291.patch75.06 KBjibran
#291 interdiff.txt3.95 KBjibran
#289 convert_the_comments-1986606-289-interdiff.txt695 bytesmbovan
#289 convert_the_comments-1986606-289.patch71.11 KBmbovan
#287 convert_the_comments-1986606-287-interdiff.txt610 bytesmbovan
#287 convert_the_comments-1986606-287.patch71.12 KBmbovan
#285 comments-admin-view-1986606-285.interdiff.txt2.44 KBArla
#285 comments-admin-view-1986606-285.patch71.07 KBArla
#283 comments-admin-view-1986606-283.patch70.95 KBArla
#273 convert_the_comments-1986606-273.patch70.53 KBdobe
#271 convert_the_comments-1986606-271.patch70.58 KBjibran
#271 interdiff.txt2.02 KBjibran
#265 convert_the_comments-1986606-265.patch69.76 KBjibran
#265 interdiff.txt4.09 KBjibran
#264 convert_the_comments-1986606-264.patch69.28 KBjibran
#264 interdiff.txt786 bytesjibran
#259 convert_the_comments-1986606-259.patch69.26 KBjibran
#259 interdiff.txt4.26 KBjibran
#256 convert_the_comments-1986606-256.patch66.73 KBjibran
#256 interdiff.txt537 bytesjibran
#255 convert_the_comments-1986606-255.patch73.35 KBjibran
#255 interdiff.txt2.46 KBjibran
#253 convert_the_comments-1986606-253.patch66.91 KBjibran
#253 interdiff.txt1.3 KBjibran
#249 convert_the_comments-1986606-247.patch66.79 KBjibran
#249 interdiff.txt612 bytesjibran
#247 provide_a_relationship-2321721-64.patch25.66 KBjibran
#247 interdiff.txt19 KBjibran
#246 convert_the_comments-1986606-246.patch66.76 KBjibran
#246 interdiff.txt4.62 KBjibran
#243 interdiff.txt12.22 KBjibran
#242 convert_the_comments-1986606-242.patch66.59 KBjibran
#242 interdiff.txt0 bytesjibran
#240 convert_the_comments-1986606-240.patch68.76 KBjibran
#240 interdiff.txt590 bytesjibran
#238 convert_the_comments-1986606-238.patch68.68 KBjibran
#238 interdiff.txt1.29 KBjibran
#238 interdiff.txt5.05 KBjibran
#232 convert_the_comments-1986606-232.patch66.41 KBjibran
#232 interdiff.txt33.96 KBjibran
#228 convert_the_comments-1986606-228.patch47.43 KBjibran
#228 interdiff.txt4.71 KBjibran
#220 convert_the_comments-1986606-220.patch49.94 KBArla
#220 convert_the_comments-1986606-220.interdiff.txt545 bytesArla
#218 Screen Shot 2015-05-24 at 19.26.23 .png9.39 KBmiro_dietiker
#218 Screen Shot 2015-05-24 at 19.25.52 .png16.13 KBmiro_dietiker
#216 convert_the_comments-1986606-216.patch49.94 KBjibran
#216 interdiff.txt1.62 KBjibran
#210 convert_the_comments-1986606-210.patch49.64 KBjibran
#210 interdiff.txt1.86 KBjibran
#208 convert_the_comments-1986606-208.patch49.73 KBjibran
#208 interdiff.txt1.86 KBjibran
#206 Screenshot 2015-05-16 17.06.53.png208.26 KBlarowlan
#204 screenshot-d8.dev 2015-05-16 08-09-47.png66.35 KBjibran
#204 screenshot-d8.dev 2015-05-16 08-08-54.png65.29 KBjibran
#204 screenshot-d8.dev 2015-05-16 08-10-34.png48.17 KBjibran
#204 screenshot-d8.dev 2015-05-16 08-11-25.png60.55 KBjibran
#204 convert_the_comments-1986606-204.patch48.92 KBjibran
#204 interdiff.txt8.64 KBjibran
#201 convert_the_comments-1986606-201.patch42.74 KBjibran
#201 interdiff.txt15.18 KBjibran
#199 convert_the_comments-1986606-199.patch34.88 KBjibran
#199 interdiff.txt1.96 KBjibran
#195 interdiff.txt27.63 KBkim.pepper
#195 1986606-comment-view-195.patch34.47 KBkim.pepper
#192 convert_the_comments-1986606-192.patch34.19 KBjibran
#188 1986606-188.patch35.27 KBjibran
#188 interdiff.txt777 bytesjibran
#187 1986606-187.patch35.39 KBjibran
#187 interdiff.txt788 bytesjibran
#185 1986606-185.patch35.01 KBjibran
#177 1986606-comment-admin-view-177.patch35.12 KBpcambra
#177 interdiff.txt3.46 KBpcambra
#174 Screenshot 2014-12-30 08.15.58.png29.79 KBlarowlan
#173 Screenshot 2014-12-30 08.12.59.png38.53 KBlarowlan
#173 Screenshot 2014-12-30 08.11.58.png52.7 KBlarowlan
#172 Screenshot 2014-12-30 07.38.43.png47.68 KBlarowlan
#169 1986606-comment-admin-view-169.patch34.84 KBpcambra
#169 interdiff.txt5.01 KBpcambra
#166 1986606-comment-admin-view-166.patch36.17 KBpcambra
#166 interdiff.txt5.5 KBpcambra
#158 1986606-comment-admin-view-158.patch38.86 KBpcambra
#158 interdiff.txt2.53 KBpcambra
#156 1986606-comment-admin-view-156.patch38.65 KBpcambra
#156 interdiff.txt2.24 KBpcambra
#154 1986606-comment-admin-view-154.patch38.7 KBpcambra
#154 interdiff.txt31.01 KBpcambra
#151 1986606-comment-admin-view-151.patch61.61 KBpcambra
#151 interdiff.txt15.77 KBpcambra
#149 1986606-comment-admin-view-149.patch63.19 KBpcambra
#146 1986606-diff-145-146.txt538 bytesvijaycs85
#146 1986606-comment-admin-view-146.patch63.18 KBvijaycs85
#145 1986606-diff-142-145.txt1.74 KBvijaycs85
#145 1986606-comment-admin-view-145.patch63.71 KBvijaycs85
#142 1986606-diff-140-142.txt1.56 KBvijaycs85
#142 1986606-comment-admin-veiew-142.patch65.17 KBvijaycs85
#140 1986606-comment-admin-veiew-140.patch65.16 KBvijaycs85
#129 interdiff.txt5.64 KBdawehner
#129 vdc-1986606-129.patch67.24 KBdawehner
#128 before_approved.png72.18 KBxjm
after_approved.png56.42 KBxjm
#125 after_unapproved.png49.35 KBxjm
#125 after_approved.png56.42 KBxjm
#125 before_unapproved.png318.43 KBxjm
#125 before_approved.png328.55 KBxjm
#118 interdiff.txt502 bytesdawehner
#118 vdc-1986606-118.patch66.59 KBdawehner
#116 interdiff.txt1.97 KBdawehner
#116 vdc-1986606-116.patch66.59 KBdawehner
#113 interdiff.txt15.69 KBdawehner
#113 vdc-1986606-113.patch66.67 KBdawehner
#111 1986606-comment_admin_view-109.patch98.9 KBtkuldeep17
#109 1986606-comment_admin_view-109.patch98.71 KBtkuldeep17
#103 1986606-diff-100-103.txt1.46 KBvijaycs85
#103 1986606-comment_admin_view-103.patch61.18 KBvijaycs85
#100 1986606-diff-97-100.txt6.71 KBvijaycs85
#100 1986606-taxonomy-views-100.patch61.25 KBvijaycs85
#97 comment-admin-screen.png84.69 KBvijaycs85
#97 1986606-diff-80-97.txt1.31 KBvijaycs85
#97 1986606-taxonomy-views-97.patch58.71 KBvijaycs85
#85 80-1986606.patch1.34 MBadnen
#84 80-1986606.patch1.34 MBadnen
#84 80-1986606.patch1.34 MBadnen
#79 comments_administration_view-1986606-79.patch58.88 KBsnig
#76 interdiff.txt751 bytesdawehner
#76 vdc-1986606.patch59.09 KBdawehner
#73 interdiff.txt2.59 KBjibran
#73 d8-comment-view-1986606-73.patch58.35 KBjibran
#71 interdiff.txt15.01 KBjibran
#71 d8-comment-view-1986606-71.patch55.76 KBjibran
#62 d8-comment-view-1986606-61.patch41.97 KBjibran
#45 1986606-comments_admin_views-45.patch91.11 KBpcambra
#45 interdiff.txt41.57 KBpcambra
#42 interdiff.txt7.73 KBpcambra
#42 1986606-comments_admin_views-42.patch85.66 KBpcambra
#38 interdiff.txt3.3 KBpcambra
#38 1986606-comments_admin_views-37.patch85.38 KBpcambra
#34 1986606-comments_admin_views-34.patch86.12 KBpcambra
#33 Comments drupal8.local 2013-06-29 10-16-46.png56.13 KBpcambra
#32 Comments drupal8.local 2013-06-29 10-10-27.png126.3 KBpcambra
#29 comments-before.png83.57 KBkim.pepper
#29 comments-after.png77.39 KBkim.pepper
#26 1986606-comments_admin_views-26.patch85.67 KBpcambra
#24 interdiff.txt20.04 KBpcambra
#24 1986606-comments_admin_views-24.patch85.77 KBpcambra
#21 interdiff.txt32.14 KBpcambra
#21 1986606-comments_admin_views-21.patch66.71 KBpcambra
#20 interdiff.txt40.22 KBpcambra
#20 1986606-comments_admin_views-20.patch67.93 KBpcambra
#19 interdiff.txt10.42 KBpcambra
#19 1986606-comments_admin_views-19.patch57.44 KBpcambra
#12 interdiff.txt934 bytespcambra
#12 1986606-comments_admin_views-12.patch53.2 KBpcambra
#11 1986606-comments_admin_views-11.patch53.2 KBpcambra
#11 interdiff.txt5.3 KBpcambra
#9 1986606-comments_admin_views-9.patch48.99 KBpcambra
#9 interdiff.txt48.77 KBpcambra
#5 1986606-comments_admin_views-5.patch31.48 KBpcambra
#5 interdiff.txt2.26 KBpcambra
#3 1986606-comments_admin_views-3.patch31.38 KBpcambra
#3 interdiff.txt17.61 KBpcambra
#2 1986606-comments_admin_views-2.patch28.9 KBpcambra
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pcambra’s picture

Assigned: Unassigned » pcambra
Issue tags: +drupalconportland
pcambra’s picture

Status: Active » Needs work
FileSize
28.9 KB

Here's a very initial take, listing has been exported to views and added some bulk operations.

TODO's

  • Add operations edit link (it's throwing an error).
  • Add count for unapproved comments on the top. "Unapproved comments (5)"
  • Tweak bulk operations form: unpublish & delete
  • Update tests
  • Expose some filters by default?
pcambra’s picture

Status: Needs work » Needs review
FileSize
17.61 KB
31.38 KB

Here's a new version of the patch, added a delete action and tests for it.

Also discussed with dawehner, we're delaying adding the number of comments to the "Unapproved comments" menu item as it could be a performance risk and actually there's no "right" way to do it apparently.

pcambra’s picture

Assigned: pcambra » Unassigned
Status: Needs work » Needs review
FileSize
2.26 KB
31.48 KB

The tab menus are not working good enough, I'd say that there's a conflict with the parent menu so the tabs are displayed but the parent is always the original screen, we could embed the view in case views is enabled but there are also other options such as replacing the secondary tabs by a filter.

Also, actions module is not a views dependency, resulting into a Broken/Missing handler in the view by default which I don't think is a good thing to display, we might want to add a custom form just the way as admin/content and admin/people are doing for other reasons.

tim.plunkett’s picture

#1846172: Replace the actions API will change how the actions stuff works.
The tab issues are caused by #2011006: Default local tasks provided by Views are broken

pcambra’s picture

Assigned: Unassigned » pcambra
Issue tags: -drupalconportland +drupaldevdays

Resuming work in this

pcambra’s picture

Status: Needs work » Needs review
FileSize
48.77 KB
48.99 KB

Here's the view, I guess that we can work in the entity list controller in #1978904: Convert comment_admin() to a Controller and deal with the number of unread comments in a follow up.

pcambra’s picture

Status: Needs work » Needs review
FileSize
5.3 KB
53.2 KB
pcambra’s picture

A little leftover regarding excluded fields

pcambra’s picture

Issue tags: +Needs tests

I guess we need better testing support for the comment admin view, sort of NodeAdminTest, but not sure what makes sense to test here.

damiankloip’s picture

+++ b/core/modules/comment/config/views.view.comments.ymlundefined
@@ -0,0 +1,1454 @@
+      empty:
+        area:
+          id: area
+          table: views
+          field: area
+          relationship: none
+          group_type: group
+          admin_label: ''
+          empty: '1'
+          content: "<p>No comments available.</p>\n"
+          format: basic_html
+          tokenize: '0'

This should use the area_text_custom ('Unfiltered text') handler instead. Otherwise we are tied to a format, and if that is not available nothing will be displayed at all.

+++ b/core/modules/comment/config/views.view.comments.ymlundefined
@@ -0,0 +1,1454 @@
+uuid: df052bd2-a90f-4632-811b-277cf6ef38eb

At the moment I thought we were still removing UUIDs but not sure if that patch/issue to add UUIDs to default config got resolved yet?

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/LinkDelete.phpundefined
@@ -19,16 +19,19 @@
+  function renderLink($comment, $values) {

public function, while we're there :)

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/LinkEdit.phpundefined
@@ -37,10 +37,15 @@ public function buildOptionsForm(&$form, &$form_state) {
+  public function access() {
...
+  function renderLink($comment, $values) {

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/LinkReply.phpundefined
@@ -19,17 +19,15 @@
+  function renderLink($comment, $values) {

Can we get a docblock for these (and public)? again, while we're there...

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentActionsTest.phpundefined
@@ -76,4 +76,22 @@ function testCommentUnpublishByKeyword() {
+  function testCommentDeleteAction() {

public

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentActionsTest.phpundefined
@@ -76,4 +76,22 @@ function testCommentUnpublishByKeyword() {
+    $action = entity_load('action', 'comment_delete_action');
...
+    $comment = comment_load($comment->id());

Maybe we should be trying to use the storage controller instead? Would be good to get a definitive answer on this, because I'm not actually sure myself.

+++ b/core/modules/views/lib/Drupal/views/Tests/DefaultViewsTest.phpundefined
@@ -105,10 +112,37 @@ protected function setUp() {
+   * Returns a new term with random properties in vocabulary $vid.

@return needed

pcambra’s picture

From a conversation with @damiankloip the test we might need would be in the lines of FrontPageTest:

  • Make sure it's on the page we expect
  • Check if the empty text is there
  • Add comments and check if they're there
  • Maybe the comment delete link needs it's own test

"We shouldn;t need to test too much as view tests should cover a lot of these things"

dawehner’s picture

Some additional feedback.

+++ b/core/modules/comment/comment.views.incundefined
@@ -247,15 +247,23 @@ function comment_views_data() {
-  $data['comment']['view_comment'] = array(
+  // Define some fields based upon views_handler_field_entity in the entity
+  // table so they can be re-used with other query backends.
+  // @see views_handler_field_entity
+
+  // Define the base group of this table. Fields that don't have a group defined
+  // will go into this field by default.

I don't think that this is longer true in Drupal 8 world and at least we should not assume it.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/Link.phpundefined
@@ -44,13 +44,12 @@ public function query() {}
+    return $this->renderLink($comment, $values);
...
-  function render_link($data, $values) {
+  function renderLink($comment, $values) {

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/LinkApprove.phpundefined
@@ -23,17 +23,14 @@ public function access() {
+  function renderLink($comment, $values) {

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/LinkDelete.phpundefined
@@ -19,16 +19,19 @@
+  function renderLink($comment, $values) {

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/LinkEdit.phpundefined
@@ -37,10 +37,15 @@ public function buildOptionsForm(&$form, &$form_state) {
+  function renderLink($comment, $values) {
+    parent::renderLink($comment, $values);

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/LinkReply.phpundefined
@@ -19,17 +19,15 @@
+  function renderLink($comment, $values) {

Then let's typehint this with CommentInterface

+++ b/core/modules/views/lib/Drupal/views/Tests/DefaultViewsTest.phpundefined
@@ -60,9 +67,9 @@ protected function setUp() {
+    $field_name = drupal_strtolower($this->randomName());

Let's better do Unicode::strtolower

pcambra’s picture

Thanks @dawehner & @damiankloip

This should use the area_text_custom ('Unfiltered text') handler instead. Otherwise we are tied to a format, and if that is not available nothing will be displayed at all.

Done.

At the moment I thought we were still removing UUIDs but not sure if that patch/issue to add UUIDs to default config got resolved yet?

As for discussed on IRC, uuids should be there.

while we're there :)

Fixed those as well :). Also "typehinted" with CommentInterface

Maybe we should be trying to use the storage controller instead? Would be good to get a definitive answer on this, because I'm not actually sure myself.

Replaced, let's see how it looks.

I don't think that this is longer true in Drupal 8 world and at least we should not assume it.

Grabbed that from the content one, is just the comment what is wrong or it's the actual thing?

Maybe the comment delete link needs it's own test

We've got a test for the action.

Attaching a new patch covering the above, still need to create a new test.

pcambra’s picture

Issue tags: -Needs tests
FileSize
67.93 KB
40.22 KB

Added tests and fixed the optional/broken links

pcambra’s picture

Removed the views_entity_comment references from the data hooks and added a test for the empty text

pcambra’s picture

After discussing with @dawehner, this and #1978904: Convert comment_admin() to a Controller needs to happen at the same time, so I'll include the fallback conversion of comment_admin() to a controller in this patch.

pcambra’s picture

pcambra’s picture

Status: Needs work » Needs review
FileSize
85.77 KB
20.04 KB

Added support for multiple comment deletion using tempstorage as nodes do and menu page callback cleanup.

It's not converted to wscci yet, but that could go to a followup just as the node is.

pcambra’s picture

It had a conflict with the comment approve issue, here's a re-roll to HEAD

dawehner’s picture

Issue tags: +Needs screenshots

It would be great to have some screenshots of before and after in the summary.

+++ b/core/modules/comment/comment.admin.incundefined
@@ -279,7 +80,7 @@ function comment_confirm_delete_page(Comment $comment) {
 /**
  * Form constructor for the confirmation form for comment deletion.
  *
- * @param Drupal\comment\Comment $comment
+ * @param Drupal\comment\CommentInterface $comment
  *   The comment that is about to be deleted.
  *

This change seems out of scope, especially because the actual code did not changed. Let's try to have the smallest change as possible.

+++ b/core/modules/comment/comment.admin.incundefined
@@ -315,3 +116,153 @@ function comment_confirm_delete_submit($form, &$form_state) {
+  $query = db_select('comment', 'c')
+    ->extend('Drupal\Core\Database\Query\PagerSelectExtender')
+    ->extend('Drupal\Core\Database\Query\TableSortExtender');
+  $query->join('node_field_data', 'n', 'n.nid = c.nid');
+  $query->addTag('node_access');
+  $result = $query
+    ->fields('c', array('cid', 'nid', 'subject', 'name', 'changed', 'status'))
+    ->limit(50)
+    ->orderByHeader($header)
+    ->execute();

So if we are build up a fallback we should do it properly and use EQ, and maybe even a fallback

kim.pepper’s picture

FileSize
77.39 KB
83.57 KB

Before:

comments-before.png

After:

comments-after.png

klonos’s picture

So, we're still missing the bulk operations part.

tim.plunkett’s picture

See core/modules/node/lib/Drupal/node/Plugin/views/field/NodeBulkForm.php and core/modules/node/node.views.inc line 230 for how comment should go about implementing a bulk form handler

pcambra’s picture

This is with views module enabled, implementing the actions, it's the fallback what doesn't have the operations anymore.

Comments  drupal8.local 2013-06-29 10-10-27.png

pcambra’s picture

Operations are there too, had a cache issue.

Summing up, with views and action modules enabled, we get the bulk operations in the comment screen which is a view. Without views enabled, we get a fallback that doesn't show the bulk operations, just lists comments with status.

Comments  drupal8.local 2013-06-29 10-16-46.png

pcambra’s picture

Status: Needs work » Needs review
FileSize
86.12 KB

Here's a fix for the actions test that checks tempstore instead of deletion, also converted the query into an entity query as for #28, but then we're losing the ability to order by the node posted in as it's in a different table. Is there any way to overcome this or is ok to drop that for the fallback?

Regarding the views + action dependency, I think it's fine to require action module for having the VBO feature enabled in the view, otherwise, we'd need to provide a form and that one won't be configurable with the actions desired, I'm not sure why the content listing took this path.

This patch will still fail in the operations part. Let's see what we agree about the bulk operations before fixing that one.

dawehner’s picture

+++ b/core/modules/comment/comment.admin.incundefined
@@ -315,3 +116,135 @@ function comment_confirm_delete_submit($form, &$form_state) {
+function comment_admin_comments() {

+++ b/core/modules/comment/config/action.action.comment_delete_action.ymlundefined
--- /dev/null
+++ b/core/modules/comment/config/views.view.comments.ymlundefined

It is just amazing how big views config is.

+++ b/core/modules/comment/config/views.view.comments.ymlundefined
@@ -0,0 +1,1487 @@
diff --git a/core/modules/comment/lib/Drupal/comment/CommentStorageController.php b/core/modules/comment/lib/Drupal/comment/CommentStorageController.php

diff --git a/core/modules/comment/lib/Drupal/comment/CommentStorageController.php b/core/modules/comment/lib/Drupal/comment/CommentStorageController.php
index 4e6ca4c..0e0b5b2 100644

index 4e6ca4c..0e0b5b2 100644
--- a/core/modules/comment/lib/Drupal/comment/CommentStorageController.php

--- a/core/modules/comment/lib/Drupal/comment/CommentStorageController.php
+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.phpundefined

+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.phpundefined
+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.phpundefined
@@ -232,4 +232,5 @@ public function getChildCids(array $comments) {

@@ -232,4 +232,5 @@ public function getChildCids(array $comments) {
       ->execute()
       ->fetchCol();
   }
+
 }

Let's skip this part of the patch.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/LinkEdit.phpundefined
@@ -37,12 +38,19 @@ public function buildOptionsForm(&$form, &$form_state) {
+  public function access() {
+    // Needs permission to administer comments in general.
+    return user_access('administer comments');
+  }

As people can replace the access controller for comments, having an access to a link has to be checked via $comment->access all the time, so we should skip the basic access() method.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentActionsTest.phpundefined
@@ -76,4 +91,23 @@ function testCommentUnpublishByKeyword() {
+    $GLOBALS['user'] = $this->admin_user;

Is this really needed after drupalLogin?

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentApprovalTest.phpundefined
@@ -11,6 +11,13 @@
 class CommentApprovalTest extends CommentTestBase {
+  /**

Let's just make an empty line here.

R.Hendel’s picture

Last patch #34 is no longer compatible with current 8.x
(warning: "Reversed (or previously applied) patch detected!")

pcambra’s picture

Status: Needs work » Needs review
FileSize
85.38 KB
3.3 KB

Addressing @dawehner comments in #36

It is just amazing how big views config is

Yes, as we have the published/unpublished thing, we need to keep several displays, we could agree in dropping the local subtabs in favor of an exposed filter for published / unpublished.

Let's skip this part of the patch.

Ok

so we should skip the basic access() method.

Dropped access method in all Link* views field plugins.

Is this really needed after drupalLogin?

Yes, as the action is executed with the user 2 (admin_user) but the tempstore is retrieved from uid 1 so that's the only way I had to access the right tempstore, it's not super nice but it's used normally in tests.

Let's just make an empty line here.

Done

As #37 mentions, re-rolled the patch against latest head.

Back to CNR, we still need to agree if action module dependency and fallback.

pcambra’s picture

Opened this followup I found while doing this #2031599: Add lock set/delete tempstore tests

tim.plunkett’s picture

Action plugins are not dependent on the action module.

The action_bulk_form plugin used here is a generic handler, but node and user each subclass \Drupal\system\Plugin\views\field\BulkFormBase, and comment should too.

Look at UserBulkForm and NodeBulkForm; they're relatively simple classes.

pcambra’s picture

Status: Needs work » Needs review
Issue tags: -Needs screenshots, -drupaldevdays
FileSize
85.66 KB
7.73 KB

Rebased to latest head and also added a CommentBulkForm as for #41

tim.plunkett’s picture

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/CommentBulkForm.phpundefined
@@ -0,0 +1,62 @@
+    if ($this->displayHandler->display['id'] == 'page_1') {
+      unset($this->actions['comment_publish_action']);
+    }
+    elseif ($this->displayHandler->display['id'] == 'page_2') {
+      unset($this->actions['comment_unpublish_action']);

This looks very brittle.

pcambra’s picture

Status: Needs work » Needs review
FileSize
41.57 KB
91.11 KB

So here's another round, the tests will still fail on the anonymous and maybe approval as I've found that any of the actions are displaying any confirmation message (i.e. "Deleted 1 comment"), but neither node actions do so I don't know if we should include them here or not.

This includes a reroll as many things have happened since #42.

As for an IRC conversation with @tim.plunkett and @dawehner, I've replaced the code in #43 by a options settings form for excluding/including the operations (i.e. not show the unpublish action for unpublished comments) but at this point the code is identical to the one in Drupal\action\Plugin\views\field\BulkForm, it might be good to move that one out of action to views as the action plugins are available even if the action module is not.
An alternative would be to make available the action form in views regardless the status of the action module.

Also fixed some updating issues related to #2011018: Reconcile entity forms and confirm forms.

pcambra’s picture

xjm’s picture

So, I am 300% in favor of converting this to a view, but I'm not sure we can get away with closing #1978904: Convert comment_admin() to a Controller as a duplicate, at least not yet. That, as part of the routing meta, is release-blocking; and Dries has stated several times that View conversions do not block release.

Maybe we can reopen the other at postponed, under the hope that this patch lands in time?

Also, this and #1907960: Helper issue for "Comment field" are going to seriously punch each other in the face. :) Might be worth considering waiting until that one is in (for either this or a rote form conversion), since that (or rather the main issue, sorry) is currently marked critical.

pcambra’s picture

Status: Needs work » Postponed

Setting status as for ##4

andypost’s picture

I think it makes no sense to mark duplicate #1978904: Convert comment_admin() to a Controller
It much easy to convert and fallback is needed anyway

andypost’s picture

andypost’s picture

pcambra’s picture

Assigned: pcambra » Unassigned
Status: Postponed » Active

I think this is not postponed anymore

dawehner’s picture

Let's better wait until https://drupal.org/comment/7894541#comment-7894541 is in, given that it conflicts in a lot of places with this patch.

Views conversions are soooo novice issues!

pcambra’s picture

Issue summary: View changes
Status: Active » Postponed
andypost’s picture

daffie’s picture

Status: Postponed » Active
jibran’s picture

Status: Needs work » Needs review

Reroll
Caution: Delete multiple action is not working. For this we should add comment list controller. View empty area is also not rendered properly I think we already have an issue for that.

jibran’s picture

Now with patch

andypost’s picture

  1. +++ b/core/modules/comment/comment.views.inc
    @@ -425,6 +425,14 @@ function comment_views_data() {
    +      'title' => t('Comment operations bulk form'),
    

    use 2 spaces for indent

  2. +++ b/core/modules/comment/config/system.action.comment_delete_action.yml
    @@ -0,0 +1,6 @@
    +id: comment_delete_action
    

    seems uuid still needed for shipped config

andypost’s picture

seems we really need list controller for fallback

tim.plunkett’s picture

seems we really need list controller for fallback

Nope. The only fallbacks needed are node and user, as decided by Dries. If they want a list, use a view.

jibran’s picture

Currently we don't have a comment.delete_multipe route in core which is required by comment delete action. Deletion of multiple comments are done using comment.admin route.
I am purposing here that if we have to add comment.delete_multipe route to fix comment delete action then why not follow the same pattern as node list controller because comment list controller is almost similar to node list controller unlike taxonomy terms listing page.

jibran’s picture

Just had a discussion with @tim.plunkett. We have to add comment.delete_multiple route to complete this patch and we can move the discussion of replacing/moving comment.admin_overview route to list controller to the follow up issue because it is related to comment module not views conversion.

jibran’s picture

Status: Needs work » Needs review
FileSize
55.76 KB
15.01 KB

Fixed #66. Comment delete is working now. Re-factor some code for delete multiple.

Fun Fact: Can someone translate uuid in comment delete action?

jibran’s picture

Fixed the comment tests. But I don't know how to fix DefaultViewsTest. It ready for review.

klonos’s picture

Status: Needs review » Needs work
dawehner’s picture

Status: Needs work » Needs review
FileSize
59.09 KB
751 bytes

Thank you for working on that issue! The bulk form would profit from #2049573: Move most or all of the ActionBulkForm class to BulkFormBase though this is certainly no blocker

I had a look at the test and realized that it is 100% bullshit. We cannot just test all default views and expect that there is never any result ... I removed the part of the test which does not make sense
though kept the bits which do: checking which display is actually executed.

olli’s picture

Should we use the "Show the empty text in the table" with this one too? That is enabled on people, content and files.

Re #76 We could just create few unpublished comments here and remove that part (or test only views' views) in a separate issue?

jibran’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
snig’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
58.88 KB

re-rolled

dawehner’s picture

This issue is a clear sign that we don't have 100% schema coverage.

willieseabrook’s picture

Issue tags: +TUNIS_2014_MARCH

Will tackle this in Sprint 29 March.

adnen’s picture

Assigned: Unassigned » adnen
Status: Needs work » Active
adnen’s picture

FileSize
1.34 MB
1.34 MB
adnen’s picture

Status: Active » Needs review
FileSize
1.34 MB
tim.plunkett’s picture

You've included what looks to be your entires /sites directory. Please don't do that :)

dawehner’s picture

@adnen
Good to see that someone continues to work on that!
I have to dissapoint you, but your fill sites directory is still smaller as the original VDC patch :P #1805996: [META] Views in Drupal Core

YesCT’s picture

Issue summary: View changes
Issue tags: +Needs issue summary update

:) https://drupal.org/contributor-tasks/create-patch has links to docs with examples and git information on creating patches that might help. And also ask anything in #drupal-contribute (https://drupal.org/irc).

xjm’s picture

Also see the git configuration instructions for how to ignore the files dir when creating patches.

vijaycs85’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
58.71 KB
1.31 KB
84.69 KB

Updated patch has:

1. Removed files dir and install specific changes.
2. Fixed an argument issue in controller

did a manual testing and all looks function at /admin/content/comment.

jibran’s picture

Status: Needs review » Needs work

Thanks for working on this. Here is some minor feedback

  1. --- /dev/null
    +++ b/core/modules/comment/config/system.action.comment_delete_action.yml
    --- /dev/null
    +++ b/core/modules/comment/config/views.view.comment.yml
    

    I think we have to move this in config/install folder.

  2. +++ b/core/modules/comment/config/system.action.comment_delete_action.yml
    @@ -0,0 +1,8 @@
    +uuid: jibran95-m4d3-th15-uu1d-f0rc0mm3n752
    

    we can remove this. and the other uuid in views.view.comment.yml

  3. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Action/DeleteComment.php
    @@ -0,0 +1,73 @@
    +  public function execute($object = NULL) {
    

    Can we change $object to $entity?

  4. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/CommentBulkForm.php
    @@ -0,0 +1,122 @@
    +      form_set_error('', t('No comments selected.'));
    

    form_set_error is @deprecated.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
61.25 KB
6.71 KB

Thanks for the quick review @jibran. Fixed all 4 review items in #98

xjm’s picture

Issue tags: +beta target
vijaycs85’s picture

Status: Needs work » Needs review
FileSize
61.18 KB
1.46 KB

This would fix few failures.

YesCT’s picture

Issue tags: +Needs screenshots
vijaycs85’s picture

@YesCT, added screenshot in #97

xjm’s picture

Priority: Normal » Major
YesCT’s picture

@vijaycs85 ah, please update the issue summary. there should be a section for UI changes. And in that section should be a before and after screenshot. :)

tkuldeep17’s picture

Status: Needs work » Needs review
FileSize
98.71 KB
tkuldeep17’s picture

Status: Needs work » Needs review
FileSize
98.9 KB

After updating latest code, patch is submitting..

dawehner’s picture

Status: Needs work » Needs review
FileSize
66.67 KB
15.69 KB

Either #109 and #111 seemed to be a completly mixed up reroll as it changes views.content.yml as well. Let me help you, this one installs without a problem.
All the fixes are required for config schema stuff.

jibran’s picture

+++ b/core/modules/comment/config/install/views.view.comment.yml
@@ -0,0 +1,1227 @@
+uuid: bf27f07e-b3e2-4460-b406-6e3610bada12

This can be removed now.

dawehner’s picture

Status: Needs work » Needs review
FileSize
66.59 KB
1.97 KB

@jibran
Good catch!

vijaycs85’s picture

+++ b/core/modules/comment/config/schema/comment.views.schema.yml
@@ -36,10 +50,10 @@ views.field.comment_link:
-      type: views_field
+      type: string

Might be my mistake setting it as views_field. but it might not be a string. By the key name 'text', it sounds it is translatable. Can we make it 'label', if it is textfield, or 'text' if it is textarea?

dawehner’s picture

FileSize
66.59 KB
502 bytes

Yeah label seems to make much more sense.

vijaycs85’s picture

Issue summary: View changes
YesCT’s picture

Issue tags: +Novice

before and after screenshots of this admin page are a novice task. tagging.

dawehner’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots, -Novice

Yeah let's apply rules everywhere instead of just open our eyes.

YesCT’s picture

So is it still true, per earlier comments, that action module needs to be enabled to get the same bulk operations as exists before?

And it looks like the count of unapproved comments isn't there in the new one.

xjm’s picture

Assigned: adnen » xjm

I'll supply the screenshots (edit: there are some in the summary) and do some manual testing. Thanks @YesCT and @dawehner.

tim.plunkett’s picture

The action entity is in the system module. See #2083649: Rename action module to action_ui for resolving that confusion.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
328.55 KB
318.43 KB
56.42 KB
49.35 KB

So, the earlier screenshots are no longer accurate. I tested the latest patch manually. The action checkboxes are available, as is the count of unapproved comments in the menu tab title.

Before

After

The one regression from HEAD is that -- because of the content relationship in the view -- we can no longer list comments on all entity types. Notice that there is no comment on my custom block entity listed in the after screenshots. This is not a feature of D7, so maybe it's okay for it to only be a view of node comments; on the other hand, moderation seems to be an intrinsic feature of comments regardless of what entity they're on. I could go either way. We could also spin that off into a followup issue.

Interestingly, simply unchecking "require this relationship" does not seem to be enough to get the comments on other entity types to show up in the view; I have to actually also remove the title field that relies on the relationship before the other comment is displayed. That feels buggy in a way that's not in scope for this issue... here's the queries.

With the content title field and "require this relationship" unchecked:

SELECT comment.subject AS comment_subject, comment.cid AS cid, comment.entity_id AS comment_entity_id, comment.entity_type AS comment_entity_type, comment.name AS comment_name, comment.uid AS comment_uid, comment.homepage AS comment_homepage, comment.changed AS comment_changed, node_comment__node_field_data.title AS node_comment__node_field_data_title, node_comment.nid AS node_comment_nid
FROM 
{comment} comment
LEFT JOIN {node} node_comment ON comment.entity_id = node_comment.nid AND comment.entity_type = 'node'
INNER JOIN {node_field_data} node_comment__node_field_data ON node_comment.nid = node_comment__node_field_data.nid
WHERE (( (comment.status <> '0') ))
ORDER BY comment_changed DESC
LIMIT 50 OFFSET 0

Same thing, but without the content title field:

SELECT comment.subject AS comment_subject, comment.cid AS cid, comment.entity_id AS comment_entity_id, comment.entity_type AS comment_entity_type, comment.name AS comment_name, comment.uid AS comment_uid, comment.homepage AS comment_homepage, comment.changed AS comment_changed, node_comment.nid AS node_comment_nid
FROM 
{comment} comment
LEFT JOIN {node} node_comment ON comment.entity_id = node_comment.nid AND comment.entity_type = 'node'
WHERE (( (comment.status <> '0') ))
ORDER BY comment_changed DESC
LIMIT 50 OFFSET 0

Setting NR to decide if the loss of other entity types' comments in the listing is an acceptable regression or not, or if it could be a followup. The texts for the actions are also different: "Publish comment" versus "Publish the selected comments". We might want a quick UX check on that. (Note that admin/content itself has it both ways: "Delete selected content" but "Publish content". (Note that these labels come from the actions themselves, not the view.)

xjm’s picture

Issue summary: View changes
FileSize
72.18 KB

Sorry, total screenshot embed fail.

dawehner’s picture

FileSize
67.24 KB
5.64 KB

Seriously good catch xjm, now we just have a views architecture problem.

INNER JOIN {node_field_data} node_comment__node_field_data ON node_comment.nid = node_comment__node_field_data.nid

As you have seen, the INNER kills it all...

Rerolled the patch.

xjm’s picture

Agreed with just dropping the INNER for {node_field_data}. Edit: what I said first made no sense, but in any case, I filed #2273849: Convert INNER joins to LEFT joins for relationships without "Require this relationship" for fixing the bug generally.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

@berdir in #2205215: {comment} and {comment_entity_statistics} only support integer entity ids regarding this issue:

I wanted to comment in that issue for a while that views should be doing the same as the current code is doing. a custom field formatter that does batch-loading of all referenced entities and displays it as a link, instead of using a relationship. We're already doing exactly that for the files view, see EntityLabel.

dawehner’s picture

Issue tags: +Needs reroll

.

@xjm
Do you still want to be assigned to this issue?

xjm’s picture

Assigned: xjm » Unassigned

Oops nope, that was just to add screenshots and test. :)

hampercm’s picture

Assigned: Unassigned » hampercm

Attempting to reroll patch from #129

hampercm’s picture

Assigned: hampercm » Unassigned

Sorry, I think this reroll is beyond my current abilities. Turned out to be much more involved than I had realized.

vijaycs85’s picture

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
65.17 KB
1.56 KB

Fixing Drupal\Core\Form\FormErrorInterface => Drupal\Core\Form\FormBuilderInterface.

tim.plunkett’s picture

+++ b/core/modules/comment/src/Plugin/views/field/CommentBulkForm.php
@@ -0,0 +1,121 @@
+      $this->formBuilder->setErrorByName('', $form_state, t('No comments selected.'));

+++ b/core/modules/system/src/Plugin/views/field/BulkForm.php
@@ -48,18 +56,21 @@ class BulkForm extends FieldPluginBase {
+    $this->formBuilder = $form_builder;

Shouldn't need the form builder at all anymore, that's a method on $form_state

vijaycs85’s picture

FileSize
63.71 KB
1.74 KB

Thanks @tim.plunkett. Updated.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
63.18 KB
538 bytes

Minor update...

larowlan’s picture

Issue tags: +Needs reroll
pcambra’s picture

Status: Needs work » Needs review
FileSize
63.19 KB

Just a plain reroll to take it from there (just one .rej file, comment.routing.yml)

pcambra’s picture

Status: Needs work » Needs review
FileSize
15.77 KB
61.61 KB

First round, updated view fields, comment bulk form field and the ConfirmDeleteMultiple.php class. There should be less failures in this one (hopefully)

diego21’s picture

Issue tags: -Needs reroll

I could apply the #151 patch, so no reroll is needed.

pcambra’s picture

Status: Needs work » Needs review
FileSize
31.01 KB
38.7 KB

Here's another take, let's hope this one is better.

I've updated the comment view to do some cleanup and to remove the translation link from the dropbutton, content view doesn't have it and I think is not working properly, but I need to look that further (probably another issue, we can add it later, I guess)

dawehner’s picture

Thank you for keeping up with this issue!

Tested the patch manually.
First thing I noticed is that we don't have added any filter ... I kinda think, given that this is some sort of advantage of views, we should leverage it here.
Maybe open up a followup?

  1. +++ b/core/modules/comment/comment.views.inc
    @@ -88,6 +88,13 @@ function comment_views_data_alter(&$data) {
    +      $data['comment']['comment_bulk_form'] = array(
    +        'title' => t('Comment operations bulk form'),
    +        'help' => t('Add a form element that lets you run operations on multiple comments.'),
    +        'field' => array(
    +          'id' => 'comment_bulk_form',
    +        ),
    +      );
    

    I kinda believe that this belongs into the CommentViewsData ...

  2. +++ b/core/modules/comment/config/schema/comment.views.schema.yml
    @@ -36,10 +40,10 @@ views.field.comment_link:
       mapping:
         text:
    -      type: views_field
    +      type: label
           label: 'Text to display'
         link_to_entity:
    -      type: views_field
    +      type: boolean
           label: 'Link field to the entity if there is no comment'
    

    Certainly a right fix!

  3. +++ b/core/modules/comment/src/Form/ConfirmDeleteMultiple.php
    @@ -77,23 +90,25 @@ public function getCancelUrl() {
    +    $this->comments = $this->tempStoreFactory->get('comment_multiple_delete_confirm')->get(\Drupal::currentUser()->id());
    
    @@ -118,11 +127,12 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      $this->tempStoreFactory->get('comment_multiple_delete_confirm')->delete(\Drupal::currentUser()->id());
    

    You should be able to use $this->currentUser() here.

  4. +++ b/core/modules/comment/src/Form/ConfirmDeleteMultiple.php
    @@ -77,23 +90,25 @@ public function getCancelUrl() {
    +      return new RedirectResponse($this->urlGenerator()
    +          ->generate('comment.admin'));
    

    This could be replaced with $this->redirect('comment.admin');

  5. +++ b/core/modules/comment/src/Plugin/Action/DeleteComment.php
    @@ -0,0 +1,73 @@
    + *   confirm_form_path = "admin/content/comment/delete"
    

    Afaik this has to be replaced with confirm_form_route_name

  6. +++ b/core/modules/views/src/Tests/DefaultViewsTest.php
    @@ -140,10 +140,6 @@ public function testDefaultViews() {
    -
    -        $count = count($view->result);
    -        $this->assertTrue($count > 0, format_string('@count results returned', array('@count' => $count)));
    -        $view->destroy();
    

    What is up with this change?

pcambra’s picture

FileSize
2.24 KB
38.65 KB

Thanks for the review Daniel!

Regarding the filters, the one straightforward is the published/non published and we've got those as tabs, maybe the bundle of the parent entity?

I kinda believe that this belongs into the CommentViewsData ...

Both user and nodes are in the _data_alter hook, maybe we can have a followup to move all of those?

Edit: both user and node bulk form view data additions happen in the data_alter, we're following the pattern atm.

You should be able to use $this->currentUser() here.
This could be replaced with $this->redirect('comment.admin');

Done.

Afaik this has to be replaced with confirm_form_route_name

Great catch! fixed.

+++ b/core/modules/views/src/Tests/DefaultViewsTest.php
@@ -140,10 +140,6 @@ public function testDefaultViews() {
-
-        $count = count($view->result);
-        $this->assertTrue($count > 0, format_string('@count results returned', array('@count' => $count)));
-        $view->destroy();

What is up with this change?

This comes from #1986606-76: Convert the comments administration screen to a view it seems :))

dawehner’s picture

Regarding the filters, the one straightforward is the published/non published and we've got those as tabs, maybe the bundle of the parent entity?

That sounds like a wonderful idea!

Both user and nodes are in the _data_alter hook, maybe we can have a followup to move all of those?

Well ... there we alter other tables, not comment tables, your hunk added stuff to 'comments'.

This comes from #1986606-76: Convert the comments administration screen to a view it seems :))

Ha ... maybe you could try to figure out whether we can remove that hunk from the patch again?

pcambra’s picture

Moved the bulk form to CommentViewsData

But I think we should add a followup then to move all alike, such as the node in node.views.inc:

/**
 * Implements hook_views_data_alter().
 */
function node_views_data_alter(&$data) {
  $data['node']['node_bulk_form'] = array(
    'title' => t('Node operations bulk form'),
    'help' => t('Add a form element that lets you run operations on multiple nodes.'),
    'field' => array(
      'id' => 'node_bulk_form',
    ),
  );
}

Also added some sample data, the test removed wasn't working because there were no unapproved comments to list.

Regarding adding the filter on Node type, we've bumped into #2273731: Filters throw unrecoverable error when using relationships and/or #2322457: Error in Views Filter Nodes based on Current user that have term reference same in nodes. which mark relationships as "complicated" in views atm. Can we do this in a followup?

dawehner’s picture

Can we do this in a followup?

Sure!

Can we add a beta evaluation, please?

pcambra’s picture

Issue summary: View changes
Issue tags: -VDC, -TUNIS_2014_MARCH
pcambra’s picture

Can we add a beta evaluation, please?

Added, feel free to improve the reasons. Not sure if anything else is required here :).

pcambra’s picture

Issue tags: +VDC

Ooops removed one tag too many

dawehner’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

+1

larowlan’s picture

Regarding adding the filter on Node type

We're limiting this to only node-based comments with a) this view and b) any follow-up. Comments can be attached to any entity type in HEAD.

  1. +++ b/core/modules/comment/src/Controller/AdminController.php
    @@ -64,4 +67,63 @@ public function adminPage(Request $request, $type = 'new') {
    +  /**
    +   * Returns an overview of the entity types a comment field is attached to.
    +   *
    +   * @param string $commented_entity_type
    +   *   The entity type to which the comment field is attached.
    +   * @param string $field_name
    +   *   The comment field for which the overview is to be displayed.
    +   *
    +   * @return array
    +   *   A renderable array containing the list of entity types and bundle
    +   *   combinations on which the comment field is in use.
    +   */
    +  public function bundleInfo($commented_entity_type, $field_name) {
    +    // Add a link to manage entity fields if the Field UI module is enabled.
    +    $field_ui_enabled = $this->moduleHandler()->moduleExists('field_ui');
    +
    +    $field_info = $this->fieldInfo->getField($commented_entity_type, $field_name);
    +
    +    $entity_type_info = $this->entityManager()->getDefinition($commented_entity_type);
    +    $entity_bundle_info = $this->entityManager()->getBundleInfo($commented_entity_type);
    +
    +    $build['usage'] = array(
    +      '#theme' => 'item_list',
    +      '#title' => String::checkPlain($entity_type_info->getLabel()),
    +      '#items' => array(),
    +    );
    +    // Loop over all of bundles to which this comment field is attached.
    +    foreach ($field_info->getBundles() as $bundle) {
    +      // Add the current instance to the list of bundles.
    +      if ($field_ui_enabled && $route_info = FieldUI::getOverviewRouteInfo($commented_entity_type, $bundle)) {
    +        // Add a link to configure the fields on the given bundle and entity
    +        // type combination.
    +        $build['usage']['#items'][] = $this->l($entity_bundle_info[$bundle]['label'], $route_info['route_name'], $route_info['route_parameters']);
    +      }
    +      else {
    +        // Field UI is disabled so fallback to a list of bundle labels
    +        // instead of links to configure fields.
    +        $build['usage']['#items'][] = String::checkPlain($entity_bundle_info[$bundle]['label']);
    +      }
    +    }
    +
    +    return $build;
    +  }
    +
    +  /**
    +   * Route title callback.
    +   *
    +   * @param string $commented_entity_type
    +   *   The entity type to which the comment field is attached.
    +   * @param string $field_name
    +   *   The comment field for which the overview is to be displayed.
    +   *
    +   * @return string
    +   *   The human readable field name.
    +   */
    +  public function bundleTitle($commented_entity_type, $field_name) {
    +    return $this->commentManager->getFieldUIPageTitle($commented_entity_type, $field_name);
    +  }
    +
    

    Is this an unintended hunk? these were removed some time ago - in particulare getFieldUIPageTitle no longer exists on comment manager

  2. +++ b/core/modules/comment/src/Form/CommentAdminOverview.php
    @@ -262,23 +275,32 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +        // Delete operation handled in \Drupal\comment\Form\ConfirmDeleteMultiple
    

    nitpick > 80

  3. +++ b/core/modules/comment/src/Form/CommentAdminOverview.php
    @@ -262,23 +275,32 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +        ->set(\Drupal::currentUser()
    

    $this->currentUser()?

  4. +++ b/core/modules/comment/src/Plugin/Action/DeleteComment.php
    @@ -0,0 +1,73 @@
    +    $this->tempStore->set(\Drupal::currentUser()->id(), $entities);
    

    We're implementing ContainerFactoryPluginInterface so we can inject this service right?

Manual review coming

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
pcambra’s picture

Status: Needs work » Needs review
FileSize
5.5 KB
36.17 KB

Thanks for the review @larowlan

nitpick > 80

$this->currentUser()?

Fixed.

Is this an unintended hunk? these were removed some time ago - in particulare getFieldUIPageTitle no longer exists on comment manager

Nice one. It would appear this is a leftover, I've removed those bits, thanks!

We're limiting this to only node-based comments with a) this view and b) any follow-up. Comments can be attached to any entity type in HEAD.

I disagree with this one, bear in mind that we're providing the default comment listings that are placed in admin/content/comments, so the same way that you wouldn't expect not-node entities listed in admin/content, the comments listing in the content section should be bound to nodes IMHO.
Otherwise you'll start to see comments there attached to entities that are not listed in admin/content, which I think is unexpected. This change if only, allows the user to change that if desired, thing that the current comment listing doesn't.

jibran’s picture

Thanks @pcambra for making it green again. Some minor points.

  1. +++ b/core/modules/comment/src/Controller/AdminController.php
    @@ -18,32 +19,29 @@
    +    $this->commentManager = $comment_manager;
    +    $this->formBuilder = $form_builder;
    

    These properties are not defined on the class.

  2. +++ b/core/modules/comment/src/Plugin/Action/DeleteComment.php
    @@ -0,0 +1,73 @@
    +    $this->tempStore->set(\Drupal::currentUser()->id(), $entities);
    

    Can we inject current user here?

  3. +++ b/core/modules/views/src/Tests/DefaultViewsTest.php
    @@ -108,6 +108,15 @@ protected function setUp() {
           entity_create('comment', $comment)->save();
    ...
    +      entity_create('comment', $comment)->save();
    

    So we are overriding $comment @var. Does it make sense?

jibran’s picture

I disagree with this one, bear in mind that we're providing the default comment listings that are placed in admin/content/comments, so the same way that you wouldn't expect not-node entities listed in admin/content, the comments listing in the content section should be bound to nodes IMHO.
Otherwise you'll start to see comments there attached to entities that are not listed in admin/content, which I think is unexpected. This change if only, allows the user to change that if desired, thing that the current comment listing doesn't.

Well currently in head we are showing all commented entities removing that here would be a regression. So I'd suggest to leave the current scenario as is and discuss this point in a follow up.

pcambra’s picture

Well currently in head we are showing all commented entities removing that here would be a regression. So I'd suggest to leave the current scenario as is and discuss this point in a follow up.

Fair enough. I'll open a batch of followups when we're RTBC again, I think it makes more sense to list just the node comments under an admin/content/comments route.

These properties are not defined on the class.

They are there (they were before, I'm just removing the unused ones here):

  /**
   * Constructs an AdminController object.
   *
   * @param \Drupal\comment\CommentManagerInterface $comment_manager
   *   The comment manager service.
   * @param \Drupal\Core\Form\FormBuilderInterface $form_builder
   *   The form builder.
   */
Can we inject current user here?

Not sure, DeleteNode.php does the same thing:

  /**
   * {@inheritdoc}
   */
  public function executeMultiple(array $entities) {
    $this->tempStore->set(\Drupal::currentUser()->id(), $entities);
  }
So we are overriding $comment @var. Does it make sense?

It doesn't really matter, but I've renamed the variable to avoid misunderstandings.

jibran’s picture

Thank you for the fixes. This is RTBC for me let's wait for @larowlan manual review :-)

larowlan’s picture

'Can we inject current user here? '
Yes ContainerFactoryPluginInterface

larowlan’s picture

Manual screenshot from HEAD - two comment fields - one on users.

larowlan’s picture

With patch

So what we're missing against HEAD (i.e. regressions) is

1) Can't perform bulk unpublish (here is screenshot from HEAD)

2) Missing the 'posted on' link/column

Also - I would expect to see some dead code removed in the patch - but there is no such deletion?

Did we decide earlier (172 comments) that 1 or 2 are acceptable in light of the 'less custom code' approach?

larowlan’s picture

Also page title on the view is 'Comment' but in HEAD it is 'Comments'

jibran’s picture

We are also missing "posted in" field.

larowlan’s picture

Sorry, said 'posted on', meant 'posted in'

pcambra’s picture

Thank you for the manual tests @larowlan!
Let's see if I didn't miss anything.

'Can we inject current user here? '
Yes ContainerFactoryPluginInterface

Added this, I guess we'll need to open a followup to make the rest of places where this happens (node, user...) use the same pattern.

Also page title on the view is 'Comment' but in HEAD it is 'Comments'

Fixed this.

1) Can't perform bulk unpublish (here is screenshot from HEAD)

Manual screenshot from HEAD - two comment fields - one on users.

Tried to add a comment field on the user entity but getting all sort of errors in the way in latest HEAD. Generating comments for nodes displays two actions: one to delete and one to unpublish in the published comments tab which becomes publish in the unpublished comments tab.

Edit, this was due to the problem fixed in #2380071: No way to add comment field to any entity. Added a comment field to users and taxonomy terms and I see both delete/publish operations for comments on not-node entities, not sure what might be happening.

2) Missing the 'posted on' link/column
Did we decide earlier (172 comments) that 1 or 2 are acceptable in light of the 'less custom code' approach?

Yes. If we deattach the comment view from the node we'd need a custom handler to display the "posted in" field. It's a matter of whether we display comments only for nodes or for all entities in admin/content/comments. If we restrict to nodes, there's no need of the extra handler but then I guess we need to fix the "fallback controller" accordingly.

Also - I would expect to see some dead code removed in the patch - but there is no such deletion?

What 'dead code' needs to be removed that it isn't?

larowlan’s picture

What 'dead code' needs to be removed that it isn't?

I was expecting a lot of CommentAdminOverview would go, if not all of it - I'm not seeing that in the patch - could be I'm mistaken - but I thought the view would replace that - or does it remain and views enhances it/the listing?

Yes. If we deattach the comment view from the node we'd need a custom handler to display the "posted in" field.

How much work is involved in that - and would it be performant? In HEAD we collect the entity IDs into entity type groups and then use a single call to entity_load_multiple per entity-type - is that kind of thing possible with views?

Might be a question for @dawehner and @jibran from the VDC team.

dawehner’s picture

I was expecting a lot of CommentAdminOverview would go, if not all of it - I'm not seeing that in the patch - could be I'm mistaken - but I thought the view would replace that - or does it remain and views enhances it/the listing?

Much like the other conversions (admin/content, admin/people) we realized that some sort of admin listing is a central functionality which is part of the corresponding module. We can't just drop the basic functionality. At the same time, we should add the flexibility.

How much work is involved in that - and would it be performant? In HEAD we collect the entity IDs into entity type groups and then use a single call to entity_load_multiple per entity-type - is that kind of thing possible with views?

Absolutely, you could write a handler which does that. We have a method (::preRender()) which retrieves all results so you can act based upon it.

andypost’s picture

The related issue should postpone this. OTOH we have no consistent method to determine is an entity use "changed" field, because EntityChangedInterface does not tell about a field for that.
Also statistics could be disabled so no idea how to sort comments properly

pcambra’s picture

dawehner’s picture

The related issue should postpone this. OTOH we have no consistent method to determine is an entity use "changed" field, because EntityChangedInterface does not tell about a field for that.
Also statistics could be disabled so no idea how to sort comments properly

Well, I would argue that the patch for itself is some sort of progress, so we don't have to wait onto another one, maybe people disagree though.

jibran’s picture

jibran’s picture

Status: Needs work » Needs review
FileSize
788 bytes
35.39 KB

ok

jibran’s picture

FileSize
777 bytes
35.27 KB

minor clean up.

andypost’s picture

last interdiff shows that there's no test coverage for access
also I wondered a way to pass comments to confirm submit via tempstore that affected by race condition like #2421263: Potential data loss: concurrent (i.e. by different users) node edits leak through preview, suppose better to pass ids via form_state somehow

jibran queued 188: 1986606-188.patch for re-testing.

jibran’s picture

FileSize
34.19 KB

Reroll.

kim.pepper’s picture

Status: Needs work » Needs review

Kicking the bot.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
34.47 KB
27.63 KB

Fixes install fatals. Not sure why they weren't happening in #192 ?

  • Moves view install config to optional
  • Updates user tempstore to private temp store. (addresses comments in #189
jibran’s picture

Assigned: Unassigned » jibran

I'm going to work on fails and #179.2.

jibran’s picture

I'm going to work on fails and #179.2.

jibran’s picture

Status: Needs work » Needs review
FileSize
1.96 KB
34.88 KB

I hope this will fix the fails.

jibran’s picture

Status: Needs work » Needs review
FileSize
15.18 KB
42.74 KB

I have wasted so much time on these config fails I should have created a new view :P
The only remaining thing is "posted on" col. Still working on this.

jibran’s picture

@larowlan & @andypost is it intentional that we don't have a comment list builder handler?

jibran’s picture

Assigned: jibran » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
8.64 KB
48.92 KB
60.55 KB
48.17 KB
65.29 KB
66.35 KB

Added new screenshots also added posted in column.

Before Empty

After Empty

Before With Comment

After With Comment

jibran’s picture

The Query

SELECT comment_field_data.changed AS comment_field_data_changed, comment_field_data.cid AS cid
FROM 
{comment_field_data} comment_field_data
WHERE (( (comment_field_data.status <> '0') ))
ORDER BY comment_field_data_changed DESC
LIMIT 50 OFFSET 0
larowlan’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
208.26 KB
+++ b/core/modules/comment/src/Plugin/views/field/CommentedEntityLabel.php
@@ -0,0 +1,135 @@
+      return $this->sanitizeValue($entity->label());

I think we need to check access here. If $entity->access('view') then show the label, otherwise show Redacted or similar.
Other than that, can't fault it.

Here's a screenshot, on the top you can see admin has unpublished the node.
On the bottom is a user with 'administer comments', 'access admin theme' and 'access admin pages' but not 'administer nodes' or 'view unpublished nodes'.

They shouldn't see the node title in the comment admin screen.

larowlan’s picture

Issue tags: +Needs tests

Sorry, given this passed but has access bypass - means we need tests :(

jibran’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.86 KB
49.73 KB

Tests are showing me some different story.

dawehner’s picture

  1. +++ b/core/modules/comment/src/Form/CommentAdminOverview.php
    @@ -262,23 +275,33 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +          $comment = $this->commentStorage->load($cid);
    ...
    +          $comment->save();
    ...
    +          $comment = $this->commentStorage->load($cid);
    +          $comment->setPublished(TRUE);
    +          $comment->save();
    +        }
    

    Do you think its worth to try to merge those calls, so they aren't duplicated?

  2. +++ b/core/modules/comment/src/Plugin/views/field/CommentedEntityLabel.php
    @@ -0,0 +1,135 @@
    +/**
    + * Views field display label of commented entity optionally linked to entity.
    + *
    + * @ViewsField("commented_entity_label")
    + */
    +class CommentedEntityLabel extends FieldPluginBase {
    +
    

    All that doesn't make sense! We should be able to just use the ordinary string formatter, which already has a link to entity option ... don't we?

jibran’s picture

FileSize
1.86 KB
49.64 KB

Do you think its worth to try to merge those calls, so they aren't duplicated?

Fixed that. We can further simplify

        if ($operation == 'unpublish') {
          $comment->setPublished(FALSE);
        }
        elseif ($operation == 'publish') {
          $comment->setPublished(TRUE);
        }

to

$comment->setPublished($operation == 'publish');

If the operation is not delete it'll either be publish or unpublish but contib can always add more operations so let's not make this FormSubmit a nightmare for contrib by oversimplifying things :)

All that doesn't make sense! We should be able to just use the ordinary string formatter, which already has a link to entity option ... don't we?

CommentedEntityLabel is a direct copy of EntityLabel without the query() and simplified render() & preRender(). I did0 extend CommentedEntityLabel form EntityLabel first but using FieldPluginBase is much simpler an more cleaner.

dawehner’s picture

CommentedEntityLabel is a direct copy of EntityLabel without the query() and simplified render() & preRender(). I did0 extend CommentedEntityLabel form EntityLabel first but using FieldPluginBase is much simpler an more cleaner.

But well, do we really need it?

jibran’s picture

But well, do we really need it?

How else we can show posted in column? This is what you suggested in #179.2 Absolutely, you could write a handler which does that. We have a method (::preRender()) which retrieves all results so you can act based upon it.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Test looks good, can't find anything else to pick at.
Thanks @jibran, @pcambra, @dawehner and @vijaycs85 for the hard slog on this.

miro_dietiker’s picture

Status: Reviewed & tested by the community » Needs work

Sorry for nitpicking...

+++ b/core/modules/comment/config/optional/views.view.comment.yml
@@ -0,0 +1,764 @@
+          text: edit
...
+          text: delete

edit and delete are both capitalised in Content overview: Edit and Delete.

For other entity overviews we have introduced hook_entity_operations_alter(). Now it is still not supported. Does that mean that all lists that are switched to a view only allow alterations of the operations by changing the view configuration?

jibran’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.62 KB
49.94 KB

Fixed #215. Thank you for the nice catch.

miro_dietiker’s picture

Thx for fixing. Looking good indeed.
I'm just a little confused there is no test coverage at all depending on "Delete" and "Edit" link on the overview..

Then i guess, the related issue about comment hook_entity_operations_alter() is rejected?

miro_dietiker’s picture

In this comment issue, each operation is added in configuration, hidden, with a Dropbutton. Thus hook_entity_operations_alter() does not work.

In the content overview, entity operations are added through one dedicated operations field.

Fine, if we look into this as a followup, but there need to be consistency in how we maintain operations...

jibran’s picture

Have you seen #202? This problem is not introduced here. It is inherited from D7/D6 and we never corrected it. This issue always came up. We don't have a sperate link for deleting the comment like node has node/nid/delete there is nothing for the comment like this when we converted comment paths to route this question came up but that was consider out of scope because priority was route conversion at that time. When we introduced list builder for entities we ignore comments because we have to isolate between admin page and delete page which we never did. Operations links can only exist if we have a list builder and for comments and that is why we have to use dropbutton here. See #2491875-14: EntityViewsData adds Operations links to all entities, which won't work if the entity type has no list builder, leading to WSOD on some views as well. Thank you for adding the related issue.

Arla’s picture

Corrected "Opertaions" typo. Keeping the RTBC status because this is so trivial. No need for commit attribution.

Berdir’s picture

Hm. Not sure I buy the argument about those operation links :)

* entity.comment.delete_form / comment/{comment}/delete exists, not sure why you think it doesn't? How else would it work? The normal delete link also points to the same?
* That comment currently doesn't have a list builder doesn't mean it shouldn't have one. You can just add the default, it won't be accessible directly in any way unless you have link template/route for it. I've been doing that in quite a few contrib projects.
* Pretty sure the primary reason this is using link fields + dropbutton is because this patch was started long before the other admin views were changed to use the new operations field. Not using it seem seems inconsistent?

Unlike other things, "getting a basic version in and then improve on it" doesn't work so well for views/default configuration because updating it requires update functions or you're stuck with existing sites on an old version of the view. So it might be now or never for 8.x ;)

damiankloip’s picture

Berdir is totally correct. When this issue started we only had the dropbutton handler that would reference other link based handlers. Once we added the operations handler, we should be using that. The other admin views do. This gives us consistency through out the operations for an entity.

As well as other modules being able to add their own, like devel, for example. If we keep using the individual links, we lose that. You would have to add a new link handler to the dropbutton each time....

jibran’s picture

Thanks @alexpott for retesting it. Thank you @Berdir and @damiankloip for clearing this up. I'm +1 for both #223 and #224 but I have some concerns.

entity.comment.delete_form / comment/{comment}/delete exists, not sure why you think it doesn't? How else would it work? The normal delete link also points to the same?

Ok that's my bad I should have done proper homework before replying.

* That comment currently doesn't have a list builder doesn't mean it shouldn't have one. You can just add the default, it won't be accessible directly in any way unless you have link template/route for it. I've been doing that in quite a few contrib projects.

This seems out of scope here. Should I create an issue and postpone this on that?

* Pretty sure the primary reason this is using link fields + dropbutton is because this patch was started long before the other admin views were changed to use the new operations field. Not using it seem seems inconsistent?

Ok right now I can't use operations links because there is no route template. If I'll create a separate issue for that can we make it pass beta eval?

Unlike other things, "getting a basic version in and then improve on it" doesn't work so well for views/default configuration because updating it requires update functions or you're stuck with existing sites on an old version of the view. So it might be now or never for 8.x ;)

One can always delete the existing view and re-import new one

Berdir’s picture

This seems out of scope here. Should I create an issue and postpone this on that?

It's just one line, I don't think we need a separate issue for that?

Ok right now I can't use operations links because there is no route template. If I'll create a separate issue for that can we make it pass beta eval?

Huh?

Comment.php:

*   links = {
 *     "canonical" = "/comment/{comment}",
 *     "delete-form" = "/comment/{comment}/delete",
 *     "edit-form" = "/comment/{comment}/edit",
 *   },

Everything that you need is here?

jibran’s picture

/me shuts up now and fixes the patch. :D

Thanks once again for pointing that out.

jibran’s picture

Reroll and replaced dropdown with operations links.

diff --git a/core/modules/comment/src/Entity/Comment.php b/core/modules/comment/src/Entity/Comment.php
index bd3fbfc..5525765 100644
--- a/core/modules/comment/src/Entity/Comment.php
+++ b/core/modules/comment/src/Entity/Comment.php
@@ -35,6 +35,7 @@
  *       "default" = "Drupal\comment\CommentForm",
  *       "delete" = "Drupal\comment\Form\DeleteForm"
  *     },
+ *     "list_builder" = "Drupal\Core\Entity\EntityListBuilder",
  *     "translation" = "Drupal\comment\CommentTranslationHandler"
  *   },
  *   base_table = "comment",

As it turns out links were already there I just have to add list builder.

Is it a good idea to create a follow up to replace \Drupal\comment\Form\CommentAdminOverview with \Drupal\comment\CommentListBuilder? Although we are using CommentAdminOverview for two links /admin/content/comment and /admin/content/comment/approval.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -beta target

Great interdiff!

Is it a good idea to create a follow up to replace \Drupal\comment\Form\CommentAdminOverview with \Drupal\comment\CommentListBuilder? Although we are using CommentAdminOverview for two links /admin/content/comment and /admin/content/comment/approval.

Feel free to try that out. I think using entity list builders for content entities is a bad idea, but this is just my personal opinion ...

miro_dietiker’s picture

Wow! Awesome to see this happen!

olli’s picture

Status: Reviewed & tested by the community » Needs review

@jibran++

  1. +++ b/core/modules/comment/comment.routing.yml
    @@ -2,7 +2,7 @@ comment.admin:
    -    _controller: '\Drupal\comment\Controller\AdminController::adminPage'
    +    _form: 'Drupal\comment\Form\CommentAdminOverview'
    
    @@ -11,7 +11,7 @@ comment.admin_approval:
    -    _controller: '\Drupal\comment\Controller\AdminController::adminPage'
    +    _form: 'Drupal\comment\Form\CommentAdminOverview'
    
    +++ b/core/modules/comment/src/Form/CommentAdminOverview.php
    @@ -262,23 +275,31 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    -      // Delete operation handled in \Drupal\comment\Form\ConfirmDeleteMultiple
    -      // see \Drupal\comment\Controller\AdminController::adminPage().
    ...
    +    if ($operation != 'delete') {
    +      foreach ($comments as $comment) {
    +        // Delete operation handled in
    +        // \Drupal\comment\Form\ConfirmDeleteMultiple
    +        // @see \Drupal\comment\Controller\AdminController::adminPage().
    

    Can we remove AdminController now?

  2. +++ b/core/modules/comment/config/optional/views.view.comment.yml
    @@ -0,0 +1,665 @@
    +    position: 1
    

    position: 0

  3. +++ b/core/modules/comment/config/optional/views.view.comment.yml
    @@ -0,0 +1,665 @@
    +            edit_comment:
    +              sortable: false
    +              default_sort_order: asc
    +              align: ''
    +              separator: ''
    +              empty_column: false
    +              responsive: ''
    +            delete_comment:
    +              sortable: false
    +              default_sort_order: asc
    +              align: ''
    +              separator: ''
    +              empty_column: false
    +              responsive: ''
    +            dropbutton:
    +              sortable: false
    +              default_sort_order: asc
    +              align: ''
    +              separator: ''
    +              empty_column: false
    +              responsive: ''
    

    Can we replace these with operations and commented_entity_label?

  4. +++ b/core/modules/comment/config/optional/views.view.comment.yml
    @@ -0,0 +1,665 @@
    +      filter_groups:
    +        operator: AND
    +        groups:
    +          1: AND
    

    Not sure where this is coming from..

  5. +++ b/core/modules/comment/src/Form/CommentAdminOverview.php
    @@ -10,12 +10,14 @@
    +use Drupal\Core\Cache\Cache;
    
    @@ -262,23 +275,31 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +      Cache::invalidateTags(['content']);
    

    I think this is not needed anymore?

  6. +++ b/core/modules/comment/src/Plugin/views/field/CommentedEntityLabel.php
    @@ -0,0 +1,135 @@
    +    if(($comment = $values->_entity) && isset($this->loadedCommentedEntities[$comment->getCommentedEntityTypeId()]) && isset($this->loadedCommentedEntities[$comment->getCommentedEntityTypeId()][$comment->getCommentedEntityId()])) {
    ...
    +      if ($entity = $value->_entity) {
    

    Should we use $this->getEntity($values) that also works with relationships?

jibran’s picture

FileSize
33.96 KB
66.41 KB

Awesome review @olli thank you very much.

  1. Fixed all the points of #231.
  2. Added unittest for CommentBulkForm and added WebTest for comment admin view.
  3. Found #206 and fixed that. Tests++
  4. Found that link to entity uses revision link template but we need paramlink for comment. Fixed that and added a workaround for it. Needs proper fix and feedback from @dawehner or @plach. Tests++
  5. admin/content/comment/approval was missing unpublish action added that as well. Tests++
  6. Found empty selected message before the patch/with disabled view is 'Select one or more comments to perform the update on.' and after patch/with enabled view is 'No comments selected.'. I haven't changed that yet but we have to choose one. Test++
  7. Minor clean up in comment.routing.yml.
  8. Minor fixed in DeleteComment.
  9. Reverted changes in NodeBulkFormTest and UserBulkFormTest as they are not needed.
jibran’s picture

Needs some feedback.

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
    @@ -158,4 +158,21 @@ protected function viewValue(FieldItemInterface $item) {
    +    if (class_exists('Drupal\comment\Entity\Comment') && $entity instanceof \Drupal\comment\CommentInterface) {
    

    Is this a proper way or do we have to create a new formatter for this?

  2. +++ b/core/modules/comment/src/Tests/Views/CommentAdminTest.php
    @@ -0,0 +1,129 @@
    +    // Make sure the label of unpublished node is not visible on listing page.
    +    $this->drupalGet('admin/content/comment');
    +    $this->postComment($this->node, $this->randomMachineName());
    +    $this->drupalLogout();
    +    $this->commentAdminUser = $this->drupalCreateUser(array(
    +      'administer comments',
    +    ));
    +    $this->drupalLogin($this->commentAdminUser);
    +    $this->drupalGet('admin/content/comment');
    +    $this->assertText(SafeMarkup::checkPlain($this->node->label()), 'Comment admin can see title of publish node');
    +    $this->node->setPublished(FALSE)->save();
    +    $this->assertFalse($this->node->isPublished(), 'Node is unpublished now.');
    +    drupal_flush_all_caches();
    +    $this->drupalGet('admin/content/comment');
    +    $this->assertNoText(SafeMarkup::checkPlain($this->node->label()), 'Comment admin cannot see title of unpublish node');
    

    dfac call seems wrong here. Is there any other way to do this?

dawehner’s picture

Is this a proper way or do we have to create a new formatter for this?

I'd argue its not. Better create an entire new field formatter for the usecase of comments, maybe called "Comment permalink" and make it just available for comments.

andypost’s picture

Berdir’s picture

2) dfac() seems definitely wrong. We need to get the node cache tag in that cached views table row so that when a node is changed, that row needs to be invalidated. This is a new field plugin right, so I'm pretty sure that can do that..

+++ b/core/modules/comment/src/Plugin/views/field/CommentedEntityLabel.php
@@ -0,0 +1,135 @@
+  public function render(ResultRow $values) {
+    /** @var \Drupal\comment\CommentInterface $comment */
+    if(($comment = $this->getEntity($values)) && isset($this->loadedCommentedEntities[$comment->getCommentedEntityTypeId()]) && isset($this->loadedCommentedEntities[$comment->getCommentedEntityTypeId()][$comment->getCommentedEntityId()])) {
+
+      /** @var $entity \Drupal\Core\Entity\EntityInterface */
+      $entity = $this->loadedCommentedEntities[$comment->getCommentedEntityTypeId()][$comment->getCommentedEntityId()];
+
+      if (!empty($this->options['link_to_entity'])) {
+        $this->options['alter']['make_link'] = TRUE;
+        $this->options['alter']['url'] = $entity->urlInfo();
+      }
+      if ($entity->access('view')) {
+        return $this->sanitizeValue($entity->label());

Here you need to return a render array that contains the cache tags for $entity And also the access cache metadata I think, so that two different users will get a different cache.

jibran’s picture

Status: Needs work » Needs review
FileSize
5.05 KB
1.29 KB
68.68 KB

Thank you @dawehner, @andypost and @Berdir for the help, suggestions and replies.

  • #235 added a new formatter.
  • #236 if parmalink issues got resolved before this patch then I'll reroll the issue. :)
  • #237 There is a problem with CommentedEntityLabel we are not using field formatter it's an old style views field plugin. We have to re-write it any way. Given that we have to support #178.2
    How much work is involved in that - and would it be performant? In HEAD we collect the entity IDs into entity type groups and then use a single call to entity_load_multiple per entity-type - is that kind of thing possible with views?

    & #179.2

    Absolutely, you could write a handler which does that. We have a method (::preRender()) which retrieves all results so you can act based upon it.

    we have to come up with new plan.

@berdir also told me on IRC

jibran: also, based on what I've commented, it might be a very good idea to have test coverage for users with different permission. For example, for that existing test, just make sure that an admin who can view unpublished nodes is still able to see the node title

I'll add it.

Meanwhile

  1. Updated empty selected message. I kept the old one because it's the same message we have since D7.
  2. Added CommentPermalinkFormatter for #235 and updated the view.
jibran’s picture

Status: Needs work » Needs review
FileSize
590 bytes
68.76 KB

Fixed schema fails.

dawehner’s picture

  1. +++ b/core/modules/comment/src/Plugin/views/field/CommentedEntityLabel.php
    @@ -0,0 +1,135 @@
    +class CommentedEntityLabel extends FieldPluginBase {
    

    This class looks perfect now!

  2. +++ b/core/modules/comment/src/Plugin/views/field/CommentedEntityLabel.php
    @@ -0,0 +1,135 @@
    +      return $this->sanitizeValue($entity->label());
    

    Yeah I don't think we have to still manually sanitize the value, due to autoescaping ... ?

jibran’s picture

FileSize
0 bytes
66.59 KB

This fixed everything mention in #238. After the current changes #241 is not applicable anymore. As it turns out entity_id is a base field of the comment entity so we don't need an extra views field plugin. I added one to fix preRender issue.
Does this mean that we should load all the entities for all ER field views? OR preRender is redundant here?
Other then that this is ready.

jibran’s picture

FileSize
12.22 KB

jibran’s picture

Status: Needs work » Needs review
FileSize
4.62 KB
66.76 KB

With the test fix. This needs multilingual approval now.

jibran’s picture

FileSize
19 KB
25.66 KB

Fixed the doc block as per @andypost's review.

andypost’s picture

+++ b/src/Tests/Views/DynamicEntityReferenceRelationshipTest.php
@@ -117,35 +162,157 @@ class DynamicEntityReferenceRelationshipTest extends ViewUnitTestBase {
+//    // Check an actual test view.
+//    $view = Views::getView('test_dynamic_entity_reference_view');
+//    $this->executeView($view);
...
+//    // Check an actual test view.
+//    $view = Views::getView('test_dynamic_entity_reference_view');
+//    $this->executeView($view);

why this hunks are commented out?

jibran’s picture

FileSize
612 bytes
66.79 KB

Sorry wrong patch and interdiff. Please ignore #247.

plach’s picture

@246:

If I'm not mistaken, that's ignoring translations and acting on comments as a whole, right? Won't that cause similar issues to the ones fixed in #2484037: Make Views bulk operations entity translation aware?

andypost’s picture

just 5c here: historically we added language column to this table in d7 the idea behide was that comment will always have one language inherited from node

jibran’s picture

@plach
There two reason I ignored that.
1. There is no language filter on comment admin page so it doesn't make sense to key it by language.
2. I haven't seen a practical example where we have to translate the comment. It's true that comment can have different languages but unlike nodes or terms we are not going to translate the comments or its fields.

Can you please have a look at views commented entity field plugin as well please? You have worked on views row caching and your are also familiar with ER system. PreRender for commented entity makes sense for me.

@andypost thanks for the clarity. I think inflation has increased the cost of the suggestion from 2¢ to 5¢ :D

jibran’s picture

FileSize
1.3 KB
66.91 KB
  • Reroll.
  • Added caching to the view.
  • Added admin category to the view.

Still waiting for a response on #252.

plach’s picture

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
    @@ -158,4 +158,18 @@ protected function viewValue(FieldItemInterface $item) {
    +    // For the default revision this falls back to 'canonical'
    

    Missing trailing dot.

  2. +++ b/core/modules/comment/src/Plugin/views/field/CommentedEntity.php
    @@ -0,0 +1,45 @@
    +  public function preRender(&$values) {
    +    parent::preRender($values);
    +
    +    $entity_ids_per_type = array();
    +    foreach ($values as $value) {
    +      /** @var \Drupal\comment\CommentInterface $comment */
    +      if ($comment = $this->getEntity($value)) {
    +        $entity_ids_per_type[$comment->getCommentedEntityTypeId()][] = $comment->getCommentedEntityId();
    +      }
    +    }
    +
    +    foreach ($entity_ids_per_type as $type => $ids) {
    +      $this->loadedCommentedEntities[$type] = $this->entityManager->getStorage($type)->loadMultiple($ids);
    +    }
    +  }
    

    Actually this code will always run if display cache is disabled, even if row cache is available. You may want to try and lazily execute it in ::getItems() instead. See Drupal\views\Plugin\views\field\Field::getItems().

jibran’s picture

Reroll

Thanks @plach for the review.

  1. Fixed.
  2. If we are going to do that in getItems(), which runs on rows level, then it doesn't solve our problem so we are better off not doing that at all.
jibran’s picture

Interdiff in 255 was right patch was wrong. Re-uploading the patch and minor fix.

jibran’s picture

Status: Needs work » Needs review
FileSize
4.26 KB
69.26 KB

Fixed the schema and added update path and update path test.

jibran’s picture

Status: Needs work » Needs review
Fatal error: Cannot use Drupal\comment\Tests\CommentTestBase as CommentTestBase because the name is already in use in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/comment/src/Tests/Views/CommentAdminTest.php on line 11
Drupal\node\Tests\Config\NodeImportChangeTest                  3 passes
FATAL Drupal\migrate_drupal\Tests\d6\MigrateDrupal6Test: test runner returned a non-zero error code (255).

This doesn't make sense and it passes on ci so retesting.

jibran’s picture

Status: Needs work » Needs review
FileSize
786 bytes
69.28 KB

This should be green.

jibran’s picture

I talked to @plach and @dawehner about this and they showed me how to use getItems properly. Here is the patch doing that.

larowlan’s picture

Assigned: Unassigned » plach

Looks good to me, would like one last look from @plach though

larowlan’s picture

plach: for your reference

jibran
larowlan: so in D8 we can have two nodes with same nid when translation is on. Right?
10:46
larowlan
jibran: I honestly don't know
jibran: ignorance is bliss
10:47
jibran
larowlan: The answer is yeas
10:47
jibran
ye*s
yes*
10:48larowlan	remembers that
10:50
jibran
larowlan: the base table only store nid, vid, uuid, type and langcode. see `describe node;`
10:50
larowlan
jibran: ok
10:50
jibran
larowlan: which mean when we create a untranlated node it stores a row
larowlan: but when we translate a node it creates a new row in node_field_data.
larowlan: now if you `describe node_field_data;` you can see nid and langcode are primary keys.
larowlan: which means the combination of both should be unique.
larowlan: i.e 1_en, 1_it, 1_fr is possible
larowlan: hence when we want to delete the node using vbo we have have a langcode with nid so that we should know that we are deleting the correct node.
10:57
larowlan
jibran: makes sense, thanks
10:58
jibran
larowlan: if we delete an original node then all the translation should be deleted as well.
10:58
larowlan
jibran: agree
10:59
jibran
larowlan: and we can always delete the translation without deleting original.
larowlan: so let me ask this in D8 can we have two comments with same cid when translation is on?
11:01
larowlan
jibran: you can only comment on one node, but you can comment in any language
jibran: ie comments don't target a translation of the host entity
jibran: but you can translate a comment
jibran: so in that regard it would be the same as nodes I think
jibran: but because we don't have language filter at admin page, it would always remove all comments?
jibran: but that would be consistent with HEAD
11:03
jibran
larowlan: so andypost said in #251 "historically we added language column to this table in d7 the idea behide was that comment will always have one language inherited from node"
larowlan: so comments inherits node language by default.
larowlan: and yes it is true that we can translate comments but that would be insane imo
larowlan: why would one want to do that?
larowlan: but it is Drupal. if you can do that then you have to have that case covered.
11:09
larowlan
jibran: no idea, when 731724 went in people said comments on comments were insane, and since then valid use cases have emerged
11:09
jibran
larowlan: so if we edit the admin view and add lang filter to it and delete the comment then there would be a case of data loss.
11:09
jibran
just like https://www.drupal.org/node/2484037
11:09
Druplicon
https://www.drupal.org/node/2484037 => Make Views bulk operations entity translation aware #2484037: Make Views bulk operations entity translation aware => 89 comments, 21 IRC mentions
11:10
larowlan
jibran: ok, I'm kind of following
jibran: so whats the solution? change the delete plugin?
jibran: add the column now?
11:10
jibran
larowlan: so seems like we are agreeing on a data loss just like nodes for comments.
larowlan: so what we can do is add a hidden field like node admin view to the comment admin view and revert comment bulk delete action.
larowlan: so that we can avoid a data loss.
11:12
jibran
hidden field  = hidden lang field
11:15
jibran
larowlan: but first we have to test the above case.
11:15
larowlan
jibran: ok
jibran: can this go in as a follow-up? or does it need to happen before?
11:16
jibran
larowlan: so that we can right proper tests for it.
larowlan: only plach can answer that question
larowlan: imo there will be a data loss but that would be same as HEAD.
larowlan: data loss means it has to happen now. Same in HEAD mean could go to follow up.
larowlan: so I'm not sure.
11:18
larowlan
jibran: fairly sure it will be same as in HEAD
jibran: but that'd be a critical on its own right?
jibran: lets wait for plach
11:20
jibran
larowlan: for plach to decide he has to have a complete picture of it. I kind a rejected his idea in #252
larowlan: so would you like to sum it up in the issue :)
11:21
larowlan
jibran: sure
jibran: can I post this convo?
11:22
jibran
larowlan: and I'm not sure that interdiff of #265 gets executed that is why I wanted you to test it before and after so that we can have a clear picture.
larowlan: sure.
larowlan’s picture

Also @jibran pointed out that the interdiff at #265 doesn't appear to execute, so asked if I could triple check that in a debugger, will do (please assign back to me @plach)

plach’s picture

Assigned: plach » larowlan

Surely translating comments is hardly as common as translating nodes, however I actually worked on a site with such a need (they were using machine translation for them). If this is committed as-is, we will get the same problems we had in #2484037: Make Views bulk operations entity translation aware, although on a fairly smaller scale. I'm afraid this could be a good argument to reject the patch, because at this stage this kind of issues need to be complete to be accepted.

I'd be fine with multiple deletion being addressed in a major follow-up, I wouldn't be fine if it were considered critical due to potential data-loss, although under very rare circumstances.

OTOH the bulk form handler is already coded to support multilingual entities and the node DeleteMultiple action + tests can serve as a good model to quickly put together the same functionality here. Ideally we should have a common delete multiple entity action, but that would require a bit more work.

jibran’s picture

Status: Needs review » Needs work

I tested this last night CommentedEntity::getItems() loads all the commented entities once on each page load so it is getting executed. But it is rendering same value for each commented entity for all rows so I have to work on that.

jibran’s picture

Status: Needs work » Needs review
FileSize
2.02 KB
70.58 KB

Reroll.

dobe’s picture

I had been working on a very similar patch as for some reason this queue was hard for me to find (using "admin" in search was key). I kept searching "drupal 8 comment views" instead of "drupal 8 comment admin views". Here is a reroll of the patch. Some other things changed in the StringFormatter file. Should work against latest dev. Don't wanna step on any of the toes of the owners of this queue mainly just trying to get familiar with the backend of d8.

xjm’s picture

Just discovered that this never landed. :( Sad kitties.

Also seems to now be blocked by this bug, which also affects comments: #2587551: Selecting Operation Links for File Type View throws an Exception

xjm’s picture

Issue tags: +rc target

@alexpott and I agreed that this is worth making an RC target.

miro_dietiker’s picture

Is @larowlan working on the remaining items?
Otherwise please state what are the missing steps so someone of our team can complete the work based on the agreed approach.. I lost track after #229 when it was RTBC.

larowlan’s picture

Assigned: larowlan » Unassigned

We need to address #269 (can't do it in a follow-up now - has to go in with this).

So that is this

OTOH the bulk form handler is already coded to support multilingual entities and the node DeleteMultiple action + tests can serve as a good model to quickly put together the same functionality here. Ideally we should have a common delete multiple entity action, but that would require a bit more work.

xjm’s picture

I separated #2604618: Views operations dropbuttons do not work with Comment because it does not specify a list builder out into its own patch since it is a separate bug. Technically this issue is dependent on that one, but since it's just one line I think work can proceed here as well.

xjm’s picture

We could also possibly proceed by adding the new comment delete action and the permalink formatter in their own issues to make this issue's patch more manageable.

Arla’s picture

Status: Needs work » Needs review
FileSize
70.95 KB

Rerolled the patch in #273. Still needs work (#279) but I'm switching to Needs review to check tests.

Arla’s picture

The view config had to be updated to match changed schema.

mbovan’s picture

Fixed failing CommentAdminTest.

mbovan’s picture

Implemented abstract setDatabaseDumpFiles method in CommentAdminViewUpdateTest class.

jibran’s picture

Forgot to add the test file.

jibran’s picture

Status: Needs work » Needs review
FileSize
14.74 KB
72.7 KB

Converted BTB to webtest. Fixed the config schema fail.

jibran’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
73.59 KB

With fetal fixes.

mbovan’s picture

Fixing tests, docs and some code style issues.

jibran’s picture

Thanks @mbovan for the fixes.

+++ b/core/modules/comment/src/Tests/Views/CommentAdminTest.php
@@ -0,0 +1,166 @@
+  public function testCommentedEntityLabel() {

This test should fail unless I'm doing something wrong. Can someone manually test this case. Meanwhile let me try something else.

xjm’s picture

Reuploading @jibran's patch with a patch extension for the testrunner.

andypost’s picture

Status: Needs work » Needs review
FileSize
7.56 KB
74.95 KB

List builder and comment_update_8001() already added in #2604618: Views operations dropbuttons do not work with Comment because it does not specify a list builder
Also fixed code style and usage of deprecated entity manager

Still broken views test, looks because of loading of commented entities

jibran’s picture

Good #300 failed so I was not wrong. Now we need to debug that test I suspect it is related to cache tags. Maybe @dawehner can have a look at it. Other then that fail I think this patch is ready to go.

@andypost why do we need to import comment delete action? Isn't it imported when view get enabled by update? Anyway if we have to import it anyway then the action should be imported before view because view is using that action.

andypost’s picture

Status: Needs work » Needs review
FileSize
8.26 KB
73.1 KB

@jibran thanx! nice catch.

Removed out of scope changes in \Drupal\Core\Field\Plugin\Field\FieldFormatter\StringFormatter
Added test for import of action.
Reverted changes in \Drupal\comment\Tests\CommentTestBase because we can use this for users so todo for #1972242: User compact view mode is not configured in minimal
So for user we need to create comment manually
Removed debug introduced in previous patch

Suppose RTBC

jibran’s picture

+++ b/core/modules/comment/src/Tests/Views/CommentAdminTest.php
@@ -147,11 +148,30 @@ public function testApprovalAdminInterface() {
+    $user_comment = Comment::create([

This will not reproduce the same situation we have to post comment using ui. I think it will not fail.

xjm’s picture

I tested the latest patch manually with a few comments and users. The author name disappears when a comment is edited. Steps to reproduce:

  1. Install Standard.
  2. Create an article node.
  3. Add a comment on the article.
  4. Visit /admin/content/comment and confirm that the username is displayed.
  5. Edit the comment and change something, e.g. the comment body.
  6. Visit /admin/content/comment. The username disappears.

This does not happen on the admin comments page in HEAD.

xjm’s picture

@andypost confirmed that #308 is a bug in HEAD with comment views. Unfortunately, I think it's a blocking bug. :( We can't regress this so that edited comments have no author name listed on the admin comments page. That probably means this will have to get postponed to 8.1.x unless the fix for that other issue (I didn't check whether it exists somewhere yet) is trivial to fix.

xjm’s picture

andypost’s picture

uncommented test

@jibran while #1972242: User compact view mode is not configured in minimal is open we can't use comments for user in integration tests, need to change profile to standard or properly setup user displays

larowlan’s picture

+++ b/core/modules/comment/config/optional/views.view.comment.yml
@@ -0,0 +1,1052 @@
+        name:
+          id: name
+          table: comment_field_data
+          field: name
+          relationship: none
+          group_type: group
+          admin_label: ''

Pretty sure this is the cause of #2614504: Values of 'name' & 'email' fields should be NULL when comment has author (uid > 0) and xjm's issues. The name field is the wrong field. We should be using uid which has the AuthorFormatter which handles using usernames of registered users and falls back to the entered name value for anonymous users.
The name field *does not* support that level of flexibility.

catch’s picture

Version: 8.0.x-dev » 8.1.x-dev

Moving to 8.1.x.

dobe’s picture

If your setting this to 8.1.x where does that leave: #1823450: [Meta] Convert core listings to Views?

jibran’s picture

Status: Needs work » Needs review
FileSize
10.47 KB
73.86 KB

Fixed #312.

jibran’s picture

Status: Needs work » Needs review
FileSize
3.59 KB
75.35 KB

Fixed the tests. Working on a test for #308

jibran’s picture

jibran’s picture

jibran’s picture

Issue summary: View changes

Leaving minor review to self.

  1. +++ b/core/modules/comment/comment.install
    @@ -125,5 +127,34 @@ function comment_update_8001() {
    +  $config_install_path = drupal_get_path('module', 'comment') . '/' . InstallStorage::CONFIG_INSTALL_DIRECTORY;
    ...
    +  $optional_install_path = drupal_get_path('module', 'comment') . '/' . InstallStorage::CONFIG_OPTIONAL_DIRECTORY;
    

    Use $module_handler->getModule('comment')->getPath();

  2. +++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentPermalinkFormatter.php
    @@ -0,0 +1,55 @@
    + * Plugin implementation of the 'string' formatter.
    

    Improve the desc.

jibran’s picture

Issue summary: View changes
Issue tags: -rc target
FileSize
4.12 KB

Added tests for #308 after implementing #312 the bug described in #308 is not an issue anymore.
Also fixed the feedback from #321.

This is ready for review now also update IS.

jibran’s picture

jibran’s picture

Issue summary: View changes
jibran’s picture

Anyone care to review.

andypost’s picture

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
    @@ -122,7 +123,7 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
           // For the default revision this falls back to 'canonical'
    -      $url = $items->getEntity()->urlInfo('revision');
    +      $url = $this->getEntityUrl($items->getEntity());
    
    @@ -160,4 +161,18 @@ protected function viewValue(FieldItemInterface $item) {
    +  protected function getEntityUrl(EntityInterface $entity) {
    +    // For the default revision this falls back to 'canonical'.
    +    return $entity->toUrl('revision');
    

    any reason for the additional method call?

  2. +++ b/core/modules/comment/comment.routing.yml
    @@ -2,7 +2,7 @@ comment.admin:
    -    _controller: '\Drupal\comment\Controller\AdminController::adminPage'
    +    _form: '\Drupal\comment\Form\CommentAdminOverview'
    
    @@ -11,7 +11,7 @@ comment.admin_approval:
    -    _controller: '\Drupal\comment\Controller\AdminController::adminPage'
    +    _form: '\Drupal\comment\Form\CommentAdminOverview'
    
    @@ -54,6 +54,14 @@ entity.comment.delete_form:
    +comment.multiple_delete_confirm:
    +  path: '/admin/content/comment/delete'
    
    +++ b/core/modules/comment/src/Form/ConfirmDeleteMultiple.php
    @@ -63,7 +75,7 @@ public function getFormId() {
    +    return $this->formatPlural(count($this->commentInfo), 'Are you sure you want to delete this comment and all its children?', 'Are you sure you want to delete these comments and all their children?');
    
    @@ -77,39 +89,56 @@ public function getCancelUrl() {
    +              '#markup' => $this->t('@label (Original translation) - <em>The following content translations will be deleted:</em>', ['@label' => $node->label()]),
    
    @@ -118,12 +147,56 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +        $this->logger('content')->notice('Deleted @count comments.', array('@count' => count($delete_comments)));
    ...
    +          $this->logger('content')->notice('Deleted @count comment translations.', array('@count' => $count));
    

    I pretty sure this changes should be moved to separate issue!

    1) get rid of AdminController
    2) Details about comments and translations deletion

  3. +++ b/core/modules/comment/src/Form/ConfirmDeleteMultiple.php
    @@ -77,39 +89,56 @@ public function getCancelUrl() {
    -    $edit = $form_state->getUserInput();
    +    $this->commentInfo = $this->tempStoreFactory->get('comment_multiple_delete_confirm')->get($this->currentUser()->id());
    ...
    -    $this->comments = $this->commentStorage->loadMultiple(array_keys(array_filter($edit['comments'])));
    
    +++ b/core/modules/comment/src/Plugin/Action/DeleteComment.php
    @@ -0,0 +1,101 @@
    + *   id = "comment_delete_action",
    ...
    +class DeleteComment extends ActionBase implements ContainerFactoryPluginInterface {
    

    Also separate issue, new action and proper delete implementation

  4. +++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentPermalinkFormatter.php
    @@ -0,0 +1,58 @@
    + * Plugin implementation of the 'comment_permalink' formatter.
    ...
    + * All the other entities use 'canonical' or 'revision' links to link the entity
    + * to itself but comments use permalink URL.
    ...
    +class CommentPermalinkFormatter extends StringFormatter {
    

    new formatter, maybe better fits into #2227503: Apply formatters and widgets to Comment base fields

  5. +++ b/core/modules/comment/src/Tests/CommentTestBase.php
    @@ -116,7 +119,7 @@ public function postComment($entity, $comment, $subject = '', $contact = NULL, $
    -      $field = FieldConfig::loadByName('node', $entity->bundle(), $field_name);
    +      $field = FieldConfig::loadByName($entity->getEntityTypeId(), $entity->bundle(), $field_name);
    

    this actually not good idea, the rest of code supposes "node" as entity.

jibran’s picture

  1. See the doc above the CommentPermalinkFormatter for that.
  2. The original purpose of this issue was to get rid of those controller you can't do it without adding this view so moving to other issue is useless imo. Details about comments and translations deletion After adding the view this will become the bug as @xjm said earlier we can't have that so I have to fix it here.
  3. Now that the issue is ready and we can add action and enable in single update hook you want me to move it new issue. IMHO blocking this issue on action plugin addition issue doesn't make sense to me.
  4. This formatter is required here for feature parity.
  5. Comments are not dependent on node anymore, I don't see any harm in making it generic. This is a test code and it has no performance repercussions and it works exactly same for all the entities so I really see the issue here.
dawehner’s picture

The original purpose of this issue was to get rid of those controller you can't do it without adding this view so moving to other issue is useless imo. Details about comments and translations deletion After adding the view this will become the bug as @xjm said earlier we can't have that so I have to fix it here.

Mh, so a maintainer decided its good to have those changes, so let's accept that. In general it feels counterintuitive that we have to change parts of the code which is overridden by views anyway though.

Now that the issue is ready and we can add action and enable in single update hook you want me to move it new issue. IMHO blocking this issue on action plugin addition issue doesn't make sense to me.

I totally agree with @jibran here.

Comments are not dependent on node anymore, I don't see any harm in making it generic. This is a test code and it has no performance repercussions and it works exactly same for all the entities so I really see the issue here.

Sure, but its more stuff to review or rather it removes time to review the actual important changes.

  1. +++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentPermalinkFormatter.php
    @@ -0,0 +1,58 @@
    +  public static function isApplicable(FieldDefinitionInterface $field_definition) {
    +    return parent::isApplicable($field_definition) && $field_definition->getName() === 'subject';
    

    IMHO we should check for the entity type here as well. This really just makes sense on comments.

  2. +++ b/core/modules/comment/src/Tests/Update/CommentAdminViewUpdateTest.php
    @@ -0,0 +1,51 @@
    +  public function testUpdateHookN() {
    +    $this->runUpdates();
    +    // Ensure we can load the view from the storage after the update and it's
    +    // enabled.
    +    $entity_type_manager = \Drupal::entityTypeManager();
    +    /** @var \Drupal\views\ViewEntityInterface $comment_admin_view */
    +    $comment_admin_view = $entity_type_manager->getStorage('view')->load('comment');
    +    $this->assertNotNull($comment_admin_view, 'Comment admin view exist in storage.');
    +    $this->assertTrue($comment_admin_view->enable(), 'Comment admin view is enabled.');
    +    $comment_delete_action = $entity_type_manager->getStorage('action')->load('comment_delete_action');
    +    $this->assertNotNull($comment_delete_action, 'Comment delete action imported');
    +  }
    

    Ideally we would also go to the comment page and check whether something is rendered, just to be sure.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

patch -p1 --dry-run checking file core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
Hunk #1 succeeded at 2 (offset -5 lines).
Hunk #2 succeeded at 118 (offset -5 lines).
Hunk #3 succeeded at 156 (offset -5 lines).
checking file core/modules/comment/comment.install
Hunk #2 succeeded at 127 with fuzz 1.
checking file core/modules/comment/comment.routing.yml
checking file core/modules/comment/config/install/system.action.comment_delete_action.yml
checking file core/modules/comment/config/optional/views.view.comment.yml
checking file core/modules/comment/config/schema/comment.schema.yml
checking file core/modules/comment/config/schema/comment.views.schema.yml
checking file core/modules/comment/src/CommentViewsData.php
Hunk #1 FAILED at 27.
Hunk #2 succeeded at 167 with fuzz 2 (offset 49 lines).
1 out of 2 hunks FAILED
checking file core/modules/comment/src/Controller/AdminController.php

vprocessor’s picture

Assigned: vprocessor » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
79.64 KB

reroll is ready

vprocessor’s picture

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Issue tags: +Triaged core major

When the Views maintainers and core committers discussed this issue at DrupalCon Dublin, we agreed that this is still a major task.

It looks like the Migrate tests might need an update for this issue -- retesting #335 to be sure.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

jibran’s picture

This is ready for the review again.

core/modules/comment/src/Tests/Update/CommentAdminViewUpdateTest.php
line 6	Unused use statement

This can be removed after next review.

Lendude’s picture

Status: Needs review » Needs work

Applied the patch, ran the update, and tada! a View, Nice!

Reviewing this makes me wish this had been split up into multiple issues with a meta or something, what a monster :)

some things I saw:

  1. +++ b/core/modules/comment/src/Form/CommentAdminOverview.php
    @@ -255,23 +265,33 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +          $comment->setPublished(FALSE);
    

    Lets use setUnpublished(), this use is deprecated

  2. +++ b/core/modules/comment/src/Form/CommentAdminOverview.php
    @@ -255,23 +265,33 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
    +          $comment->setPublished(TRUE);
    

    And lose the TRUE here.

  3. +++ b/core/modules/comment/src/Form/ConfirmDeleteMultiple.php
    @@ -72,39 +83,56 @@ public function getCancelUrl() {
    +              '#markup' => $this->t('@label (Original translation) - <em>The following content translations will be deleted:</em>', ['@label' => $node->label()]),
    

    $node should be $comment and that should fatal, so I think we lack test coverage for this

  4. +++ b/core/modules/comment/src/Plugin/Action/DeleteComment.php
    @@ -0,0 +1,96 @@
    +  /**
    +   * Constructs a new DeleteComment object.
    +   *
    +   * @param array $configuration
    +   *   A configuration array containing information about the plugin instance.
    +   * @param string $plugin_id
    +   *   The plugin ID for the plugin instance.
    +   * @param array $plugin_definition
    +   *   The plugin implementation definition.
    +   * @param \Drupal\user\PrivateTempStoreFactory $temp_store_factory
    +   *   The tempstore factory.
    +   */
    +  public function __construct(array $configuration, $plugin_id, array $plugin_definition, PrivateTempStoreFactory $temp_store_factory, AccountInterface $current_user) {
    

    $current_user is missing from @params

  5. +++ b/core/modules/comment/src/Plugin/views/field/CommentedEntity.php
    @@ -0,0 +1,48 @@
    +class CommentedEntity extends Field {
    

    This should extend EntityField

  6. +++ b/core/modules/comment/src/Tests/CommentAdminTest.php
    @@ -98,6 +99,15 @@ public function testApprovalAdminInterface() {
    +    $this->node->setPublished(FALSE)->save();
    

    Lets lose the FALSE

  7. +++ b/core/modules/comment/src/Tests/Views/CommentAdminTest.php
    @@ -0,0 +1,179 @@
    +    $this->node->setPublished(FALSE)->save();
    

    lets lose the FALSE

jibran’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
7.92 KB
80.58 KB

Fixed #341.
Re: #342 Thanks, for the review.

  1. Fixed.
  2. Fixed.
  3. Fixed. I agree comment translation deletion is missing tests. I'll add tests later.
  4. Fixed.
  5. Fixed. This patch is old and the new class is new. :D
  6. Fixed.
  7. Fixed.
jibran’s picture

Status: Needs work » Needs review
FileSize
713 bytes
80.58 KB

Wrong substitution.

jibran’s picture

Added tests for #342.3

jibran’s picture

I had a chat with @Lendude on IRC here is the new patch fixes his feedback.

  1. wondering why the two displays use different ways to show the author
    +++ b/core/modules/comment/config/optional/views.view.comment.yml
    @@ -795,10 +795,10 @@ display:
    -        name:
    -          id: name
    +        uid:
    +          id: uid
    

    Fixed it.

  2. the fields settings are overridden for the second display and I was wondering why that was, and can’t really see a reason for it

    The reason for this is because we have different operations on both pages.

  3. Updated the issue summary with the new screenshots.
  4. Updated the admin display description in view.
Lendude’s picture

#347.2 Ah yeah that makes sense.

@jibran sorry about the fractured feedback but every time I look through this I see some new stuff, its big, there is a lot to look at :)

I think this is really close, one (hopefully) last thing: We seem to be missing explicit test coverage for \Drupal\comment\Plugin\Field\FieldFormatter\CommentPermalinkFormatter. Can't really see any implicit test coverage either, or am I overlooking that?

jibran’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks, @Lendude for the reviews all good as long as we are moving along and improving the patch I'm fine with the changes.

Can't really see any implicit test coverage either, or am I overlooking that?

Nice catch, I'll add some asserts for implicit tests.

jibran’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
8.47 KB
84.88 KB

Added some asserts around AuthorNameFormatter and CommentPermalinkFormatter.

jibran’s picture

CommentEditTest++

    // Empty author ID should revert to anonymous.
    $author_id = $form_state->getValue('uid');
    if ($comment->id() && $this->currentUser->hasPermission('administer comments')) {
      // Admin can leave the author ID blank to revert to anonymous.
      $author_id = $author_id ?: 0;
    }
    if (!is_null($author_id)) {
      if ($author_id === 0 && $form['author']['name']['#access']) {
        // Use the author name value when the form has access to the element and
        // the author ID is anonymous.
        $comment->setAuthorName($form_state->getValue('name'));
      }
      else {
        // Ensure the author name is not set.
        $comment->setAuthorName(NULL);
      }
    }
    else {
      $author_id = $this->currentUser->id();
    }
    $comment->setOwnerId($author_id);

Because of this convoluted logic 'name' field of comment can be empty, at that point we can show the uid of the comment so I have updated the view like this:

+++ b/core/modules/comment/config/optional/views.view.comment.yml
@@ -293,7 +358,7 @@ display:
+          empty: '{{ uid }}'

@@ -838,7 +968,7 @@ display:
+          empty: '{{ uid }}'
Lendude’s picture

Status: Needs review » Reviewed & tested by the community

@jibran great work on this.

The extra test coverage looks good and great to see we have coverage to catch stuff like #350.

larowlan’s picture

jibran’s picture

RE: #354 No, I have worked around that in #352 and added some asserts for that as well.

andypost’s picture

@larowlan This is no longer blocker, admin view will use uid field to display author in "comment manner"

So I changed title and explained in #2614504-79: Values of 'name' & 'email' fields should be NULL when comment has author (uid > 0) - this is not blocker but about data integrity (name&mail should be cleared if comment gets author)

larowlan’s picture

Updating issue credits

Adding myself for reviews/manual testing
Adding @miro_dietiker for manual reviews/suggestions
Adding @tim.plunkett for mentoring and reviews
Adding @Berdir for mentoring and reviews
Adding @Lendude for reviews
Adding @plach for providing guidance on translation/language and reviews
Adding @olli for reviews
Adding @damiankloip for reviews

larowlan’s picture

Reviewed code again, looks good.

Queued tests for other DB backends in case we have a sort issue.

Manually testing upgrade path and patch.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record

Manually tested, works well.

Confirmed that #310 is resolved

We just need a change record here.

larowlan’s picture

Also, some follow up feature requests to add some filters and other shiny things that weren't previously possible would be awesome.

jibran’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Here we go https://www.drupal.org/node/2898013. Thanks, for reviewing it.

  • larowlan committed 04d6992 on 8.5.x
    Issue #1986606 by jibran, pcambra, vijaycs85, dawehner, andypost, mbovan...

  • larowlan committed 9c443f8 on 8.4.x
    Issue #1986606 by jibran, pcambra, vijaycs85, dawehner, andypost, mbovan...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.4 release notes

Woohoo!

Thanks to everyone for the marathon effort here - particularly @jibran and @pcambra with 45 and 23 patches respectively.

Committed as 04d6992 and pushed to 8.5.x.

Cherry-picked as 9c443f8 and pushed to 8.4.x.

Published changed record.

Can someone open a follow-up to add some shiny filters like we have on admin/content.

Once again - thank you so much to everyone who has helped get this one over the line.

larowlan’s picture

Fixing release notes tag

larowlan’s picture

jibran’s picture

> Can someone open a follow-up to add some shiny filters like we have on admin/content.
> Also, some follow up feature requests to add some filters and other shiny things that weren't previously possible would be awesome.
On super powerful public demand added #2898344: Add filters to the comments administration pages.

Thanks for the commits @larowlan.

xjm’s picture

So I don't see any profiling documented on this issue. Usually when we add new listing queries we at least should add an explain. See: https://www.drupal.org/core/gates#performance

Not necessarily a reason to roll it back and hopefully the view is already being optimized correctly, but we should confirm with an EXPLAIN on the view query and possibly compare it to the 8.3.x query for the old hardcoded page.

jibran’s picture

Not necessarily a reason to roll it back and hopefully the view is already being optimized correctly, but we should confirm with an EXPLAIN on the view query and possibly compare it to the 8.3.x query for the old hardcoded page.

\Drupal\comment\Form\CommentAdminOverview handles is it in three steps and we load user entity for each row.

    $cids = $this->commentStorage->getQuery()
      ->condition('status', $status)
      ->tableSort($header)
      ->pager(50)
      ->execute();

    /** @var $comments \Drupal\comment\CommentInterface[] */
    $comments = $this->commentStorage->loadMultiple($cids);

    $commented_entity_ids = [];
    $commented_entities = [];

    foreach ($comments as $comment) {
      $commented_entity_ids[$comment->getCommentedEntityTypeId()][] = $comment->getCommentedEntityId();
    }

    foreach ($commented_entity_ids as $entity_type => $ids) {
      $commented_entities[$entity_type] = $this->entityTypeManager
        ->getStorage($entity_type)
        ->loadMultiple($ids);
    }

The view query is

SELECT comment_field_data.cid AS cid, users_field_data_comment_field_data.uid AS users_field_data_comment_field_data_uid
FROM 
{comment_field_data} comment_field_data
LEFT JOIN {users_field_data} users_field_data_comment_field_data ON comment_field_data.uid = users_field_data_comment_field_data.uid
WHERE comment_field_data.status = '1'
ORDER BY comment_field_data.changed DESC
LIMIT 50 OFFSET 0

And the exlplain of this is

+------+-------------+-------------------------------------+------+----------------------------------------------+------------------------------+---------+------------------------------+------+-----------------------------+
| id   | select_type | table                               | type | possible_keys                                | key                          | key_len | ref                          | rows | Extra                       |
+------+-------------+-------------------------------------+------+----------------------------------------------+------------------------------+---------+------------------------------+------+-----------------------------+
|    1 | SIMPLE      | comment_field_data                  | ref  | comment__status_comment_type                 | comment__status_comment_type | 1       | const                        |    2 | Using where; Using filesort |
|    1 | SIMPLE      | users_field_data_comment_field_data | ref  | PRIMARY,user__id__default_langcode__langcode | PRIMARY                      | 4       | local.comment_field_data.uid |    1 | Using where; Using index    |
+------+-------------+-------------------------------------+------+----------------------------------------------+------------------------------+---------+------------------------------+------+-----------------------------+

We have never discussed views render queries when replacing the admin page. Do you want me to share those as well?

andypost’s picture

@jibran I bet @xjm called to measure performance because views load each single comment while rendering, so removal of $comments = $this->commentStorage->loadMultiple($cids); should bring serious impact on performance particularly on cold caches.

Explaining main query will not show actual impact of the change, so checking perf here is tricky

Probably this cases are most needed
1) cold entity cache - the most issue I bet
2) cold render & views cache
3) posting new comment (must invalidate "comment_list" cache tag) so affects both

jibran’s picture

Can we please create a follow-up issue for this?

xjm’s picture

@jibran, well, the core gates are supposed to be addressed before commit, so we should confirm them in this issue.

larowlan’s picture

Here's a call graph between 8.3 and 8.4 with warm caches https://blackfire.io/profiles/compare/c8b886bc-a6b4-49b4-a0da-7cdc8ea5dc....

Net take away is that building the form was slower than the view.

Tested with previousnext.com which has lots of comments.

Disabled breakpoint, admin toolbar, toolbar, responsive image which make the page 6x slower (looking at you admin toolbar).

The table display (8.3) took 131ms, the view display (8.4) took 135, but the form (8.3) took 97ms whereas the views pre-render (form stuff) took 93ms, so its swings and roundabouts.

Additional query checks/EXPLAIN etc are in #2898344: Add filters to the comments administration pages

Will ping catch for sign-off.

larowlan’s picture

Cold caches https://blackfire.io/profiles/compare/11203d6d-d566-4d61-afee-216467eb74... not as big a difference, but still in favour of this patch

jibran’s picture

@catch posted in #2898344-48: Add filters to the comments administration pages

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!

Status: Fixed » Closed (fixed)

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