Convert all of confirm_form() in comment.admin.inc to the new form interface

This patch converts the two instances of confirm_form() into the new FormInterface and adds a route for /comment/{comment}/delete.

+++ b/core/modules/comment/comment.admin.inc
@@ -22,7 +22,7 @@ function comment_admin($type = 'new') {
+    return drupal_get_form(new Drupal\comment\Form\ConfirmDeleteMultiple());

Classes should be "use"d at the top of the file to avoid needing to inline the entire namespace.

+++ b/core/modules/comment/lib/Drupal/comment/Form/ConfirmDeleteMultiple.php
@@ -0,0 +1,97 @@
+        $subject = db_query('SELECT subject FROM {comment} WHERE cid = :cid', array(':cid' => $cid))->fetchField();

It pains me every time I see this, because we can't fix it yet. :-(

Thanks for the review, here's an updated patch with ConfirmDeleteMultiple use'd at the top. I think I'd just copied blindly from the change notice. A few lines like that stuck out at me as well, but I guessed this wasn't the time/place to refactor it.

+++ b/core/modules/comment/comment.module
@@ -270,11 +270,8 @@ function comment_menu() {
+    'route name' => 'comment_confirm_delete',

This should be route_name, not "route name". I think the menu system may be hiding the difference, but elsewhere we've been including the _.

I think that's all that's left? (Sorry, I hate it when I don't find things on the first pass.)

No problem, here's an updated patch that changes route name to route_name. It does seem like a bit of an inconsistency compared to the rest of the keys in hook_menu though.

Oh, here's the patch!

OK, let's get this in. Thanks, splatio!

Looks great... we just have a new documentation standard to apply..

+++ b/core/modules/comment/lib/Drupal/comment/Form/ConfirmDelete.phpundefined
@@ -0,0 +1,82 @@
+   * @var \Drupal\comment\Plugin\Core\Entity\Comment
...
+   * Implements \Drupal\Core\Form\FormInterface::getFormID().
...
+   * Implements \Drupal\Core\Form\ConfirmFormBase::getQuestion().
...
+   * Implements \Drupal\Core\Form\ConfirmFormBase::getCancelPath().
...
+   * Implements \Drupal\Core\Form\ConfirmFormBase::getDescritpion().
...
+   * Implements \Drupal\Core\Form\ConfirmFormBase::getConfirmText().
...
+   * Implements \Drupal\Core\Form\FormInterface::buildForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::submitForm().

+++ b/core/modules/comment/lib/Drupal/comment/Form/ConfirmDeleteMultiple.phpundefined
@@ -0,0 +1,97 @@
+   * Implements \Drupal\Core\Form\FormInterface::getFormID().
...
+   * Implements \Drupal\Core\Form\ConfirmFormBase::getQuestion().
...
+   * Implements \Drupal\Core\Form\ConfirmFormBase::getCancelPath().
...
+   * Implements \Drupal\Core\Form\ConfirmFormBase::getDescritpion().
...
+   * Implements \Drupal\Core\Form\ConfirmFormBase::getConfirmText().
...
+   * Implements \Drupal\Core\Form\FormInterface::buildForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::submitForm().

Should be {@inheritdoc} see http://drupal.org/coding-standards/docs#inheritdoc

Here's the patch with {@inheritdoc} replacing all Implements... Should still apply cleanly to HEAD.

#12: convert-comment-confirm-form-1946348-12.patch queued for re-testing.

OK, looks like it needed re-rolling after all!

i believe the patch misses comment.admin.inc function deletions?

+++ b/core/modules/comment/comment.moduleundefined
@@ -272,12 +272,8 @@ function comment_menu() {
-    'access callback' => 'entity_page_access',
-    'access arguments' => array(1, 'delete'),

+++ b/core/modules/comment/comment.routing.ymlundefined
@@ -0,0 +1,6 @@
+  requirements:
+    _permission: 'administer comments'

also not quite sure how these are equal?

I missed the deletions when re-rolling the patch, should have been obvious from the file sizes!

The route access has changed since the earlier patches, I'm not sure if we should wait for #1947432: Add a generic EntityAccessCheck to replace entity_page_access() or implement our own access check class in the mean time.

#1947432: Add a generic EntityAccessCheck to replace entity_page_access() is in:)
Also:

+++ b/core/modules/comment/lib/Drupal/comment/Form/ConfirmDelete.phpundefined
@@ -0,0 +1,82 @@
+    cache_invalidate_tags(array('content' => TRUE));

+++ b/core/modules/comment/lib/Drupal/comment/Form/ConfirmDeleteMultiple.phpundefined
@@ -0,0 +1,97 @@
+      cache_invalidate_tags(array('content' => TRUE));

Should be Cache::invalidateTags() after you add a use statement for \Drupal\Core\Cache\Cache

+++ b/core/modules/comment/lib/Drupal/comment/Form/ConfirmDeleteMultiple.phpundefined
@@ -0,0 +1,97 @@
+      $comment = comment_load($cid);
...
+      comment_delete_multiple(array_keys($form_state['values']['comments']));

you should inject plugin.manager.entity and use it instead

+++ b/core/modules/comment/lib/Drupal/comment/Form/ConfirmDeleteMultiple.phpundefined
@@ -0,0 +1,97 @@
+        $subject = db_query('SELECT subject FROM {comment} WHERE cid = :cid', array(':cid' => $cid))->fetchField();

instead of db_query, inject the database service:)

+++ b/core/modules/comment/lib/Drupal/comment/Form/ConfirmDeleteMultiple.phpundefined
@@ -0,0 +1,97 @@
+      drupal_goto('admin/content/comment');

should be new RedirectResponse(url('admin/content/comment', array('absolute' => TRUE)))..

Added entity access permission, injected all the things, etc. as per #18

+++ b/core/modules/comment/comment.admin.incundefined
@@ -22,7 +23,7 @@ function comment_admin($type = 'new') {
+    return drupal_get_form(new ConfirmDeleteMultiple());

Just realised this is going to cause issues. Should we make it a service?

i think doing this
ConfirmDeleteMultiple::create(Drupal::getContainer()) in drupal_get_form
would work.

+++ b/core/modules/comment/lib/Drupal/comment/Form/ConfirmDeleteMultiple.phpundefined
@@ -0,0 +1,144 @@
+      return RedirectResponse(url('admin/content/comment', array('absolute' => TRUE)));

i am having second thoughts for this.we are in a form so lets use $form_state['redirect'];
sth like

$form_state['redirect'] = 'admin/content/comment';
return $form;

Does it work that way? If yes then it would be perfect:)

ConfirmDeleteMultiple::create(Drupal::getContainer()) in drupal_get_form

I'm not sure I've seen that pattern before. Seeing what the bot says, but I'm also invoking @Crell for some insight into this.

+++ b/core/modules/comment/comment.admin.inc
@@ -6,6 +6,7 @@
@@ -22,7 +23,7 @@ function comment_admin($type = 'new') {

@@ -22,7 +23,7 @@ function comment_admin($type = 'new') {
   $edit = $_POST;
 
   if (isset($edit['operation']) && ($edit['operation'] == 'delete') && isset($edit['comments']) && $edit['comments']) {
-    return drupal_get_form('comment_multiple_delete_confirm');
+    return drupal_get_form(ConfirmDeleteMultiple::create(Drupal::getContainer()));
   }
   else {
     return drupal_get_form('comment_admin_overview', $type);

This is ugly, but at this point in the code there isn't really an alternative.

The better approach would be to convert comment_admin() to a controller now as well. Then the controller class's create() method will have the container available, and it can (get this), create the form on the spot and pass it into the constructor, so the controller can just return drupal_get_form($this->confirmDeleteMultipleForm).

(That may not be the best design, but it sounds cool to describe it at least!)

Alternatively, refactor the paths so that comment_admin() isn't fronting for 2 different forms, and we can then wire the forms directly to the appropriate route. What we're doing here with $_POST is an affront to the gods anyway.

Over at #1978904: Convert comment_admin() to a Controller we are converting the comment_admin to a controller. It seems to be blocked on this issue.

However, I think this issue should wait until that issue is in. We can move out the crappy $_POST stuff over there.

Linked to blocking issue

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.

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

This is still needed for comment_confirm_delete_page().

Re-roll and now I'm trying to convert comment_confirm_delete_page

Oops, forgot the patch.

updated status, to see the test results.

Modernized this a bit.

Is there anything relating to an entity that hasn't been generalized? :-)

Needs a reroll…

Checking patch core/lib/Drupal/Core/Entity/EntityNGConfirmFormBase.php...
Checking patch core/modules/comment/comment.admin.inc...
error: while searching for:
 */

use Drupal\comment\Plugin\Core\Entity\Comment;
use Symfony\Component\HttpFoundation\RedirectResponse;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;


error: patch failed: core/modules/comment/comment.admin.inc:6
error: core/modules/comment/comment.admin.inc: patch does not apply
Checking patch core/modules/comment/comment.module...
Checking patch core/modules/comment/comment.routing.yml...
Checking patch core/modules/comment/lib/Drupal/comment/Form/ConfirmDeleteMultiple.php...
Checking patch core/modules/comment/lib/Drupal/comment/Form/DeleteForm.php...
Checking patch core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.php...
error: core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.php: No such file or directory

+++ b/core/modules/comment/comment.routing.yml
@@ -19,3 +19,9 @@ comment_permalink:
+comment_confirm_delete:
+  pattern: 'comment/{comment}/delete'
+  defaults:
+    _entity_form: 'comment.delete'
+  requirements:
+    _entity_access: 'comment.delete'
\ No newline at end of file

Also, newline at EOF needed I think.

Reroll and minor cleanup.

And back.

+++ b/core/modules/comment/lib/Drupal/comment/Form/ConfirmDeleteMultiple.phpundefined
@@ -0,0 +1,132 @@
+    return t('Are you sure you want to delete these comments and all their children?');
...
+    return t('Delete comments');
...
+      drupal_set_message(t('There do not appear to be any comments to delete, or your selected comment was deleted by another administrator.'));

+++ b/core/modules/comment/lib/Drupal/comment/Form/DeleteForm.phpundefined
@@ -0,0 +1,61 @@
+    return t('Are you sure you want to delete the comment %title?', array('%title' => $this->entity->subject->value));
...
+    return t('Any replies to this comment will be lost. This action cannot be undone.');
...
+    return t('Delete');
...
+    drupal_set_message(t('The comment and all its replies have been deleted.'));

Let's use $this->t from FormBase

Fixed #39.

#40: 1946348-40.patch queued for re-testing.

Reroll once more.

1. +++ b/core/lib/Drupal/Core/Entity/EntityNGConfirmFormBase.php
   @@ -113,4 +113,12 @@ protected function actions(array $form, array &$form_state) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function form(array $form, array &$form_state) {
   +    // Do not attach fields to a confirm form.
   +    return $form;
   +  }
   

   This was already added in another issue.

2. +++ b/core/modules/comment/comment.routing.yml
   @@ -19,3 +19,9 @@ comment_permalink:
   +  pattern: 'comment/{comment}/delete'
   

   '/comment

Fixed #45.

+++ b/core/modules/comment/comment.module
@@ -224,12 +224,7 @@ function comment_menu() {
-    'type' => MENU_LOCAL_TASK,

Where is the local task going?

Where is the local task going?

Fixed.

Reverted unwanted change in EntityNGConfirmFormBase.

Looks good to me, thanks!

Committed and pushed to 8.x. Thanks!