Post a comment as a reply to another comment, check the pid is correct, probably threading too.

CommentFileSizeAuthor
#6 comment_pid_thread_test.patch2.33 KBjonskulski
#2 comment_pid_test.patch907 bytesAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Assigned: Unassigned »

Working on it..

Anonymous’s picture

Priority: Critical » Normal
Status: Active » Needs review
FileSize
907 bytes

Submitted 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.

catch’s picture

Should we be checking the thread string as well here?

jonskulski’s picture

Test 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...)

jonskulski’s picture

I wrote a patch to this found at:

http://drupal.org/node/320784

jonskulski’s picture

Here 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

Damien Tournoud’s picture

Status: Needs review » Fixed

Dries committed #6.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.