Files: 
CommentFileSizeAuthor
#49 1946348-49.patch12.96 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,456 pass(es).
[ View ]
#49 interdiff.txt403 bytesjibran
#48 1946348-48.patch13.36 KBjibran
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#48 interdiff.txt431 bytesjibran
#46 1946348-46.patch13.36 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,525 pass(es).
[ View ]
#46 interdiff.txt1.03 KBjibran
#43 1946348-43.patch13.51 KBjibran
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Entity/EntityNGConfirmFormBase.php.
[ View ]
#40 1946348-40.patch13.51 KBjibran
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Entity/EntityNGConfirmFormBase.php.
[ View ]
#40 interdiff.txt3.1 KBjibran
#37 1946348-37.patch13.46 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,159 pass(es).
[ View ]
#37 interdiff.txt996 bytesjibran
#34 comment-1946348-34.patch13.45 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,864 pass(es).
[ View ]
#34 interdiff.txt11.82 KBtim.plunkett
#31 1946348-comment-confirm-form-30.patch12.95 KBDavid Hernández
FAILED: [[SimpleTest]]: [MySQL] 57,146 pass(es), 10 fail(s), and 6 exception(s).
[ View ]
#23 1946348-comment-confirm-form-23.patch12.62 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 55,406 pass(es), 12 fail(s), and 6 exception(s).
[ View ]
#23 interdiff.txt1.39 KBkim.pepper
#19 1946348-comment-confirm-form-19.patch12.6 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 55,562 pass(es), 6 fail(s), and 10 exception(s).
[ View ]
#19 interdiff.txt5.92 KBkim.pepper
#17 convert-comment-confirm-form-1946348-17.patch11.21 KBsplatio
PASSED: [[SimpleTest]]: [MySQL] 54,479 pass(es).
[ View ]
#15 convert-comment-confirm-form-1946348-15.patch7.04 KBsplatio
PASSED: [[SimpleTest]]: [MySQL] 54,430 pass(es).
[ View ]
#12 convert-comment-confirm-form-1946348-12.patch11.19 KBsplatio
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert-comment-confirm-form-1946348-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 convert-comment-confirm-form-1946348-8.patch11.83 KBsplatio
PASSED: [[SimpleTest]]: [MySQL] 54,063 pass(es).
[ View ]
#6 convert-comment-confirm-form-1946348-6.patch11.83 KBsplatio
PASSED: [[SimpleTest]]: [MySQL] 53,898 pass(es).
[ View ]
#3 convert-comment-confirm-form-1946348-3.patch11.66 KBsplatio
PASSED: [[SimpleTest]]: [MySQL] 53,575 pass(es).
[ View ]

Comments

mtift’s picture

Issue tags:+WSCCI-conversion

Anything that's "take something from hook_menu and make a route of it" should get the WSCCI-conversion tag

splatio’s picture

Assigned:Unassigned» splatio
splatio’s picture

StatusFileSize
new11.66 KB
PASSED: [[SimpleTest]]: [MySQL] 53,575 pass(es).
[ View ]

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

splatio’s picture

Status:Active» Needs review
Crell’s picture

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

splatio’s picture

StatusFileSize
new11.83 KB
PASSED: [[SimpleTest]]: [MySQL] 53,898 pass(es).
[ View ]

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.

Crell’s picture

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

splatio’s picture

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.

splatio’s picture

StatusFileSize
new11.83 KB
PASSED: [[SimpleTest]]: [MySQL] 54,063 pass(es).
[ View ]

Oh, here's the patch!

Crell’s picture

Status:Needs review» Reviewed & tested by the community

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

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

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

splatio’s picture

Status:Needs work» Needs review
StatusFileSize
new11.19 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch convert-comment-confirm-form-1946348-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

ParisLiakos’s picture

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

The last submitted patch, convert-comment-confirm-form-1946348-12.patch, failed testing.

splatio’s picture

