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

CommentFileSizeAuthor
#94 comment-admin-1978904-94.patch29.81 KBandypost
#94 interdiff.txt3.04 KBandypost
#92 interdiff.txt874 bytesh3rj4n
#92 comment-admin-1978904-92.patch28.75 KBh3rj4n
#90 comment-admin-1978904-90.patch0 bytesh3rj4n
#90 interdiff.txt874 bytesh3rj4n
#87 comment-admin-1978904-87.patch28.74 KBpcambra
#85 comment-admin.85.patch28.99 KBlarowlan
#82 1978904-comment_admin-82.patch28.98 KBandypost
#82 interdiff.txt1.25 KBandypost
#80 drupal_1978904_80.patch28.71 KBandypost
#78 drupal_1978904_78.patch21.09 KBXano
#73 1978904-comment_admin-71.patch28.68 KBandypost
#73 interdiff.txt888 bytesandypost
#69 interdiff.txt4.65 KBandypost
#69 1978904-comment_admin-69.patch28.61 KBandypost
#65 interdiff.txt1.64 KBandypost
#65 drupal8.comment-module.1978904-65.patch27.55 KBandypost
#64 comment-admin.61.patch27.59 KBandypost
#62 drupal8.comment-module.1978904-62.patch27.5 KBjibran
#62 interdiff.txt897 bytesjibran
#61 comment-admin.61.patch27.59 KBandypost
#54 interdiff.txt1.28 KBlarowlan
#54 comment-admin.54.patch27.6 KBlarowlan
#50 drupal8.comment-module.1978904-50.patch27.23 KBjibran
#50 interdiff.txt1.98 KBjibran
#48 drupal8.comment-module.1978904-48.patch27.24 KBjibran
#48 interdiff.txt6.51 KBjibran
#46 drupal8.comment-module.1978904-46.patch27.97 KBjibran
#46 interdiff.txt6.03 KBjibran
#42 drupal8.comment-module.1978904-42.patch28.09 KBjibran
#42 interdiff.txt2.81 KBjibran
#41 Screenshot 2013-10-11 05.56.19.png83.03 KBlarowlan
#39 drupal8.comment-module.1978904-39.patch27.9 KBjibran
#39 interdiff.txt4.54 KBjibran
#37 drupal8.comment-module.1978904-37.patch24.94 KBjibran
#37 interdiff.txt2.89 KBjibran
#34 drupal8.comment-module.1978904-34.patch24.7 KBjibran
#34 interdiff.txt772 bytesjibran
#30 drupal8.comment-module.1978904-30.patch24.68 KBjibran
#30 interdiff.txt804 bytesjibran
#28 drupal8.comment-module.1978904-28.patch24.67 KBjibran
#28 interdiff.txt885 bytesjibran
#26 drupal8.comment-module.1978904-26.patch24.67 KBjibran
#22 drupal8.comment-module.1978904-21-do-not-test.patch24.62 KBjibran
#21 drupal8.comment-module.1978904-21.patch22.56 KBjibran
#21 interdiff.txt3.84 KBjibran
#19 drupal8.comment-module.1978904-19.patch20.91 KBjibran
#19 interdiff.txt1.92 KBjibran
#16 drupal8.comment-module.1978904-15.patch20.86 KBjibran
#4 comment-admin-controller-1978904-4.patch16.67 KBshanethehat
#4 interdiff.txt15.33 KBshanethehat
#1 comment-admin-controller-1978904-1.patch3.38 KBshanethehat
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

shanethehat’s picture

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

shanethehat’s picture

Status: Active » Needs review

Status: Needs review » Needs work

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

shanethehat’s picture

Status: Needs work » Needs review
FileSize
15.33 KB
16.67 KB

Status: Needs review » Needs work

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

shanethehat’s picture

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.

tim.plunkett’s picture

Fixing tags

kim.pepper’s picture

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

kim.pepper’s picture

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.

tim.plunkett’s picture

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.

tim.plunkett’s picture

Issue summary: View changes

Added link to blocked issue

kim.pepper’s picture

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.

pcambra’s picture

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

andypost’s picture

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

jibran’s picture

Assigned: Unassigned » jibran

Working on it.

andypost’s picture

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

