Deleting nodes via devel generate does not delete the comments on those nodes.

Files: 
CommentFileSizeAuthor
#46 D7-tests-for-comment-deletion-890790-46.patch1.16 KBTor Arne Thune
PASSED: [[SimpleTest]]: [MySQL] 39,106 pass(es).
[ View ]
#44 d8-tests-for-comment-deletion-890790-44.patch25.19 KBTor Arne Thune
PASSED: [[SimpleTest]]: [MySQL] 36,645 pass(es).
[ View ]
#44 interdiff-890790-43-44.txt491 bytesTor Arne Thune
#43 d8-tests-for-comment-deletion-890790-43.patch25.23 KBTor Arne Thune
PASSED: [[SimpleTest]]: [MySQL] 36,640 pass(es).
[ View ]
#43 interdiff-890790-40-43.txt520 bytesTor Arne Thune
#40 d8-tests-for-comment-deletion-890790-40.patch25.27 KBdags
PASSED: [[SimpleTest]]: [MySQL] 36,626 pass(es).
[ View ]
#40 interdiff.txt2.11 KBdags
#37 d8-tests-for-comment-deletion-890790-37.patch25.27 KBdags
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#37 interdiff.txt2.11 KBdags
#33 d8-tests-for-comment-deletion-890790-33.patch25.06 KBdags
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#33 interdiff.txt1.9 KBdags
#30 d8-tests-for-comment-deletion-890790-30.patch25.06 KBdags
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#30 interdiff.txt1.9 KBdags
#25 d8-tests-for-comment-deletion-890790-25.patch24.07 KBdags
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#25 interdiff.txt417 bytesdags
#22 D8-tests-for-comment-deletion-890790-22.patch24.07 KBTor Arne Thune
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#20 D8-tests-for-comment-deletion-890790-20.patch1.31 KBTor Arne Thune
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#16 D8-tests-for-comment-deletion-890790-16.patch1.18 KBTor Arne Thune
PASSED: [[SimpleTest]]: [MySQL] 35,418 pass(es).
[ View ]
#14 D8-tests-for-comment-deletion-890790-14.patch1.18 KBTor Arne Thune
FAILED: [[SimpleTest]]: [MySQL] 35,427 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#12 D8-tests-for-comment-deletion-890790-12.patch1.14 KBTor Arne Thune
FAILED: [[SimpleTest]]: [MySQL] 35,393 pass(es), 10 fail(s), and 4 exception(s).
[ View ]
#10 D8-tests-for-comment-deletion-890790-10.patch1.11 KBTor Arne Thune
FAILED: [[SimpleTest]]: [MySQL] 35,409 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#8 D8-tests-for-comment-deletion-890790-8.patch1.14 KBTor Arne Thune
FAILED: [[SimpleTest]]: [MySQL] 35,396 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#3 ndm_reorder.diff1.31 KBmoshe weitzman
PASSED: [[SimpleTest]]: [MySQL] 23,291 pass(es).
[ View ]
#1 ndm_reorder.diff1.85 KBmoshe weitzman
FAILED: [[SimpleTest]]: [MySQL] 23,294 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Comments

Project:Devel» Drupal core
Version:7.x-1.x-dev» 7.x-dev
Component:devel_generate» comment.module
Assigned:Unassigned» moshe weitzman
Priority:Normal» Critical
Status:Active» Needs review
StatusFileSize
new1.85 KB
FAILED: [[SimpleTest]]: [MySQL] 23,294 pass(es), 1 fail(s), and 0 exception(es).
[ View ]

Oh for fucks sake. You just found a critical core bug. Deleting a single node via the UI created orphaned comment records. Maybe D7 isn't so ready after all.

The problem is that we delete from node table and then we call the hook where comment_load_multiple() fires. But it can't load any comments because the node record is gone.

Seems like we should just delete from node table after calling the hooks. Attached patch does that.

Status:Needs review» Needs work

