For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Meta issue: #1971384: [META] Convert page callbacks to controllers

Blocked by#1986606: Convert the comments administration screen to a view
Blocks #1946348: Convert all of confirm_form() in comment.admin.inc to the new form interface

UX Changes
No longer possible to sort the results by 'posted in' however in HEAD that is broken since #731724: Convert comment settings into a field to make them work with CMI and non-node entities

Files: 
CommentFileSizeAuthor
#94 comment-admin-1978904-94.patch29.81 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 60,193 pass(es).
[ View ]
#94 interdiff.txt3.04 KBandypost

Comments

StatusFileSize
new3.38 KB
FAILED: [[SimpleTest]]: [MySQL] 55,311 pass(es), 65 fail(s), and 406 exception(s).
[ View ]

First attempt at one of these. Not sure if I should do anything with the drupal_get_form calls?

Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch, comment-admin-controller-1978904-1.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new15.33 KB
new16.67 KB
FAILED: [[SimpleTest]]: [MySQL] 55,536 pass(es), 12 fail(s), and 4 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, comment-admin-controller-1978904-4.patch, failed testing.

Status:Needs work» Postponed

I believe the remaining fails are the fault of the delete form not being found. That form is being converted as part of #1946348: Convert all of confirm_form() in comment.admin.inc to the new form interface, so this should probably wait for that.

Fixing tags

Status:Postponed» Needs work

We need to sort out the crufty handling of comment_admin() in this issue, before we can fix #1946348: Convert all of confirm_form() in comment.admin.inc to the new form interface

We could just make this a _form router item, and then create a separate one for confirming deleting over at #1946348: Convert all of confirm_form() in comment.admin.inc to the new form interface.

Can we pass multiple comment ids in the url to a the delete multiple form?

There must be an example of where we're doing this somewhere else.

Ah yes, I've done this in both #1895160: Convert admin/content to a View, keep a non-views fallback with no bulk operations and #1851086: Replace admin/people with a View by using tempstore.

From the admin/people issue:

function user_user_operations_cancel(array $accounts) {
  global $user;
  // Store the accounts to be canceled in a tempstore.
  Drupal::service('user.tempstore')->get('user_user_operations_cancel')->set($user->uid, $accounts);
}
function user_multiple_cancel_confirm($form, &$form_state) {
  global $user;
  // Retrieve the accounts to be canceled from the temp store.
  $accounts = Drupal::service('user.tempstore')->get('user_user_operations_cancel')->get($user->uid);
  // confirm_form() call down here
}
function user_multiple_cancel_confirm_submit($form, &$form_state) {
  global $user;
  // Clear out the accounts from the temp store.
  Drupal::service('user.tempstore')->get('user_user_operations_cancel')->delete($user->uid);
}

Or similar.

Basically, you store the array of objects, redirect to the confirm form, retrieve them, and then go about as usual. Just don't forget to clear them out.

Issue summary:View changes

Added link to blocked issue

Status:Needs work» Postponed

After discussion with @tim.plunkett in IRC, we've agreed to convert the
admin/comments page to a view with bulk operations #1986606: Convert the comments administration screen to a view, which should help (or
remove the need for) this issue.

Status:Postponed» Closed (duplicate)

After discussing with @dawehner, we need to do the fallback conversion of comment_admin in #1986606: Convert the comments administration screen to a view, marking this one as dupe

Status:Closed (duplicate)» Needs work

Discussed with @dawehner in IRC

dawehner_> andypost: the problem is that these conversions are not straighforward as views have to support overriding existing routes in a proper way
andypost> dawehner_, so maybe convert this to proper controller with a todo for view-route override issue
dawehner_> andypost: yeah this sounds like the safest way forward for now

EDIT: #2027115: Allow views to override existing routing items

Assigned:Unassigned» jibran

Working on it.

Please separate this to own AdminController.php or CommentAdminController.php because #731724: Convert comment settings into a field to make them work with CMI and non-node entities uses separate AdminController for administrative tasks

