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.
Post a comment as a reply to another comment, check the pid is correct, probably threading too.
Comment | File | Size | Author |
---|---|---|---|
#6 | comment_pid_thread_test.patch | 2.33 KB | jonskulski |
#2 | comment_pid_test.patch | 907 bytes | Anonymous (not verified) |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedWorking on it..
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedSubmitted the patch providing the required patch. One thing, that, in my opinion, should be done in the future, is to rewrite postComment (ant other comment methods) to return the full comment object (I had to resort to using comment_load). As always, if there's anything wrong with the code, please let me know.
Comment #3
catchShould we be checking the thread string as well here?
Comment #4
jonskulski CreditAttribution: jonskulski commentedTest applies and succeeds.
Manual replying to a comment results in a 'notice: Undefined offset: 1 in /home/jskulski/public_html/drupal7/modules/comment/comment.module on line 734.'
$parent_depth = count(explode('.', $parent->thread));
$last = $parts[$parent_depth];
parent depth is always always going to be one greater than parts. It's possible the SQL query is not grabbing the right row, and that results in the thread post number never getting increased).
(Multiple 05.01's instead of 05.01, 05.02, 05.03...)
Comment #5
jonskulski CreditAttribution: jonskulski commentedI wrote a patch to this found at:
http://drupal.org/node/320784
Comment #6
jonskulski CreditAttribution: jonskulski commentedHere is a patch that includes a (0) original pid test from domas (1) test for threading and(2) a second comment reply
(0) Because it worked :)
(1) One because it was asked for and we would have found #320784: Comment threads not incrementing with it
(2) Because with the patch i submitted in the above issue, we need to cover more of comment.module code.
The coverage test (http://acquia.com/files/test-results/modules_comment_comment.module.html) will be changing, once the patch gets committed and we will need to test both conditions on the if statement on line 712.
(1) will test the if, and (2) will test the else
I ran into the issue domas noted, there was no available comment object, so it had to be loaded. However postComment also does not have the object, so I think it overkill to load it in postComment and pass it back for every call. Why not just let the test decide if it needs to be loaded? RFC.
here's the patch, I had to change the some surrounding tests to reflect my changes, as well. (Test for 3 comment link instead of 2 comment link)
NOTE: These tests will fail on current HEAD until the above patch is committed
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedDries committed #6.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.