+++ modules/comment/comment.module
@@ -1641,6 +1641,7 @@ class CommentController extends DrupalDefaultEntityController {
+    print_r((string)$query);

debug statement to remove.

+++ modules/node/node.module
@@ -1162,6 +1152,17 @@ function node_delete_multiple($nids) {
+    ¶
+    // Delete after calling hooks so that they can query node tables as needed.
+    db_delete('node')
+      ->condition('nid', $nids, 'IN')
+      ->execute();

whitespace issue

patch works otherwise.

Status:Needs work» Needs review
StatusFileSize
new1.31 KB
PASSED: [[SimpleTest]]: [MySQL] 23,291 pass(es).
[ View ]

fixed those. thanks.

Status:Needs review» Reviewed & tested by the community

looks good.

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

Nicely spotted, Jen. :)

Committed to HEAD to clear out the critical. However, this is a really stupid bug and we should make sure we don't ever have it again. Tests, please!

Title:deleting nodes does not delete their comments.(Tests needed) deleting nodes does not delete their comments.
Version:7.x-dev» 8.x-dev
Category:bug» task
Issue tags:+needs backport to D7

Status:Needs work» Needs review
StatusFileSize
new1.14 KB
FAILED: [[SimpleTest]]: [MySQL] 35,396 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

I'm sick of seeing this issue in my tracker ;) Will this test work? I've generalized the test case, so that other test methods can be added to it that tests if comments behave correctly after a node changes.

Status:Needs review» Needs work

The last submitted patch, D8-tests-for-comment-deletion-890790-8.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.11 KB
FAILED: [[SimpleTest]]: [MySQL] 35,409 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

*coughs*

Status:Needs review» Needs work

The last submitted patch, D8-tests-for-comment-deletion-890790-10.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.14 KB
FAILED: [[SimpleTest]]: [MySQL] 35,393 pass(es), 10 fail(s), and 4 exception(s).
[ View ]

Mhmm, bill me for the wasted testbot server time.

Status:Needs review» Needs work

The last submitted patch, D8-tests-for-comment-deletion-890790-12.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.18 KB
FAILED: [[SimpleTest]]: [MySQL] 35,427 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

Let's try with a user that has permission to post comments *blushes*

Status:Needs review» Needs work

The last submitted patch, D8-tests-for-comment-deletion-890790-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.18 KB
PASSED: [[SimpleTest]]: [MySQL] 35,418 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

The test works like it's supposed to.

Status:Reviewed & tested by the community» Needs work

The test looks good, but now that #1477218: Convert Tracker tests to PSR-0 / PSR-0 test class discovery is in, we should start ensuring that new test classes that are added are PSR-0 so we don't need to go back and convert them again later.

Sure, makes sense. I'll reroll it.

Status:Needs work» Needs review
StatusFileSize
new1.31 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

I'm probably doing something fundamentally wrong here. It's not working.

Status:Needs review» Needs work

The last submitted patch, D8-tests-for-comment-deletion-890790-20.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new24.07 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Not working with CommentHelperCase (now named CommentTestBase) namespaced either.

Status:Needs review» Needs work

The last submitted patch, D8-tests-for-comment-deletion-890790-22.patch, failed testing.

Assigned:moshe weitzman» dags

StatusFileSize
new417 bytes
new24.07 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

The use path was incomplete. Let's see if this works.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, d8-tests-for-comment-deletion-890790-25.patch, failed testing.

Oh yes, right you are. It will need to be changed in core/modules/comment/lib/Drupal/comment/Tests/CommentNodeChangesTest.php as well.

Also, it looks like tests for other core modules are using CommentHelperCase, so those need to be updated as well:

grep -rl "CommentHelperCase" .
./core/modules/rdf/rdf.test
./core/modules/user/user.test

StatusFileSize
new1.9 KB
new25.06 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Changes from #28 & #29. Interdiff against #22.

Status:Needs work» Needs review

I always forget this part...

Status:Needs review» Needs work

The last submitted patch, d8-tests-for-comment-deletion-890790-30.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.9 KB
new25.06 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Off by one letter.

Status:Needs review» Needs work

Almost there.

+++ b/core/modules/rdf/rdf.testundefined
@@ -416,7 +416,7 @@ class RdfMappingDefinitionTestCase extends TaxonomyWebTestCase {
-class RdfCommentAttributesTestCase extends CommentHelperCase {
+class RdfCommentAttributesTestCase extends CommentTestCase {
   public static function getInfo() {
     return array(
diff --git a/core/modules/user/user.test b/core/modules/user/user.test
diff --git a/core/modules/user/user.test b/core/modules/user/user.test
index 7671eba..7818193 100644
index 7671eba..7818193 100644
--- a/core/modules/user/user.test
--- a/core/modules/user/user.test
+++ b/core/modules/user/user.testundefined
+++ b/core/modules/user/user.testundefined
+++ b/core/modules/user/user.testundefined
@@ -1946,7 +1946,7 @@ class UserSignatureTestCase extends WebTestBase {
@@ -1946,7 +1946,7 @@ class UserSignatureTestCase extends WebTestBase {
     $this->drupalPost(NULL, array(), t('Save'));
     // Get the comment ID. (This technique is the same one used in the Comment
-    // module's CommentHelperCase test case.)
+    // module's CommentTestCase test case.)
     preg_match('/#comment-([0-9]+)/', $this->getURL(), $match);
     $comment_id = $match[1];
Should be CommentTestBase. Also the rdf.test file should have use Drupal\comment\Tests\CommentTestBase; as well.

Status:Needs work» Needs review

Sorry, cross-post.

Status:Needs review» Needs work

The last submitted patch, d8-tests-for-comment-deletion-890790-33.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.11 KB
new25.27 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Yikes. Once again with the changes from #34.

Status:Needs review» Needs work

The last submitted patch, d8-tests-for-comment-deletion-890790-37.patch, failed testing.

Your last patch also reverted the CommentTestCase -> CommentTestBase change.

Status:Needs work» Needs review
StatusFileSize
new2.11 KB
new25.27 KB
PASSED: [[SimpleTest]]: [MySQL] 36,626 pass(es).
[ View ]

Crossing fingers...

Status:Needs review» Reviewed & tested by the community

Nice. Thanks for the help on this. It should be ready for catch again.

Opened #1588284: Convert comment tests to PSR-0 to convert the rest of the comment tests to PSR-0.

Assigned:dags» Tor Arne Thune
StatusFileSize
new520 bytes
new25.23 KB
PASSED: [[SimpleTest]]: [MySQL] 36,640 pass(es).
[ View ]

Just removing an unneeded use statement.

StatusFileSize
new491 bytes
new25.19 KB
PASSED: [[SimpleTest]]: [MySQL] 36,645 pass(es).
[ View ]

Berdir pointed out that there is no need for use Drupal\comment\Tests\CommentTestBase; when we're in the same namespace, which of course makes sense.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)
Issue tags:+Novice

Sorry I didn't think about CommentTestBase when asking for PSR-0-ification but thanks! Committed/pushed to 8.x, moving to 7.x for backport.

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.16 KB
PASSED: [[SimpleTest]]: [MySQL] 39,106 pass(es).
[ View ]

Thanks. Learned a lot converting it to follow PSR-0, with the help of davidjdagino. The attached D7 patch is a re-roll of the RTBC patch in comment #16.

Status:Needs review» Reviewed & tested by the community

Setting this to RTBC as the identical patch in #16 was RTBCed in #17. The rest of the discussion is just about the PSR-0 conversion of the D8 version of the patch.

Backport looks good to me.

Status:Reviewed & tested by the community» Fixed

Title:(Tests needed) deleting nodes does not delete their comments.deleting nodes does not delete their comments.
Assigned:Tor Arne Thune» Unassigned
Issue tags:-Needs tests

Priority:Normal» Critical

Setting back to original priority of this bug. (See #5.)

Status:Fixed» Closed (fixed)
Issue tags:-Novice, -needs backport to D7

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