Status:Needs work» Needs review
StatusFileSize
new7.04 KB
PASSED: [[SimpleTest]]: [MySQL] 54,430 pass(es).
[ View ]

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

ParisLiakos’s picture

Status:Needs review» Needs work

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?

splatio’s picture

Status:Needs work» Needs review
StatusFileSize
new11.21 KB
PASSED: [[SimpleTest]]: [MySQL] 54,479 pass(es).
[ View ]

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.

ParisLiakos’s picture

Status:Needs review» Needs work

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

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new5.92 KB
new12.6 KB
FAILED: [[SimpleTest]]: [MySQL] 55,562 pass(es), 6 fail(s), and 10 exception(s).
[ View ]

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

kim.pepper’s picture

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

ParisLiakos’s picture

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

Status:Needs review» Needs work

The last submitted patch, 1946348-comment-confirm-form-19.patch, failed testing.

kim.pepper’s picture

Status:Needs work» Needs review
Issue tags:+Stalking Crell
StatusFileSize
new1.39 KB
new12.62 KB
FAILED: [[SimpleTest]]: [MySQL] 55,406 pass(es), 12 fail(s), and 6 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, 1946348-comment-confirm-form-23.patch, failed testing.

Crell’s picture

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

kim.pepper’s picture

Status:Needs work» Postponed

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.

kim.pepper’s picture

Issue summary:View changes

Linked to blocking issue

kim.pepper’s picture

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

tim.plunkett’s picture

Assigned:splatio» Unassigned
Status:Closed (duplicate)» Active

This is still needed for comment_confirm_delete_page().

David Hernández’s picture

Assigned:Unassigned» David Hernández

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

David Hernández’s picture

StatusFileSize
new12.95 KB
FAILED: [[SimpleTest]]: [MySQL] 57,146 pass(es), 10 fail(s), and 6 exception(s).
[ View ]

Oops, forgot the patch.

David Hernández’s picture

Status:Active» Needs review

updated status, to see the test results.

Status:Needs review» Needs work

The last submitted patch, 1946348-comment-confirm-form-30.patch, failed testing.

tim.plunkett’s picture

Assigned:David Hernández» Unassigned
Status:Needs work» Needs review
Issue tags:-Stalking Crell
StatusFileSize
new11.82 KB
new13.45 KB
PASSED: [[SimpleTest]]: [MySQL] 57,864 pass(es).
[ View ]

Modernized this a bit.

Crell’s picture

Status:Needs review» Reviewed & tested by the community

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

Cottser’s picture

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

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.

jibran’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new996 bytes
new13.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,159 pass(es).
[ View ]

Reroll and minor cleanup.

Crell’s picture

Status:Needs review» Reviewed & tested by the community

And back.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ 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

jibran’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new3.1 KB
new13.51 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Entity/EntityNGConfirmFormBase.php.
[ View ]

Fixed #39.

kim.pepper’s picture

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

Status:Reviewed & tested by the community» Needs work
Issue tags:+FormInterface, +WSCCI-conversion

The last submitted patch, 1946348-40.patch, failed testing.

jibran’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new13.51 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/lib/Drupal/Core/Entity/EntityNGConfirmFormBase.php.
[ View ]

Reroll once more.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1946348-43.patch, failed testing.

tim.plunkett’s picture

  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

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new1.03 KB
new13.36 KB
PASSED: [[SimpleTest]]: [MySQL] 58,525 pass(es).
[ View ]

Fixed #45.

tim.plunkett’s picture

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

Where is the local task going?

jibran’s picture

StatusFileSize
new431 bytes
new13.36 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Where is the local task going?

Fixed.

jibran’s picture

StatusFileSize
new403 bytes
new12.96 KB
PASSED: [[SimpleTest]]: [MySQL] 58,456 pass(es).
[ View ]

Reverted unwanted change in EntityNGConfirmFormBase.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

Looks good to me, thanks!

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary:View changes

Added link to 1986606