Related Issues

This issue is a child of #1632492: Figure out and port project_issue comment numbering functionality to D7

Overview

This module is being considered for use on www.drupal.org, but it does not provide for persistent numbering of each comment. Thus, if a comment were deleted, or inaccessible to a user due to permissions settings, all the comments on the page would renumber themselves (possibly differently for each user).

It is possible to fix this by creating a new table in this module with three columns for cid, nid, and comment_number and a second table with three columns for nid, last_comment_number, and db_lock. These two tables will be used to persistently match the comment ids to the displayed numbers, and to retrieve the next comment number and store it while ensuring that two simultaneous new comments cannot create a race condition and become out-of-order.

Background

(Below text copied from https://drupal.org/node/1632492#comment-6215528)
"if you look in the 6.x-1.x branch of project_issue there's a {project_issue_comments} table that includes all the additional fields the project_issue module injects into comments. However, the persistent comment numbering functionality also depends on the {project_issues} table, which stores the last comment number for a given issue node. We had to do some pretty tricky magic to ensure that there were no race conditions when two users post comments simultaneously and the ordering gets screwed up. Feel free to study that code and copy it for your module. Point being, I don't think there's a safe way to accomplish this feature with a single column in a {comments} table, even if altering the core schema was actually a wise thing to do. ;) Therefore, I don't think a single {comment_easy_reply_comment} table with cid, nid and comment_number will be sufficient. I think you'll need table like that, plus something like {comment_easy_reply_node} with nid, last_comment_number, and db_lock. Again, see the 6.x-1.x branch of project_issue for more."

Meta logic for handling the creation of a new comment using the table locking system in core's lock.inc:

<?php
// Comment has just been added.
if (lock_acquire('comment_easy_reply' . $comment->nid)) {
    // $next_id = SELECT FROM {comment_easy_reply_serial} WHERE nid=$comment->nid
    // INSERT {comment_easy_reply_number} VALUES($comment->nid, $comment->cid, $next_id)
    // $next_id++
    // UPDATE {comment_easy_reply_serial} set number=$next_id
    // ...
    lock_release('comment_easy_reply' . $comment->nid);
}
?>

Comments

senpai’s picture

senpai commented
Assigned: Unassigned » bdragon
Priority: Normal » Major

Assigning to Bdragon for some code this Friday.

dww’s picture

dww
we/he/they
commented
Assigned: bdragon » Unassigned
Category: bug » feature

Assuming the core lock API all works as expected, I'm totally fine with that. Somehow I forgot that was added. ;) It'd require some pretty heavy testing, possibly with some sleep() calls temporarily put in place to make the race condition window wide enough to ensure we're hitting it. But, assuming it's solid (which seems like a safe assumption) I'm happy to just use lock.inc instead of reimplementing our own.

Also, technically I think this is a feature request for the module -- it's not really a bug that it doesn't work like we expected/hoped it does. ;) However, since this is a blocker for using this on d.o, I'm bumping to major.

Thanks!
-Derek

dww’s picture

dww
we/he/they
commented
Assigned: Unassigned » bdragon

Sorry, whoops.

kongoji’s picture

kongoji commented

Hi bdragon,
with the hoping to help you somehow and to make it easier for you to make improvements to the module, I'll try to describe below the current module's numbering logic.
Maybe it could be useful also to say that the dev version of the module has Views integration and any changing on comment's numbering system could be good as weel for module's Views fields (just in case you need Views for comments on next d.o release).

function _comment_easy_reply_get_comment_number

This function currently store any association comment<-->number on a static array trying to avoid as much as possible to hit the db with a query (that is done only on the worst scenario, when we are on /comment/reply/[nid]/[cid] page).
Everything is inside this function could be replaced by a single database query.
The comment number is different from flat comments and threaded comments, to respect the parent/child relationship. Threaded comments have numbers like #1.1, #1.2, #5.2.10, #16.3.1, etc...

function _comment_easy_reply_get_comment_cid_from_num

This is the opposite of _comment_easy_reply_get_comment_number: given a node id and a comment number, returns the comment id (the cid).
It's used to replace tags like @#NUM in comments' body with the appropriate link+tooltip containing the referenced comment.
The function uses a custom function derived from comment_get_thread. This was needed because comment_get_thread function modified also the pager. Now it's not needed anymore.

function _comment_easy_reply_get_comment_page

This function is used to calculate the page a comment is. It is used to create links to referred comments.

function _comment_easy_reply_comment_replace

Replace every @#NUM occurrence in a comment with an appropriate link+tooltip. Every @#NUM occurrence inside the tooltip is replaced by the only link (without the tooltip), to avoid having a tooltip inside another tooltip.

function _comment_easy_reply_comment_body_replace

It's the same of _comment_easy_reply_comment_replace, but it's used for integration with other modules. Right now it is used by module's Views fields

Hoping all this will be somehow useful for you, in case you need you can contact me anytime in this issue queue, by mail or on skype (my skype id is alb.colella)

Alberto

bdragon’s picture

bdragon commented
Status: Active » Fixed

I think this is fixed, since code to use $comment->thread was added.

Status: Fixed » Closed (fixed)

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