Updated #204

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

Fix failing tests.
Review.
Manual testing.

User interface changes

Before Empty


After Empty


Before With Comment


After With Comment


API changes

N/A

Beta phase evaluation

Copied from https://www.drupal.org/node/1823450#beta-evaluation

This is appropriate for 8.0.x.

It is a major task that has impact greater than disruption, so it can proceed in 8.0.x.

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because the functionality of these listings is already in core (with custom code)
Issue priority Major because this Meta is about replacing core listings in many places, across systems, and has community consensus for being important. Not critical because we can release 8.0.0 without completing the conversion to views and do it in a later release. Keeping the custom code (not converting a listing) will not cause severe performance or usability regressions.
Prioritized changes A bit, it maybe would reduce fragility by re-using views code and getting rid of custom listing code.
Disruption It will not be disruptive for core/contributed and custom modules/themes because it will not require a BC break/deprecation/widespread changes since changes are isolated to core listing custom code.
Files: 
CommentFileSizeAuthor
#253 convert_the_comments-1986606-253.patch66.91 KBjibran
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,390 pass(es).
[ View ]

Comments

pcambra’s picture

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

Status:Active» Needs work
StatusFileSize
new28.9 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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
StatusFileSize
new17.61 KB
new31.38 KB
FAILED: [[SimpleTest]]: [MySQL] 56,119 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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
StatusFileSize
new2.26 KB
new31.48 KB
FAILED: [[SimpleTest]]: [MySQL] 55,332 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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
StatusFileSize
new48.77 KB
new48.99 KB
FAILED: [[SimpleTest]]: [MySQL] 58,126 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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
StatusFileSize
new5.3 KB
new53.2 KB
PASSED: [[SimpleTest]]: [MySQL] 58,127 pass(es).
[ View ]
pcambra’s picture

StatusFileSize
new53.2 KB
PASSED: [[SimpleTest]]: [MySQL] 58,075 pass(es).
[ View ]
new934 bytes

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

StatusFileSize
new57.44 KB
PASSED: [[SimpleTest]]: [MySQL] 56,822 pass(es).
[ View ]
new10.42 KB

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
StatusFileSize
new67.93 KB
PASSED: [[SimpleTest]]: [MySQL] 58,138 pass(es).
[ View ]
new40.22 KB

Added tests and fixed the optional/broken links

pcambra’s picture

StatusFileSize
new66.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,337 pass(es).
[ View ]
new32.14 KB

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
StatusFileSize
new85.77 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1986606-comments_admin_views-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new20.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

StatusFileSize
new85.67 KB
FAILED: [[SimpleTest]]: [MySQL] 58,187 pass(es), 46 fail(s), and 0 exception(s).
[ View ]

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

StatusFileSize
new77.39 KB
new83.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
StatusFileSize
new86.12 KB
FAILED: [[SimpleTest]]: [MySQL] 56,574 pass(es), 46 fail(s), and 0 exception(s).
[ View ]

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
StatusFileSize
new85.38 KB
FAILED: [[SimpleTest]]: [MySQL] 56,384 pass(es), 45 fail(s), and 0 exception(s).
[ View ]
new3.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
StatusFileSize
new85.66 KB
FAILED: [[SimpleTest]]: [MySQL] 57,143 pass(es), 46 fail(s), and 3 exception(s).
[ View ]
new7.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.

Status:Needs review» Needs work

The last submitted patch, 1986606-comments_admin_views-42.patch, failed testing.

pcambra’s picture

Status:Needs work» Needs review
StatusFileSize
new41.57 KB
new91.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1986606-comments_admin_views-45.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

andypost’s picture

Parent issue:» #1978904: Convert comment_admin() to a Controller
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

StatusFileSize
new41.97 KB
FAILED: [[SimpleTest]]: [MySQL] 59,370 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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
StatusFileSize
new55.76 KB
FAILED: [[SimpleTest]]: [MySQL] 59,294 pass(es), 15 fail(s), and 0 exception(s).
[ View ]
new15.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?

The last submitted patch, 71: d8-comment-view-1986606-71.patch, failed testing.

jibran’s picture

StatusFileSize
new58.35 KB
FAILED: [[SimpleTest]]: [MySQL] 59,366 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.59 KB

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

The last submitted patch, 73: d8-comment-view-1986606-73.patch, failed testing.

klonos’s picture

Status:Needs review» Needs work
dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new59.09 KB
PASSED: [[SimpleTest]]: [MySQL] 59,346 pass(es).
[ View ]
new751 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
StatusFileSize
new58.88 KB
FAILED: [[SimpleTest]]: [MySQL] 63,564 pass(es), 59 fail(s), and 1 exception(s).
[ View ]

re-rolled

Status:Needs review» Needs work

The last submitted patch, 79: comments_administration_view-1986606-79.patch, failed testing.

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

StatusFileSize
new1.34 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
new1.34 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
adnen’s picture

Status:Active» Needs review
StatusFileSize
new1.34 MB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
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
StatusFileSize
new58.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,092 pass(es), 14 fail(s), and 4 exception(s).
[ View ]
new1.31 KB
new84.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
StatusFileSize
new61.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,126 pass(es), 60 fail(s), and 4 exception(s).
[ View ]
new6.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
StatusFileSize
new61.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,098 pass(es), 46 fail(s), and 0 exception(s).
[ View ]
new1.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
StatusFileSize
new98.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1986606-comment_admin_view-109.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
tkuldeep17’s picture

Status:Needs work» Needs review
StatusFileSize
new98.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

After updating latest code, patch is submitting..

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new66.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,555 pass(es), 14 fail(s), and 4 exception(s).
[ View ]
new15.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
StatusFileSize
new66.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,569 pass(es).
[ View ]
new1.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

