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

moshe weitzman’s picture

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.

scor’s picture

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.

moshe weitzman’s picture

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

fixed those. thanks.

scor’s picture

Status:Needs review» Reviewed & tested by the community

looks good.

webchick’s picture

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!

Tor Arne Thune’s picture

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
Tor Arne Thune’s picture

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.

Tor Arne Thune’s picture

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.

Tor Arne Thune’s picture

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.

Tor Arne Thune’s picture

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.

Tor Arne Thune’s picture

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

Status:Needs review» Reviewed & tested by the community

The test works like it's supposed to.

catch’s picture

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.

Tor Arne Thune’s picture

Sure, makes sense. I'll reroll it.

Tor Arne Thune’s picture

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.

Tor Arne Thune’s picture

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.

dags’s picture

Assigned:moshe weitzman» dags
dags’s picture

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.

dags’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Tor Arne Thune’s picture

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

Tor Arne Thune’s picture

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
dags’s picture

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.

dags’s picture

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.

dags’s picture

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.

Tor Arne Thune’s picture

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.

Tor Arne Thune’s picture

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.

dags’s picture

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.

Tor Arne Thune’s picture

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

dags’s picture

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

Crossing fingers...

Tor Arne Thune’s picture

Status:Needs review» Reviewed & tested by the community

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

Tor Arne Thune’s picture

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

Tor Arne Thune’s picture

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.

Tor Arne Thune’s picture

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.

catch’s picture

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.

Tor Arne Thune’s picture

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.

Tor Arne Thune’s picture

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.

Cottser’s picture

Backport looks good to me.

David_Rothstein’s picture

Status:Reviewed & tested by the community» Fixed
Tor Arne Thune’s picture

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
Tor Arne Thune’s picture

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.