node_save() fails silently

API functions like node_save() don't set messages or issue watchdog and so on. thats the responsibility of the caller.

It could do $status = drupal_write_record($node); then return $status at the end, that gives you 1 or 2 depending on whether it's an insert or update and would be consistent with taxonomy_term_save() and possibly others. The exception watchdog message ought to be caught by test bot but I think there's an outstanding patch for that.

Agreed this is critical, just eating errors and writing them to dblog is a recipe for hard to find bugs all over the place, and a serious regression from D6.

IMO, the exception should be propagated up, that is if node_save wants to catch it and write to watchdog then fine, but I think it should re-throw the exception is has caught. I am currently unaware of the scope of changes this would require in calling code.

NODE_SAVE() RETURN VALUE

As catch said, node_save() should return FALSE if any of its drupal_write_record() calls return FALSE.
Same applies to _node_save_revision() and therefore _node_save_revision needs to return FALSE on failure, based on its drupal_write_record() return.

I'm not qualified to judge whether re-throwing the exception makes sense but it would allow the UI to give more useful error messages.

UI

The good news is that only 5 functions call node_save:
- book_admin_edit_submit in modules/book/book.admin.inc
- node_form_submit in modules/node/node.pages.inc
- node_revision_revert_confirm_submit in modules/node/node.pages.inc
- node_save_action in modules/node/node.module
- _node_mass_update_helper in modules/node/node.admin.inc
http://api.drupal.org/api/function/node_save/7 (I love that site)

So fixing the UI actually reporting failure shouldn't be too hard.

TESTS

Testing does handle exceptions, apparently:
#305077: Rework simpletest backend solved #293521: Simpletest should catch exceptions and #242069: Simpletest should include exception data in reporting so this should follow naturally. Also #304924: Extend drupal_error_handler to manage exceptions

This patch doesn't fiddle with exceptions because it doesn't seem necessary for the actual calling functions. Provides a return status for node_save() and _node_save_revision() and implements what seem to me to be reasonable responses for the callers.

UI:
- book_admin_edit_submit in modules/book/book.admin.inc - report failure to change title but carry on with ordering and other nodes
- node_form_submit in modules/node/node.pages.inc - report failure
- node_revision_revert_confirm_submit in modules/node/node.pages.inc - report failure
- node_save_action in modules/node/node.module - return status, which actions_do passes to its calling function. These don't appear to take any notice, but that's not our problem here.
- _node_mass_update_helper in modules/node/node.admin.inc - made it take a node rather than a nid, and return the status. Calling functions then drupal_set_message() if there's an error. This implies a lot of drupal_set_messages() but is very unlikely to happen, I'd have thought.

+++ modules/book/book.admin.inc	21 Feb 2010 15:55:07 -0000
@@ -131,7 +131,10 @@ function book_admin_edit_submit($form, &
-        node_save($node);
+        $status = node_save($node);
+        if (!$status) {
+          drupal_set_message(t('Could not update title for %title.', array('%title' => $node->title)), 'error');
+        }
         watchdog('content', 'book: updated %title.', array('%title' => $node->title), WATCHDOG_NOTICE, l(t('view'), 'node/' . $node->nid));

the watchdog() should be in an else(), no?

Otherwise, looks good.

Powered by Dreditor.

Yes.

Rerolled because '*=' is the wrong operator to use with boolean logic. Use '&=' instead.

Also, I must admit I think this is much worse than throwing relevant exceptions at failure points, instead of just aggregating a boolean state by accumulating failures over the course of the procedure. At very least we should be dropping out early on the first fail. It's crazy that the function is failing on the first $return = FALSE, but yet doing like 85 other things between there and the return.

Putting in my vote for throwing exceptions, but on the chance that no one really listens or cares, here's the rerolled patch to use the proper operators for this... approach.

Oh, I see why. Can't do &&=, silly me. Maybe *= is better here? Seems wrong to use arithmetic to get boolean answers. I appreciate 'clever hax' as much as the next guy but it at least warrants a comment if we keep this, yeah?

$foo = TRUE;
$foo &= FALSE;
$foo &= TRUE;
var_dump($foo);

$foo = TRUE;
$foo *= FALSE;
$foo *= TRUE;
var_dump($foo);

Prints int(0) int(0). No difference, but no boolean either. No idea, sorry.

I think I know why... it's because the rvalue in those expr's don't necessarily equal 1.

1 & 2 = 0 :)

Yo, got a new patch here.

Addresses the things jhodgdon mentioned, and also makes more sense, logically.

Instead of just plowing over stuff when there's a fail (current version) or just tiptoeing over sensitive stuff once there's a fail ($return = $return &&), I've implemented exceptions for each of the failure points.

