comment_save() fails silently

Here's a start.

Subscribing.

Reroll

+++ modules/comment/comment.module	10 Mar 2010 22:31:07 -0000
@@ -2086,25 +2091,32 @@
-    $page = comment_get_display_page($comment->cid, $node->type);
-    if ($page > 0) {
-      $query['page'] = $page;
...
+      // Redirect the user to the node they are commenting on.
+      $redirect = 'node/' . $node->nid;
     }
-    // Redirect to the newly posted comment.
-    $redirect = array('node/' . $node->nid, array('query' => $query, 'fragment' => 'comment-' . $comment->cid));

Why are we no longer redirecting to the page that contains this comment? If I just submitted a comment that appears on page 3 I'd expect to be redirected to page 3 so I can see my comment. Thoughts?

+++ modules/comment/comment.module	10 Mar 2010 22:31:07 -0000
@@ -2477,8 +2489,12 @@
+    watchdog('action', 'Saved comment %title', array('%title' => $comment->subject));

I may be completely wrong, but I thought watchdog messages were supposed to be outputted in a t() too (if so, there are several places in the patch to fix this ... if not, ignore this)

Powered by Dreditor.

status

@#4

No, watchdog messages are translated when they are displayed.

Re-roll of #3 *trying* to take into account other recent changes to the comment module.

Regarding #4 "Why are we no longer redirecting to the page that contains this comment? "
AFAICT, we DO redirect to the page on which a comment appears, but in the case a comment doesn't save properly, then we redirect to the node that was attempting to be commented on...

Fix patch from #7. I'm not sure we need to replace the comment_save call in comment_approve(). Could loading and changing the status of a comment really cause comment_save to fail? There's one last call to comment_save in comment_admin_overview_submit(), and it also merely loads and changes the status of comments.

#9: Bear in mind I'm just looking at this patch and issue for the first time, but... if there is a db issue doing an update statement, then it would show up in comment_save no matter where it's called, right? Wouldn't it be better to remain consistent?

I agree. Also don't forget that not only comment_save() itself can fail but any of the hook implementations that are called, a single broken query in any of these and the whole transation is rolled back.

Added handling of comment_save() to comment_admin_overview_submit().

This looks pretty good to me. What does anyone else think about the translatable strings (I don't have an English degree)?

Looking at the sister topic, #704646: node_save() fails silently, in comment #28, Berdir suggests to use a sub class of Exception instead of Exception itself. This doesn't exist yet. Only an empty FieldException exists inside core modules. I think this is good to patch, and if it's necessary (critical or normal) for Drupal 7 to add in *Exceptions, then it should be done in another issue.

Just pointing out the above statement isn't exactly true :)

Assuming that all Exceptions end with "Exception" and extend another Exception

$ grep -R "Exception extends" *
includes/database/schema.inc:class DatabaseSchemaObjectExistsException extends Exception {}
includes/database/schema.inc:class DatabaseSchemaObjectDoesNotExistException extends Exception {}
includes/database/database.inc:class DatabaseTransactionNoActiveException extends Exception { }
includes/database/database.inc:class DatabaseTransactionNameNonUniqueException extends Exception { }
includes/database/database.inc:class DatabaseTransactionCommitFailedException extends Exception { }
includes/database/database.inc:class DatabaseTransactionExplicitCommitNotAllowedException extends Exception { }
includes/database/database.inc:class InvalidMergeQueryException extends Exception {}
includes/database/database.inc:class FieldsOverlapException extends Exception {}
includes/database/database.inc:class NoFieldsException extends Exception {}
includes/filetransfer/filetransfer.inc:class FileTransferException extends Exception {
includes/install.inc:class DatabaseTaskException extends Exception {
includes/updater.inc:class UpdaterException extends Exception {
includes/updater.inc:class UpdaterFileTransferException extends UpdaterException {
includes/update.inc:class DrupalUpdateException extends Exception { }
modules/field/field.attach.inc:class FieldValidationException extends FieldException {
modules/field/field.attach.inc:class FieldQueryException extends FieldException {}
modules/field/field.module:class FieldException extends Exception {}
modules/field/field.module:class FieldUpdateForbiddenException extends FieldException {}

I qualified my statement with "core modules" not just core for that reason. ;-)

Having now read #704646: node_save() fails silently, I agree. Per #704646-33: node_save() fails silently, this patch re-throws the exceptions. I prefer having a big red error to littering code with exception handling (whose messages communicate no information other than "Failure, sorry.").

Changing title to not presume handling exceptions is the right choice.

Committed to CVS HEAD. Thanks.