Assigned:jibran» Unassigned
Status:Needs work» Needs review
Issue tags:+FormBase
StatusFileSize
new20.86 KB
FAILED: [[SimpleTest]]: [MySQL] 58,952 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

Here is the reroll. Yay!! core/modules/comment/comment.admin.inc is deleted.

Looks great!

  1. +++ b/core/modules/comment/lib/Drupal/comment/Form/CommentAdminOverview.php
    @@ -0,0 +1,266 @@
    +

    Extra line (nitpick)

  2. +++ b/core/modules/comment/lib/Drupal/comment/Form/CommentAdminOverview.php
    @@ -0,0 +1,266 @@
    +            '#options' => array('attributes' => array('title' => truncate_utf8($comment->comment_body->value, 128)), 'fragment' => 'comment-' . $comment->id()),

    Unicode::truncate()

Status:Needs review» Needs work

The last submitted patch, drupal8.comment-module.1978904-15.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.92 KB
new20.91 KB
PASSED: [[SimpleTest]]: [MySQL] 58,904 pass(es).
[ View ]

Fixed #17 and tests. I have done some manually testing and it is working fine.

  1. +++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.php
    @@ -243,11 +232,11 @@ public function getReplyForm(Request $request, NodeInterface $node, $pid = NULL)
    -    if ($this->currentUser->isAnonymous()) {
    +    if ($this->currentUser()->isAnonymous()) {

    Great clean-up!

  2. +++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.php
    @@ -273,11 +262,22 @@ public function renderNewCommentsNodeLinks(Request $request) {
    -  public function adminPage($type) {
    ...
    +  public function adminPage(Request $request, $type) {

    As I said before, please move administrative task to own AdminController.php!

  3. +++ b/core/modules/comment/lib/Drupal/comment/Form/CommentAdminOverview.php
    @@ -0,0 +1,266 @@
    +    $query = $this->connection->select('comment', 'c')
    ...
    +    $query->join('node_field_data', 'n', 'n.nid = c.nid');
    +    $query->addTag('node_access');

    Not sure it's right to add 'node_access' tag to other base table. see #2054993-19: Remove (untested, unused, broken) comment_get_recent()

StatusFileSize
new3.84 KB
new22.56 KB
PASSED: [[SimpleTest]]: [MySQL] 59,017 pass(es).
[ View ]

Thanks for the review @andypost. Fixed #20.

Status:Needs review» Reviewed & tested by the community

#21 is RTBC, probably we could get #22 after comment-field patch

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

#22 needs re-roll

Assigned:Unassigned» jibran

Working on the reroll.

Assigned:jibran» Unassigned
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new24.67 KB
FAILED: [[SimpleTest]]: [MySQL] 58,987 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

Reroll of #22.

Status:Needs review» Needs work

The last submitted patch, drupal8.comment-module.1978904-26.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new885 bytes
new24.67 KB
PASSED: [[SimpleTest]]: [MySQL] 59,055 pass(es).
[ View ]

blahhh same fail as #16

Probably defaults for route makes no sense here, so please add default argument

+++ /dev/null
@@ -1,231 +0,0 @@
- * @param $type
- *   The type of the overview form ('approval' or 'new'). See
...
-function comment_admin($type = 'new') {
+++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.php
@@ -224,4 +227,27 @@ public function bundleTitle($field_name) {
+   * @param string $type
+   *   The type of the overview form ('approval' or 'new').
...
+  public function adminPage(Request $request, $type) {

should be 'new' by default

StatusFileSize
new804 bytes
new24.68 KB
PASSED: [[SimpleTest]]: [MySQL] 58,872 pass(es).
[ View ]

Fixed #29.

Status:Needs review» Needs work

The last submitted patch, drupal8.comment-module.1978904-30.patch, failed testing.

Uh, last nitpick ;)

+++ b/core/modules/comment/lib/Drupal/comment/Controller/AdminController.php
@@ -224,4 +227,27 @@ public function bundleTitle($field_name) {
+   * @param string $type
+   *   The type of the overview form ('approval' or 'new').
...
+  public function adminPage(Request $request, $type = 'new') {

Needs - Defaults to 'new'.

Status:Needs work» Needs review

StatusFileSize
new772 bytes
new24.7 KB
PASSED: [[SimpleTest]]: [MySQL] 58,730 pass(es).
[ View ]

Fixed #32.

Status:Needs review» Reviewed & tested by the community

@jibran thank you!

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.89 KB
new24.94 KB
FAILED: [[SimpleTest]]: [MySQL] 58,797 pass(es), 74 fail(s), and 2 exception(s).
[ View ]

Reroll after #2101155: Add CommentManagerInterface and minor doc fixes.

Status:Needs review» Needs work
Issue tags:-FormInterface, -WSCCI-conversion

The last submitted patch, drupal8.comment-module.1978904-37.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.54 KB
new27.9 KB
PASSED: [[SimpleTest]]: [MySQL] 59,094 pass(es).
[ View ]

Sorry for the messed up reroll. Please find some bounce doc fixes. :)

+++ b/core/modules/comment/lib/Drupal/comment/Form/CommentAdminOverview.php
@@ -132,10 +132,26 @@ public function buildForm(array $form, array &$form_state, $type = 'new') {
+        'field' => 'node_title',

Is this still correct?

Other than that looks fine

StatusFileSize
new83.03 KB

Yeah that busts the tablesort headers on the comment-admin page, but could very well be broken in HEAD, lets fix it here.

Screenshot 2013-10-11 05.56.19.png

StatusFileSize
new2.81 KB
new28.09 KB
PASSED: [[SimpleTest]]: [MySQL] 58,991 pass(es).
[ View ]

How about removing it? IMO this node_title doesn't make sense anymore. Now it can be taxonomy_term_name, user_name or {$entity_type}_{$entity_info['entity_keys']['label']} we can't do it for single table unless we make sperate table for each entity_type.

Status:Needs review» Reviewed & tested by the community

Yep, exactly like that.
For committers, this patch removes the ability to sort on 'posted in' from comment-admin screen, but since #731724: Convert comment settings into a field to make them work with CMI and non-node entities that has been broken as seen by #41
The interdiff at 42 is the wrong one but the patch itself is right.
Unless bot disagrees.

Issue summary:View changes

Added link to 1986606

Status:Reviewed & tested by the community» Needs work
  1. +++ b/core/modules/comment/lib/Drupal/comment/Form/CommentAdminOverview.php
    @@ -0,0 +1,316 @@
    +    if ($this->moduleHandler->moduleExists('node')) {
    +      // Special case to ensure node access works.
    +      $query->leftJoin('node_field_data', 'n', "n.nid = c.entity_id AND c.entity_type = 'node'");
    +    }

    Any idea why node access should be fired here? see 3.
    Let's get rid of the join - we list comments!

  2. +++ b/core/modules/comment/lib/Drupal/comment/Form/CommentAdminOverview.php
    @@ -0,0 +1,316 @@
    +    $result = $query
    +      ->fields('c', array('cid', 'subject', 'name', 'changed', 'entity_id', 'entity_type', 'field_id'))
    ...
    +    foreach ($result as $row) {
    +      $entity_ids[$row->entity_type][] = $row->entity_id;
    +      $cids[] = $row->cid;
    +    }

    We actually use 'cid, entity_type, entity_id' so other fields useless

  3. +++ b/core/modules/comment/lib/Drupal/comment/Form/CommentAdminOverview.php
    @@ -0,0 +1,316 @@
    +      // Use the first entity label.
    +      $entity = $entities[$comment->entity_type->value][$comment->entity_id->value];
    +      $entity_uri = $entity->uri();
    ...
    +            '#title' => $entity->label(),
    +            '#href' => $entity_uri['path'],
    +            '#options' => $entity_uri['options'],
    +            '#access' => $entity->access('view'),

    Code comment makes no sense.
    Here's a actual access checking so "node access" should be removed

    There's CommentManager::getParentEntityUri() so maybe we get rid of the micro-optimization with loading commented entities?
    And let's change s/$entity/$commented_entity

  4. +++ b/core/modules/comment/lib/Drupal/comment/Form/CommentAdminOverview.php
    @@ -0,0 +1,316 @@
    +      foreach ($cids as $cid) {
    +        $comment = $this->commentStorage->load($cid);
    +
    +        if ($operation == 'unpublish') {
    +          $comment->status->value = COMMENT_NOT_PUBLISHED;
    +        }
    +        elseif ($operation == 'publish') {
    +          $comment->status->value = COMMENT_PUBLISHED;
    +        }
    +        $comment->save();

    no reason to save comment entity when no changes made...
    Also #2028025: Expand CommentInterface to provide methods will use setPublished() method for that

+++ /dev/null
@@ -1,231 +0,0 @@
-    'posted_in' => array('data' => t('Posted in'), 'field' => 'node_title', 'class' => array(RESPONSIVE_PRIORITY_LOW)),
...
-  if (\Drupal::moduleHandler()->moduleExists('node')) {
-    // Special case to ensure node access works.
-    $query->leftJoin('node_field_data', 'n', "n.nid = c.entity_id AND c.entity_type = 'node'");
-    $query->addTag('node_access');

and the table 'node_field_data' have 'title' column that changed before #2057401: Make the node entity database schema sensible

Status:Needs work» Needs review
Issue tags:+WSCCI, +WSCCI Conversions
StatusFileSize
new6.03 KB
new27.97 KB
PASSED: [[SimpleTest]]: [MySQL] 58,814 pass(es).
[ View ]

Thanks for the review @andypost.

  1. Fixed. I think we can use EFQ now.
  2. Fixed.
  3. Comment Removed. I don't think CommentManager::getParentEntityUri() is worth using. $entity is gone and $commented_entity is in.
  4. Fixed.

Also added comment_uri() call because $comment->uri() is not working :(.

Awesome! the $comment->uri() should work, so this needs debug because we need to get rid of this procedural wrapper and use https://drupal.org/node/2020491 so this hunk needs @todo with link to issue
Otherwise this RTBC

StatusFileSize
new6.51 KB
new27.24 KB
FAILED: [[SimpleTest]]: [MySQL] 59,004 pass(es), 7 fail(s), and 36 exception(s).
[ View ]

This is beauty look at the interdiff. I love EFQ :D. Added @todo.

Status:Needs review» Needs work

The last submitted patch, drupal8.comment-module.1978904-48.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.98 KB
new27.23 KB
PASSED: [[SimpleTest]]: [MySQL] 59,048 pass(es).
[ View ]

Not Drupal\Core\Entity\Query\QueryInterface it is Drupal\Core\Entity\Query\QueryFactory. blahh!!

We lost the bulk loading. Previous code grouped commented entity ids by type then loaded each type in bulk. New code calls load on each row so is far less performant. Besides that, great job on using efq.

We lost the bulk loading. Previous code grouped commented entity ids by type then loaded each type in bulk. New code calls load on each row so is far less performant.

Well you are right about this but I EFQ will not return $entity_type or $entity_id.
We have to make a hard decision here #46 is without EFQ and green, #50 is with EFQ. So both comments maintainers are in the issue it is your call :).

Can't we load the comment entities using the results from efq and then loop those to create an array of commented entity ids, grouped by type, then load each group in one call to load multiple. That's basically what the old code did.

StatusFileSize
new27.6 KB
PASSED: [[SimpleTest]]: [MySQL] 59,008 pass(es).
[ View ]
new1.28 KB

Like this

Status:Needs review» Reviewed & tested by the community

Looks good to me. Assuming @andypost and @larowlan are happy.

+++ b/core/modules/comment/lib/Drupal/comment/Form/CommentAdminOverview.php
@@ -177,8 +177,19 @@ public function buildForm(array $form, array &$form_state, $type = 'new') {
+    foreach ($comments as $comment) {
...
     foreach ($comments as $comment) {

I have the same idea but I am not at all sold out on looping $comments array twice. Bulk loading vs looping 50 comments twice.

I vote for keeping the existing flow an optimising as a follow with real data.

I think the two loops is the far lesser evil than querying (and waiting for) the database over 200 times.

Consider a typical install. 90% of sites will only have comments on nodes.
They will also have menu module installed.
And path module.
And field module.

So to load one node you are looking at these queries

  • the node data tables
  • the field data tables
  • the menu links table
  • the url alias table.

So minimum 4 database queries per node load.
Assuming there are no other contrib modules installed that also react to hook_node_load.
So with 50 nodes thats 200 database queries.
Each of those has the overhead of the DBTNG API and then the wait time for the query (and transfer time if separate db server).

With the patch at #54 there's only 4 queries - one call to node load (multiple).

I'll take 4 queries and two foreach loops through a 50-item array over 200 queries and one foreach loop any day.

Thanks for the detail explanation @larowlan. RTBC +1 for

lesser evil

+1 RTBC

Not sure about 50&200 because most of times we get entities are loaded from cache, anyway this out of scope and this tuning could be done latter and suppose for views implementation

The only thing that left here is 'title callback' that uses comment_count_unpublished() so filed follow-up #2111357: Get rid of comment_count_unpublished() in favor of CommentManager method

StatusFileSize
new27.59 KB
PASSED: [[SimpleTest]]: [MySQL] 59,272 pass(es).
[ View ]

StatusFileSize
new897 bytes
new27.5 KB
FAILED: [[SimpleTest]]: [MySQL] 59,093 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Status:Reviewed & tested by the community» Needs work

The last submitted patch, drupal8.comment-module.1978904-62.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new27.59 KB
PASSED: [[SimpleTest]]: [MySQL] 59,156 pass(es).
[ View ]

Now I see what's wrong here:
1) $comment->uri() have no fragment and does not count proper page
2) comment_uri() the same as above but adds fragment
3) CommentManager::getParentEntityUri() is similar to $comment->permalink() but have fragment

So I recommend leave the issue as proper conversion and fix this URIs in #2010202: Comment permalinks are lacking context and present a Usability regression (for e.g. drupal.org)

+++ b/core/modules/comment/lib/Drupal/comment/Form/CommentAdminOverview.php
@@ -199,9 +199,7 @@ public function buildForm(array $form, array &$form_state, $type = 'new') {
-      $comment_uri = comment_uri($comment);
+      $comment_uri = $comment->permalink();

this change is not valid

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new27.55 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.comment-module.1978904-65.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.64 KB

@jibran was right, we need 2 different URIs here

PS: related issue #2111419: Remove CommentManager::getParentEntityUri() in favor of Comment::permalink()

Status:Needs review» Needs work
Issue tags:+WSCCI, +FormBase, +WSCCI Conversions

The last submitted patch, drupal8.comment-module.1978904-65.patch, failed testing.

Issue tags:+Needs reroll

:/ patch no longer applies.

Issue summary:View changes

added note about sorting

Issue summary:View changes
Status:Needs work» Needs review
Issue tags:-Needs reroll
Parent issue:» #1971384: [META] Convert page callbacks to controllers
Related issues:+#1986606: Convert the comments administration screen to a view, +#1946348: Convert all of confirm_form() in comment.admin.inc to the new form interface
StatusFileSize
new28.61 KB
PASSED: [[SimpleTest]]: [MySQL] 59,382 pass(es).
[ View ]
new4.65 KB

Re-roll
Simplified logic (delete without confirmation is not allowed)
Use form builder service

+++ b/core/modules/comment/lib/Drupal/comment/Form/CommentAdminOverview.php
@@ -278,26 +278,23 @@ public function submitForm(array &$form, array &$form_state) {
-    if ($operation == 'delete') {
-      $comments = $this->commentStorage->loadMultiple($cids);
-      $this->commentStorage->delete($comments);
...
+    foreach ($cids as $cid) {
+      // Delete operation handled in \Drupal\comment\Form\ConfirmDeleteMultiple.

That's primary change

Status:Needs review» Needs work

+++ b/core/modules/comment/lib/Drupal/comment/Form/CommentAdminOverview.php
@@ -278,26 +278,23 @@ public function submitForm(array &$form, array &$form_state) {
+      // Delete operation handled in \Drupal\comment\Form\ConfirmDeleteMultiple.
+      if ($operation == 'unpublish') {

What? Why are we unpublishing here? We should delete here.

+++ b/core/modules/comment/lib/Drupal/comment/Form/CommentAdminOverview.php
@@ -0,0 +1,301 @@
+      // Delete operation handled in \Drupal\comment\Form\ConfirmDeleteMultiple.

ok I got it now. Let's improve the comment by adding @see \Drupal....::adminPage().

Status:Needs work» Needs review
StatusFileSize
new888 bytes
new28.68 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1978904-comment_admin-71.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

here it is

It is RTBC for me. Let's wait @larowlan for RTBC.

Status:Needs review» Reviewed & tested by the community

73: 1978904-comment_admin-71.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 73: 1978904-comment_admin-71.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new21.09 KB
PASSED: [[SimpleTest]]: [MySQL] 60,607 pass(es).
[ View ]

Re-roll.

Status:Needs review» Reviewed & tested by the community

Thanks for the reroll @Xano. Back to RTBC.

StatusFileSize
new28.71 KB
PASSED: [[SimpleTest]]: [MySQL] 60,326 pass(es).
[ View ]

@Xano you forget to remove comment.admin.inc
Patch just adds this hunk so leaving RTBC

Sharp, @andypost! I admit I just checked the reject files and forgot to go through Git's verbose output.

StatusFileSize
new1.25 KB
new28.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1978904-comment_admin-82.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

82: 1978904-comment_admin-82.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 82: 1978904-comment_admin-82.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new28.99 KB
PASSED: [[SimpleTest]]: [MySQL] 59,032 pass(es).
[ View ]

reroll

Status:Needs review» Reviewed & tested by the community

Back to RTBC.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new28.74 KB
PASSED: [[SimpleTest]]: [MySQL] 59,310 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

RTBC again

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

+++ b/core/modules/comment/lib/Drupal/comment/Form/CommentAdminOverview.php
@@ -0,0 +1,304 @@
+      form_set_error('', $this->t('Select one or more comments to perform the update on.'));

We must not have coverage for this at all. form_set_error now requires that $form_state be passed to it. (And you can use $this->setFormError now)

Status:Needs work» Needs review
StatusFileSize
new874 bytes
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,433 pass(es).
[ View ]

Changed form_set_error to setFormError().

Status:Needs review» Needs work

That was a 0 byte patch :)
Also it still needs tests.

StatusFileSize
new28.75 KB
PASSED: [[SimpleTest]]: [MySQL] 59,311 pass(es).
[ View ]
new874 bytes

What kind of test does it need because some things are already checked in Drupal\comment\Tests\CommentAdminTest::testApprovalAdminInterface.

What should the test check? I've writed / experimented with some test so I'm able to write it but I'm not sure what to test. Should it just check (like CommentAdminTest::testCommentAdmin) that it returns 200? And check if the comment popups up when you add a comment? And after removing the content, check if it displays 'No comments available' again?

Reuploaded the patch WITH content this time ;)

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

I think Tim means to add a test that validation fails with message 'Select one or more comments to perform the update on.'
OTOH the scope of the issue to convert things, not to add new tests

EDIT: There's #1847540: [META] Clean up comment module tests and decouple from node to clean-up comment module and its tests

StatusFileSize
new3.04 KB
new29.81 KB
PASSED: [[SimpleTest]]: [MySQL] 60,193 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Thanks andypost, that's exactly what I meant.

Looks good!

94: comment-admin-1978904-94.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 94: comment-admin-1978904-94.patch, failed testing.

Status:Needs work» Needs review

94: comment-admin-1978904-94.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Still applies cleanly

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Yeah!

Thank you @webchick for committing this and thank you @andypost, @h3rj4n, @larowlan, @shanethehat, @pcambra and @Xano for the patches :)

Status:Fixed» Closed (fixed)

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