Refer to user.install, {users} come with some integer column. user_save() update section handle the data type matching in lossy style: match all data as '%s' (string handling). It should have similar handling as its insert section: handle integer fields as %d, where the others as '%s' explicitly.

We can check and handle all {users} fields as user_save() insert section (but this should be too fancy...); BTW, as we now having drupal_write_record(), which can ensure the correctness of data and its column type matching, we can utilize it.

Patch via CVS HEAD, tested with mysql, mysqli and also pgsql, pass user add and update handling.

CommentFileSizeAuthor
user_save-0.1.patch3.47 KBhswong3i

Comments

hswong3i’s picture

hswong3i commented

P.S. I discover this bug when develop ibm_db2 driver: we try to update a string value into {users}.status and so face critical error. We haven't discover this critical error just because mysql and pgsql is too lossy in data type checking. In case of my personal research project Siren, the patch also pass with oci8 and ibm_db2.

catch’s picture

catch
he/him
Primary language English
commented
Version: 6.x-dev » 7.x-dev
Category: bug » task
Priority: Critical » Normal

This is about the third issue converting user_save to drupal_write_record that's been bumped to D7 now.

hswong3i’s picture

hswong3i commented
Version: 7.x-dev » 6.x-dev
Category: task » bug
Priority: Normal » Critical

@catch: I don't really care about if we are using drupal_write_record() for user_save() or not. My only concern is: user_save() is now handling data type matching incorrectly (update some string value into integer field), and this should count as a bug. We may say "PHP is lossy, MySQL and PostgreSQL are both lossy", but I don't think this is the point for forgive this bug, and left it behind for D7, especially when we are able to figure this out.

You may propose some other solution, e.g. manually handle all type matching as its INSERT section as I mentioned above, if not feeling well with drupal_write_record(). I would like to have your counter proposal about that, and I would like to give a hand for its testing, too :-)

catch’s picture

catch
he/him
Primary language English
commented
Version: 6.x-dev » 7.x-dev
Priority: Critical » Normal
Status: Needs review » Closed (duplicate)

My only concern at the moment is that the critical issues queue isn't cluttered by non-critical code refactoring. Especially the same refactoring that's already been reviewed and postponed to D7 in previous issues.

Marking as duplicate of:

http://drupal.org/node/181411

See also comments on this issue, which was actually a bug: http://drupal.org/node/204705