StatusFileSize
new66.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,757 pass(es).
[ View ]
new502 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
StatusFileSize
new328.55 KB
new318.43 KB
new56.42 KB
new49.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
StatusFileSize
new72.18 KB

Sorry, total screenshot embed fail.

dawehner’s picture

StatusFileSize
new67.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,331 pass(es), 92 fail(s), and 21 exception(s).
[ View ]
new5.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.

Status:Needs review» Needs work

The last submitted patch, 129: vdc-1986606-129.patch, failed testing.

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

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new65.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Reroll of #129

Status:Needs review» Needs work

The last submitted patch, 140: 1986606-comment-admin-veiew-140.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new65.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new1.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

Status:Needs review» Needs work

The last submitted patch, 142: 1986606-comment-admin-veiew-142.patch, failed testing.

vijaycs85’s picture

StatusFileSize
new63.71 KB
new1.74 KB

Thanks @tim.plunkett. Updated.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new63.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,077 pass(es), 95 fail(s), and 12 exception(s).
[ View ]
new538 bytes

Minor update...

Status:Needs review» Needs work

The last submitted patch, 146: 1986606-comment-admin-view-146.patch, failed testing.

larowlan’s picture

Issue tags:+Needs reroll
pcambra’s picture

Status:Needs work» Needs review
StatusFileSize
new63.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,389 pass(es), 112 fail(s), and 51 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 149: 1986606-comment-admin-view-149.patch, failed testing.

pcambra’s picture

Status:Needs work» Needs review
StatusFileSize
new15.77 KB
new61.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,408 pass(es), 85 fail(s), and 44 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 151: 1986606-comment-admin-view-151.patch, failed testing.

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
StatusFileSize
new31.01 KB
new38.7 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,798 pass(es).
[ View ]

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

StatusFileSize
new2.24 KB
new38.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,803 pass(es).
[ View ]

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
StatusFileSize
new5.5 KB
new36.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,868 pass(es).
[ View ]

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

StatusFileSize
new5.01 KB
new34.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,779 pass(es).
[ View ]

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

StatusFileSize
new47.68 KB

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

StatusFileSize
new29.79 KB

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

StatusFileSize
new3.46 KB
new35.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1986606-comment-admin-view-177.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 177: 1986606-comment-admin-view-177.patch, failed testing.

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new35.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

Re-rolled

Status:Needs review» Needs work

The last submitted patch, 185: 1986606-185.patch, failed testing.

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new788 bytes
new35.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,947 pass(es).
[ View ]

ok

jibran’s picture

StatusFileSize
new777 bytes
new35.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1986606-188.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 188: 1986606-188.patch, failed testing.

jibran’s picture

StatusFileSize
new34.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,985 pass(es), 151 fail(s), and 109 exception(s).
[ View ]

Reroll.

kim.pepper’s picture

Status:Needs work» Needs review

Kicking the bot.

Status:Needs review» Needs work

The last submitted patch, 192: convert_the_comments-1986606-192.patch, failed testing.

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new34.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,479 pass(es), 14 fail(s), and 2 exception(s).
[ View ]
new27.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

Status:Needs review» Needs work

The last submitted patch, 195: 1986606-comment-view-195.patch, failed testing.

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
StatusFileSize
new1.96 KB
new34.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,609 pass(es), 58 fail(s), and 50 exception(s).
[ View ]

I hope this will fix the fails.

Status:Needs review» Needs work

The last submitted patch, 199: convert_the_comments-1986606-199.patch, failed testing.

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new15.18 KB
new42.74 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,461 pass(es), 14 fail(s), and 2 exception(s).
[ View ]

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?

Status:Needs review» Needs work

The last submitted patch, 201: convert_the_comments-1986606-201.patch, failed testing.

jibran’s picture

Assigned:jibran» Unassigned
Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new8.64 KB
new48.92 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,356 pass(es).
[ View ]
new60.55 KB
new48.17 KB
new65.29 KB
new66.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
StatusFileSize
new208.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
StatusFileSize
new1.86 KB
new49.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,454 pass(es).
[ View ]

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

StatusFileSize
new1.86 KB
new49.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,610 pass(es).
[ View ]

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

Fixed that. We can further simplify

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

to

<?php
$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
StatusFileSize
new1.62 KB
new49.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,098 pass(es).
[ View ]

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 as well. Thank you for adding the related issue.

Arla’s picture

StatusFileSize
new545 bytes
new49.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch convert_the_comments-1986606-220.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 220: convert_the_comments-1986606-220.patch, failed testing.

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

Status:Needs work» Needs review
StatusFileSize
new4.71 KB
new47.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,381 pass(es).
[ View ]

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

StatusFileSize
new33.96 KB
new66.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,724 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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?

Status:Needs review» Needs work

The last submitted patch, 232: convert_the_comments-1986606-232.patch, failed testing.

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.

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
StatusFileSize
new5.05 KB
new1.29 KB
new68.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,890 pass(es), 67 fail(s), and 48 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 238: convert_the_comments-1986606-238.patch, failed testing.

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new590 bytes
new68.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,636 pass(es).
[ View ]

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

StatusFileSize
new0 bytes
new66.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,647 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

StatusFileSize
new12.22 KB

Status:Needs review» Needs work

The last submitted patch, 242: convert_the_comments-1986606-242.patch, failed testing.

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new4.62 KB
new66.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,637 pass(es).
[ View ]

With the test fix. This needs multilingual approval now.

jibran’s picture

StatusFileSize
new19 KB
new25.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,849 pass(es).
[ View ]

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

StatusFileSize
new612 bytes
new66.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,104 pass(es).
[ View ]

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

StatusFileSize
new1.3 KB
new66.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,390 pass(es).
[ View ]
  • 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().