currently, the docs for drupal_write_record say:
(boolean) Failure to write a record will return FALSE. Otherwise, TRUE is returned. The $object parameter contains values for any serial fields defined by the $table. For example, $object->nid will be populated after inserting a new node.
however, the return value from db_query() is not checked, so if a query fails for whatever reason, the function will still return one of the success values.
attached patch checks for the return of db_query() and sets the return value to false if db_query fails.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | drupal_write_record-6.patch | 1.31 KB | pancho |
| #6 | drupal_write_record-0.4.patch.txt | 2.48 KB | gábor hojtsy |
| #5 | drupal_write_record-0.3.patch | 1.28 KB | hswong3i |
| #2 | drupal_write_record-2.patch | 1.27 KB | pancho |
| #1 | drupal_write_record-0.1.patch | 734 bytes | hswong3i |
Comments
Comment #1
hswong3i commentedOr we may handle in this way?
Comment #2
panchoAs this is the core query of a very important function, I'd like to make its behavior a little more explicit. But that's just my feelings about it. Otherwise, the two other variants equally work well.
No doubt this is a critical bug. Seems to be easy to solve though. Some more reviews would be great, to immediately get this in for RC2.
Comment #3
panchoComment #4
gábor hojtsyDrupal does "good things first" (instead of negative conditionals). So do an
if (db_query(...)) { do stuff } else { return FALSE; }insteadComment #5
hswong3i commentedImplement as "good things first".
Comment #6
gábor hojtsyThanks, committed this patch with some updates. Actually, drupal_write_record() returns SAVED_NEW or SAVED_UPDATED on success, *not* TRUE, so the return value is not boolean. Also, the phpdoc formatting was bad on the return value, and the $update parameter as well. Committed the attached patch.
Comment #7
panchoJust slightly prefer having an explicit else-clause here. Also I prefer the commentary "Perform query." to "Execute the SQL."
But that's very much my opinion and yes it is nitpicking. Just think this is a very central function that should be documented as clearly as possible.
Otherwise #5 is good, and either #5 or #6 should be rtbc.
Comment #8
panchoWhoops, crossed posts. Was too late, nevermind.
Comment #9
Anonymous (not verified) commentedThanks all for the speedy responses and patch improvements!
Comment #10
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.