It's evil that people can post completely empty comments, for example:
http://drupal.org/node/189972#comment-624115
We used to prevent this, with code to check if you had either a) changed some metadata or b) filled in something for the followup body. IFAC seems to have lost this code. I know I mentioned it before in the IFAC issue, but I think hunmonk wanted to punt to a separate issue. Well, here we are. ;)
Related to http://drupal.org/node/140473 but not duplicate, since this is about a bug (the regression in functionality) and that's about a code refactoring task. However, I wanted to mention it here so that it's on everyone's radar...
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | ifac_empty_comment_fix_2.patch | 2.36 KB | dww |
| #9 | ifac_empty_comment_fixa.patch | 2.25 KB | hunmonk |
| #5 | ifac_empty_comment_fix.patch | 1.86 KB | dww |
Comments
Comment #1
hunmonk commentediirc, the validation code to accomplish this looked to be pretty ridiculous. i think we should weigh that against our desire to prevent this.
Comment #2
dwwThat's why I mention http://drupal.org/node/140473 ... I see this as a golden opportunity to switch to something more sane -- we could even implement hook_diff() and incorporate that.
Now that the issue replies are directly on the issue page themselves, it seems to increase the likelihood that someone will press "post comment" accidentally without meaning to, generating a lot of noise to people watching the issue via tracker, email, rss, etc. Send more signal, please. ;)
-Derek
Comment #3
dwwBump: now that http://drupal.org/node/140473 is done and committed, this should be relatively easy.
Comment #4
aclight commentedFYI, I don't think this is all that easy. Last time I looked at this, the issue node and/or comment objects were not loaded at the point where we would want to call project_issue_comment_changes() to see if there are any changes in a comment or not. So it's possible that there's a really easy way to load the necessary node/comments at that point, but nothing came to mind when I looked at this a bit ago.
Comment #5
dwwThe issue node itself is all initialized at the top of project_issue_comment. That's not the problem. The bummer here is that hook_comment() is evil, and you get a comment object in op 'view', and an array of form values in op 'validate', so we can't cleanly share the code to initialize the array of changes (as described in the code comment in this patch). Instead of trying, I just left that object-based code alone in op 'view', and added new code to handle the array version of $arg in op 'validate'.
So, this patch isn't as small as I had hoped, but it totally works in my local testing. I don't think it's as evil as feared, and it's mostly yucky from the wonky object vs. array ambiguity in the core API itself, not through our own lameness. project_issue_comment_changes() itself is clean and happy, as I had hoped. Needs review, but I think this is a good move and is RTBC.
Comment #6
dwwFYI: installed on project.drupal.org for testing, if anyone wants to play with it there.
Comment #7
aclight commentedChanging the issue by only adding a file doesn't seem to pass validation. Shouldn't that be allowed?
Comment #8
dwwHrm, interesting. Yeah, I guess so. I guess it's unreasonable to expect that project_issue_comment_changes() would contain such information -- we should just add this to the
empty($arg['comment'])check. No time to fix this now, but I'll re-roll later. Thanks for paying such close attention. ;)Comment #9
hunmonk commentedattached should do it.
the file testing made things a bit more complicated, because we can't simply test for the existance of a files array in the comment array -- it's possible that someone may add a file during preview, then mark it for removal. so, i added just a bit more checking code to handle that edge case as well. if we're going to fix it, we might as well fix it all ;)
tested pretty thoroughly, and i think we're good now.
Comment #10
aclight commentedI tested this out a fair bit on project.drupal.org and couldn't find any problems with it. Looks good to me.
Comment #11
hunmonk commentedcommitted to 5.x-2.x, deployed on d.o, sec.d.o, project.d.o
Comment #12
dwwThat works, but it's kind of convoluted. Why test for the files on *every* iteration of the metadata changes? Furthermore, why bother computing the metadata changes at all if we already know we've got a file attached? I think this is better. Since you already committed, this is a patch against revision 1.120.
Comment #13
hunmonk commentedyup, seems more sane. tested, and works the same. committed to 5.x-2.x and deployed on d.o, et. all
Comment #14
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.