Support from Acquia helps fund testing for Drupal Acquia logo

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
FileSize
1.85 KB

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
FileSize
1.31 KB

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!

joachim’s picture

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
FileSize
1.14 KB

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
FileSize
1.11 KB

*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
FileSize
1.14 KB

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
FileSize
1.18 KB

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
FileSize
1.18 KB
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
FileSize
1.31 KB

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
FileSize
24.07 KB

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

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

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
FileSize
1.9 KB
25.06 KB

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
FileSize
2.11 KB
25.27 KB

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
FileSize
2.11 KB
25.27 KB

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

Just removing an unneeded use statement.

Tor Arne Thune’s picture

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
FileSize
1.16 KB

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.

star-szr’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.