Posted by catch on August 17, 2008 at 8:40pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | tests |
| Category: | bug report |
| Priority: | normal |
| Assigned: | domas |
| Status: | closed (fixed) |
Issue Summary
Post a comment as a reply to another comment, check the pid is correct, probably threading too.
Comments
#1
Working on it..
#2
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.
#3
Should we be checking the thread string as well here?
#4
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...)
#5
I wrote a patch to this found at:
http://drupal.org/node/320784
#6
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
#7
Dries committed #6.
#8
Automatically closed -- issue fixed for two weeks with no activity.