jibran’s picture

Assigned: jibran » Unassigned
Status: Needs work » Needs review
Issue tags: +FormBase
FileSize
20.86 KB

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

larowlan’s picture

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.

jibran’s picture

Status: Needs work » Needs review
FileSize
1.92 KB
20.91 KB

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

andypost’s picture

  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()

jibran’s picture

Thanks for the review @andypost. Fixed #20.

jibran’s picture

andypost’s picture

Status: Needs review » Reviewed & tested by the community

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

andypost’s picture

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

#22 needs re-roll

jibran’s picture

Assigned: Unassigned » jibran

Working on the reroll.

jibran’s picture

Assigned: jibran » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
24.67 KB

Reroll of #22.

Status: Needs review » Needs work

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

jibran’s picture

Status: Needs work » Needs review
FileSize
885 bytes
24.67 KB

blahhh same fail as #16

andypost’s picture

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

jibran’s picture

Fixed #29.

Status: Needs review » Needs work

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

andypost’s picture

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'.

andypost’s picture

Status: Needs work » Needs review
jibran’s picture

andypost’s picture

Status: Needs review » Reviewed & tested by the community

@jibran thank you!

jibran’s picture

jibran’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.89 KB
24.94 KB

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.

jibran’s picture

Status: Needs work » Needs review
FileSize
4.54 KB
27.9 KB

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

larowlan’s picture

+++ 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

larowlan’s picture

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

jibran’s picture

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.

larowlan’s picture

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.

larowlan’s picture

Issue summary: View changes

Added link to 1986606

andypost’s picture

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

andypost’s picture

+++ /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

jibran’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI, +WSCCI Conversions
FileSize
6.03 KB
27.97 KB

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 :(.

andypost’s picture

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

jibran’s picture

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.

jibran’s picture

Status: Needs work » Needs review
FileSize
1.98 KB
27.23 KB

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

larowlan’s picture

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.

jibran’s picture

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 :).

larowlan’s picture

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.

larowlan’s picture

FileSize
27.6 KB
1.28 KB

Like this

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

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

jibran’s picture

+++ 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.

kim.pepper’s picture

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

larowlan’s picture

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.

jibran’s picture

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

lesser evil
andypost’s picture

+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 CommentStorage method

andypost’s picture

jibran’s picture

Status: Reviewed & tested by the community » Needs work

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

andypost’s picture

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

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: Deprecate comment_uri()

+++ 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

andypost’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
27.55 KB
1.64 KB

@jibran was right, we need 2 different URIs here

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

jibran’s picture

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

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

jibran’s picture

Issue tags: +Needs reroll

:/ patch no longer applies.

jibran’s picture

Issue summary: View changes

added note about sorting

andypost’s picture

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

andypost’s picture

+++ 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

jibran’s picture

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.

jibran’s picture

+++ 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().

andypost’s picture

Status: Needs work » Needs review
FileSize
888 bytes
28.68 KB

here it is

jibran’s picture

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

larowlan’s picture

Status: Needs review » Reviewed & tested by the community
Xano’s picture

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.

Xano’s picture

Status: Needs work » Needs review
FileSize
21.09 KB

Re-roll.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the reroll @Xano. Back to RTBC.

andypost’s picture

FileSize
28.71 KB

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

Xano’s picture

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

andypost’s picture

jibran’s picture

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.

larowlan’s picture

Status: Needs work » Needs review
FileSize
28.99 KB

reroll

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

pcambra’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
28.74 KB
andypost’s picture

Status: Needs review » Reviewed & tested by the community

RTBC again

tim.plunkett’s picture

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)

h3rj4n’s picture

Status: Needs work » Needs review
FileSize
874 bytes
0 bytes

Changed form_set_error to setFormError().

tim.plunkett’s picture

Status: Needs review » Needs work

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

h3rj4n’s picture

FileSize
28.75 KB
874 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 ;)

andypost’s picture

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

andypost’s picture

FileSize
3.04 KB
29.81 KB
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks andypost, that's exactly what I meant.

Looks good!

xjm’s picture

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.

jibran’s picture

Status: Needs work » Needs review

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

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Still applies cleanly

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

larowlan’s picture

Yeah!

jibran’s picture

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.