The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Actually, it is now possible to reply to non existent comments, which enters a comment parent (pid) in the database that may break comment rendering or other unexpected results.
The patch checks is the comment exists, and generates a page not found if it doesn't.
Please, review.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | comment.module.reply_nfnd_0.patch | 2.27 KB | markus_petrux |
| #6 | comment_28.patch | 67.99 KB | dries |
| #5 | comment_27.patch | 1.54 KB | dries |
| #1 | comment.module_47.patch | 955 bytes | eaton |
| comment.module.reply_nfnd.patch | 706 bytes | markus_petrux |
Comments
Comment #1
eaton commentedWhen I tested the patch it generated a page not found screen, but ALSO displayed the normal drupal page output below that. Added a simple return; after the drupal_page_not_found(); and it seems to work fine.
Also tested some bogus scenerios (like creating a comment that is a parent of itself)... default comment rendering doesn't seem to break, though obviously it doesn't display a parent.
Comment #2
eaton commentedChanging the status of this one, as it's a legitimate bug but it's very minor, has no ill effects, and would only occur if the user deliberately entered a bad pid.
Comment #3
markus_petrux commentedThis is a trivial fix, does it worth to spend a couple of minutes to commit?
Comment #4
john.money commentedThis condition could occur if user A started replying to a legitimate comment, say a spam, but before they completed submitting it (went to get a beverage or something), a moderator deleted the original spam comment. It doen't require any "bad behavior" on the part of user A.
Comment #5
dries commentedThis patch looks more in line with the rest of the function.
One could argue that we should use
drupal_not_found()as that will generate the proper HTTP headers.I'm fine with that but in that case we need to be consistent and change the
drupal_set_message(t('You are not authorized to post comments.'), 'error');lines (and friends) intodrupal_access_denied().Comment #6
dries commentedHere is a patch that uses
drupal_not_found()anddrupal_access_denied().This function can be cleaned up significantly, it seems. Also, some of the checks, like
else if (user_access('access content')) {should probably be moved up. Also, we don't check if$nodeis non-empty.Comment #7
dries commentedNevermind that last patch. Something got messed up.
Comment #8
markus_petrux commentedI tested the patch in comment #5. It still shows the reply form. It lacks a return statement, just after the message is set (same thing Eaton did to my first patch).
I like this approach better than drupal_not_found(), 'cause besides consistency, it has the ability the tell the user "what" the problem is.
Well, I first tried to add a return statement after the error message is set, but the result is a page (with possible blocks around) and the message on top. Not very nice.
So... instead of generating an empty page with only the error message, I tried to use drupal_goto("node/$nid"). Much nicer, IMO, so I applied the same trick (for consistency) to the other places where an error is set.
Note that, in the context of comment_reply(), $nid is valid, it's been verified in hook_menu.
Comment #9
dries commentedAdding the possibility to display custom errors on 404 and 403 pages might get us the best of both worlds: good error messages for users and good error codes/headers for crawlers/bots/tools.
Your last patch looks good though; we can go with that for now.
Comment #10
markus_petrux commentedswitching issue status then. Please, correct me if I'm wrong.
Comment #11
dries commentedCommitted to HEAD. Thanks.
Comment #12
(not verified) commented