Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Deleting nodes via devel generate does not delete the comments on those nodes.
Comment | File | Size | Author |
---|---|---|---|
#46 | D7-tests-for-comment-deletion-890790-46.patch | 1.16 KB | Tor Arne Thune |
#44 | d8-tests-for-comment-deletion-890790-44.patch | 25.19 KB | Tor Arne Thune |
#44 | interdiff-890790-43-44.txt | 491 bytes | Tor Arne Thune |
#43 | d8-tests-for-comment-deletion-890790-43.patch | 25.23 KB | Tor Arne Thune |
#43 | interdiff-890790-40-43.txt | 520 bytes | Tor Arne Thune |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedOh 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.
Comment #2
scor CreditAttribution: scor commenteddebug statement to remove.
whitespace issue
patch works otherwise.
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedfixed those. thanks.
Comment #4
scor CreditAttribution: scor commentedlooks good.
Comment #5
webchickNicely 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!
Comment #6
joachim CreditAttribution: joachim commentedFollow-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.
Comment #7
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #8
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedI'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.
Comment #10
Tor Arne Thune CreditAttribution: Tor Arne Thune commented*coughs*
Comment #12
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedMhmm, bill me for the wasted testbot server time.
Comment #14
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedLet's try with a user that has permission to post comments *blushes*
Comment #16
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #17
ramlev CreditAttribution: ramlev commentedThe test works like it's supposed to.
Comment #18
catchThe 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.
Comment #19
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedSure, makes sense. I'll reroll it.
Comment #20
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedI'm probably doing something fundamentally wrong here. It's not working.
Comment #22
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedNot working with CommentHelperCase (now named CommentTestBase) namespaced either.
Comment #24
dags CreditAttribution: dags commentedComment #25
dags CreditAttribution: dags commentedThe use path was incomplete. Let's see if this works.
Comment #26
dags CreditAttribution: dags commentedComment #28
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedOh yes, right you are. It will need to be changed in
core/modules/comment/lib/Drupal/comment/Tests/CommentNodeChangesTest.php
as well.Comment #29
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedAlso, it looks like tests for other core modules are using CommentHelperCase, so those need to be updated as well:
Comment #30
dags CreditAttribution: dags commentedChanges from #28 & #29. Interdiff against #22.
Comment #31
dags CreditAttribution: dags commentedI always forget this part...
Comment #33
dags CreditAttribution: dags commentedOff by one letter.
Comment #34
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedAlmost there.
Should be CommentTestBase. Also the
rdf.test
file should haveuse Drupal\comment\Tests\CommentTestBase;
as well.Comment #35
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedSorry, cross-post.
Comment #37
dags CreditAttribution: dags commentedYikes. Once again with the changes from #34.
Comment #39
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedYour last patch also reverted the CommentTestCase -> CommentTestBase change.
Comment #40
dags CreditAttribution: dags commentedCrossing fingers...
Comment #41
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedNice. Thanks for the help on this. It should be ready for catch again.
Comment #42
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedOpened #1588284: Convert comment tests to PSR-0 to convert the rest of the comment tests to PSR-0.
Comment #43
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedJust removing an unneeded use statement.
Comment #44
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedBerdir 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.Comment #45
catchSorry 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.
Comment #46
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedThanks. 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.
Comment #47
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedSetting 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.
Comment #48
star-szrBackport looks good to me.
Comment #49
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/ecf40c6
Comment #50
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #51
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedSetting back to original priority of this bug. (See #5.)