recently we had 2 admins dealing with a spammer at once, and one clicked "edit" for a comment that the other had just deleted. he got a page with invalid foreach/implode and a sql error. The attached patch checks that the selected comment exists before proceeding and just shows "Page not found" instead.

CommentFileSizeAuthor
#11 comment_edit_not_found_0.patch1.17 KBFreso
#9 comment_edit_not_found.patch1.18 KBFreso
#7 comment_edit_not_found.patch_4.txt1.17 KBmindless
#5 comment_edit_not_found.patch_3.txt1.13 KBmindless
#3 comment_edit_not_found.patch_2.txt772 bytesmindless
comment_edit_not_found.patch.txt826 bytesmindless

Comments

heine’s picture

heine commented
Status: Needs review » Needs work

objects without properties are no longer empty() in php5.

mindless’s picture

mindless commented

guess you could use !isset. i don't really see when a comment object with no properties would be valid here..

mindless’s picture

mindless commented
Version: x.y.z » 6.x-dev
Status: Needs work » Needs review
StatusFileSize
new comment_edit_not_found.patch_2.txt772 bytes

Updated patch attached, against current HEAD rev for comment.module.
you can still get a series of php warnings with a comment/edit/{invalid#} URL.

dries’s picture

dries commented
Status: Needs review » Needs work

Can we write this as if ($comment) { // do stuff } else { return drupal_not_found(); } ? That's more consistent with the rest of the Drupal code. Thanks.

mindless’s picture

mindless commented
StatusFileSize
new comment_edit_not_found.patch_3.txt1.13 KB

attached.

mindless’s picture

mindless commented
Status: Needs work » Needs review
mindless’s picture

mindless commented
StatusFileSize
new comment_edit_not_found.patch_4.txt1.17 KB

Rerolled patch from current cvs.

mindless’s picture

mindless commented

Let me know if anything more is needed on this one, thanks.

Freso’s picture

Freso commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new comment_edit_not_found.patch1.18 KB

I can reproduce the bug, but the current patch doesn't apply:

gentoo-vm drupal6 # patch -p0 < comment_edit_not_found.patch_4.txt
patching file modules/comment/comment.module
patch: **** malformed patch at line 10:

Re-rolled the patch which made it work as advertised. AFAICT, it's RTBC.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Needs work

Neither drupal_access_denied(), nor drupal_not_found() return anything. So why do we have return drupal_not_found() in the patch? It looks like being at the end of the function even.

Freso’s picture

Freso commented
Status: Needs work » Needs review
StatusFileSize
new comment_edit_not_found_0.patch1.17 KB

This patch is identical to the last one, with the exception of one less return. I haven't tested that this new patch works or even applies, but it should work just as well as the other did. But as I haven't actually tested it, I'll just leave it as "needs review".

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

No longer applies.

dpearcefl’s picture

dpearcefl commented
Status: Needs work » Postponed (maintainer needs more info)

Is this still a problem in current D6?

dpearcefl’s picture

dpearcefl commented
Status: Postponed (maintainer needs more info) » Needs work

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.