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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Heine’s picture

Heine CreditAttribution: Heine commented
Status: Needs review » Needs work

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

mindless’s picture

mindless CreditAttribution: 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 CreditAttribution: mindless commented
Version: x.y.z » 6.x-dev
Status: Needs work » Needs review
FileSize
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 CreditAttribution: 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 CreditAttribution: mindless commented
FileSize
comment_edit_not_found.patch_3.txt1.13 KB

attached.

mindless’s picture

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

mindless CreditAttribution: mindless commented
FileSize
comment_edit_not_found.patch_4.txt1.17 KB

Rerolled patch from current cvs.

mindless’s picture

mindless CreditAttribution: mindless commented

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

Freso’s picture

Freso CreditAttribution: Freso commented
Status: Needs review » Reviewed & tested by the community
FileSize
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
CreditAttribution: Gábor Hojtsy 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 CreditAttribution: Freso commented
Status: Needs work » Needs review
FileSize
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
CreditAttribution: catch commented
Status: Needs review » Needs work

No longer applies.

dpearcefl’s picture

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

Is this still a problem in current D6?

dpearcefl’s picture

dpearcefl CreditAttribution: 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.