Posted by jenlampton on August 23, 2010 at 11:35am
12 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | comment.module |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | needs backport to D7, Novice |
Issue Summary
Deleting nodes via devel generate does not delete the comments on those nodes.
Comments
#1
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.
#2
+++ 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.
#3
fixed those. thanks.
#4
looks good.
#5
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!
#6
Follow-on: this broke tracker: #921944: error messages on tracker page - tracker_node_delete() does not correctly act on node deletion. And also docs #988030: Document correct sequence of hooks in node deletion.
#7
#8
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.
#9
The last submitted patch, D8-tests-for-comment-deletion-890790-8.patch, failed testing.
#10
*coughs*
#11
The last submitted patch, D8-tests-for-comment-deletion-890790-10.patch, failed testing.
#12
Mhmm, bill me for the wasted testbot server time.
#13
The last submitted patch, D8-tests-for-comment-deletion-890790-12.patch, failed testing.
#14
Let's try with a user that has permission to post comments *blushes*
#15
The last submitted patch, D8-tests-for-comment-deletion-890790-14.patch, failed testing.
#16
#17
The test works like it's supposed to.
#18
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.
#19
Sure, makes sense. I'll reroll it.
#20
I'm probably doing something fundamentally wrong here. It's not working.
#21
The last submitted patch, D8-tests-for-comment-deletion-890790-20.patch, failed testing.
#22
Not working with CommentHelperCase (now named CommentTestBase) namespaced either.
#23
The last submitted patch, D8-tests-for-comment-deletion-890790-22.patch, failed testing.
#24
#25
The use path was incomplete. Let's see if this works.
#26
#27
The last submitted patch, d8-tests-for-comment-deletion-890790-25.patch, failed testing.
#28
Oh yes, right you are. It will need to be changed in
core/modules/comment/lib/Drupal/comment/Tests/CommentNodeChangesTest.phpas well.#29
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
#30
Changes from #28 & #29. Interdiff against #22.
#31
I always forget this part...
#32
The last submitted patch, d8-tests-for-comment-deletion-890790-30.patch, failed testing.
#33
Off by one letter.
#34
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];
rdf.testfile should haveuse Drupal\comment\Tests\CommentTestBase;as well.#35
Sorry, cross-post.
#36
The last submitted patch, d8-tests-for-comment-deletion-890790-33.patch, failed testing.
#37
Yikes. Once again with the changes from #34.
#38
The last submitted patch, d8-tests-for-comment-deletion-890790-37.patch, failed testing.
#39
Your last patch also reverted the CommentTestCase -> CommentTestBase change.
#40
Crossing fingers...
#41
Nice. Thanks for the help on this. It should be ready for catch again.
#42
Opened #1588284: Convert comment tests to PSR-0 to convert the rest of the comment tests to PSR-0.
#43
Just removing an unneeded use statement.
#44
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.#45
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.
#46
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.
#47
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.
#48
Backport looks good to me.
#49
Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/ecf40c6
#50
#51
Setting back to original priority of this bug. (See #5.)
#52
Automatically closed -- issue fixed for 2 weeks with no activity.