Now, if anything fails, an exception is thrown, which jumps right over anything else and right to the transaction rollback (Something that wasn't handled on non-exception-throwing failures), and allow us to mark the return as FALSE from the catch block, instead of having it all over the place.

Good call on the return variable. I also fixed the batch update function, and added a test.

Oh, and I also added a check for node_save in the SimpleTest system.

Let's see what zee bot thinks.

Ah, I invoked the batch function incorrectly from the test.

Subscribing.

+++ modules/node/node.admin.inc	23 Feb 2010 18:47:31 -0000
@@ -300,22 +300,23 @@ function node_mass_update($nodes, $updat
+      $node = node_load($nid, NULL, TRUE);
+      if (!_node_mass_update_helper($node, $updates)) {
+        drupal_set_message(t('An error occurred while processing %title. The %type could not be saved.', array('%title' => $node->title, '%type' => $node->type)), 'error');
+      }

The %type.. sentence doesn't make much to me. Do we even need that one?

+++ modules/node/node.admin.inc	23 Feb 2010 18:47:31 -0000
@@ -300,22 +300,23 @@ function node_mass_update($nodes, $updat
+function _node_mass_update_helper(&$node, $updates) {
   foreach ($updates as $name => $value) {
     $node->$name = $value;
   }
-  node_save($node);
-  return $node;
+  return node_save($node);
 }

I see why it is necessary, but this seems like an API change. Yes, it's probably necessary and it's an internal helper function, Just saying.. :)

+++ modules/node/node.admin.inc	23 Feb 2010 18:47:31 -0000
@@ -333,7 +334,11 @@ function _node_mass_update_batch_process
+    $node = node_load($nid, NULL, TRUE);
+    $status = _node_mass_update_helper($node, $updates);
+    if (!$status) {
+      drupal_set_message(t('An error occurred while processing %title. The %type could not be saved.', array('%title' => $node->title, '%type' => $node->type)), 'error');
+    }

Same as above

+++ modules/node/node.module	23 Feb 2010 18:47:34 -0000
@@ -1013,22 +1016,32 @@ function node_save($node) {
+      if (SAVED_NEW != drupal_write_record('node', $node)) {
+        throw new Exception('saving node');
+      }
+      if (!_node_save_revision($node, $user->uid)) {
+        throw new Exception('saving node revision');
+      }

Imho, Exception shouldn't be used directly but something like NodeSaveException instead. Also, the strings can be improved. It looks strange that they aren't translated but that's because they are passed to watchdog and translated when displayed. Quite a mess imho, but nothing we can do about it...

+++ modules/simpletest/drupal_web_test_case.php	23 Feb 2010 18:47:37 -0000
@@ -777,7 +777,9 @@ class DrupalWebTestCase extends DrupalTe
-    node_save($node);
+    if (!node_save($node)) {
+      $this->fail(t('Node could not be created for drupalCreateNode().'), t('SimpleTest'));
+    }

Agreed that we dont the function name in the message and can't we just use $this->assertTrue(node_save($node), .. here?

Powered by Dreditor.

Hm, maybe :) But note that $node->type is the machine readable name....

Just wanna bump this to the top of my queue.

Per #9 we should just re-throw the exception, bloating the calling code like this just in case there's a critical error seems unreasonable to me. If there's an exception you want a big red error, not a nice message saying sorry.

I agree. We should not be using exceptions half-way. Exceptions were introduced to programming specifically to avoid handling and throwing return values of FALSE and TRUE all the way up the call chain. Here is a patch that re-throws exceptions, addressing the issues in the OP and in #7.

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

Update test to expect exception.

I think I need to be calling node_save() instead of drupalPost in this test, as otherwise I can't catch the exception thrown by node_test_exception_node_insert.

re-rolled as a cvs diff patch.

I didn't even know hook_node_post_save() existed seems quite useless given _insert() and fapi both exist, we should get rid of that in D8. Patch looks good, tests pass.

Oh I see, just the existing comment is wrong talking about a hook which doesn't exist, should've realised since there's no change to the dummy module itself. Good cleanup either way.

Committed to CVS HEAD -- glad to see it was synced with comment_save() fails silently.

Was this ever fixed for D6? I'm having this exact issue with node_save() on a D6 site. I checked the {watchdog} table and found no errors. I am at a loss to identify what the error is. Each time I call node_save() the nid gets incremented yet no new node is added to the table.

This doesn't affect Drupal 6. The problem here was that the exceptions was thrown and there was no way to catch it. There are no exceptions used in D6. Make sure that you have error messages enabled.

I have messages enabled. No messages are displayed. I get a message when I create a node/add/story but nothing when I use node_save(). (I don't want to hijack this thread, should I start a new one? But, I'm not sure that I have a real issue)

Hi everybody

in my case i need for several content types that the "the content x has been created" doesn't display other i will have a button for the next step. Is there any way i can disable this confirmation?

thank you