Use schema API during user_save()

I did a code review and the code looks good, Moshe. One remark: user_fields() is only called from one location. We should be able to nuke that function now.

(I haven't tested this patch yet.)

I cleaned up a bit more cruft around the ifs that set/unset $data parts.

I've tested this patch and it works. The quality of the code comments needs to improve though. For example for ext. auth are should be written in full so that it is accessible for less savvy developers. That line is _critical_ for security so let's not take that lightly. Thanks.

Is the + if (!empty($data)) { check in user_save() still required now?

I reviewed this patch and it works fine. Besides Moshe's test plan, I also tested the serialized data field: add a value, preserve a value if it is unset, remove a value if it is set to NULL. It probably would have taken less time to write simpletests than to test it manually.

I made some changes:

1. Instead of setting $user_fields = array_keys($user_table['fields']) and then testing in_array($key, $user_fields), I set $user_fields = $user_table['fields'] and then test empty($user_fields[$key]). This turns O(n^2) to O(n lg n) for no extra work.

2. I improved the docs for user_save() and moved a comment for coding standards.

A new patch with these changes is attached.

I also note that this patch introduces an API change: Previously, in hook_user('update'), $edit['pass'] was always set but was '' if the user did not enter a password. Now, $edit['pass'] is unset is the user enters no password. This triggered an E_ALL error in persistent_login.module which is how I noticed. I think the new behavior is fine but should be documented in hook_user and perhaps in the D5 > D6 module upgrade notes.

If Moshe reviews and approves my in_array() to empty() and comment change, this is RTBC. Whether or not this constitutes a "bugfix" that gets into D6 is a separate issue.

Sorry for not doing this myself, I was out of town all week at a client site. Just got back last night.

This looks a bit like a hack. I can see why it's necessary but it ain't pretty. Maybe there is a creative workaround that looks slightly prettier? ;-)

+  if (empty($fields)) {
+    // No changes requested. 
+    // If we began with an array, convert back so we don't surprise the caller.
+    if ($array) {
+      $object = (array)$object;
     }
+    return;

This looks really odd and not something we had in the old code, I believe. Is this an inherent drawback of the new mechanism?

+  else {
+    // Avoid overwriting an existing password with a blank password.
+    unset($array['pass']);
+  }

Regarding unset($array['pass']), the current code does this:

      if ($key == 'pass' && !empty($value)) {
        $query .= "$key = '%s', ";
        $v[] = md5($value);
      }

If no key of the array is 'pass' *or* if pass is empty, the query never sets that column. drupal_write_record() does the former ("don't set columns whose keys are unset in the passed object") but cannot assume that empty-string values also should not be set (because often they should be).

So, yes, this unset is effectively something we have in the current code in a different form, and requiring it is not a drawback of drupal_write_record(). It is just an example of having to normalize a data structure to comply with a slightly more general-purpose API function.

Thanks for clarifying, Barry! You're absolutely correct.

$ patch -p0 < mw_105.patch
patching file includes/common.inc
Hunk #1 succeeded at 3291 (offset 126 lines).
patching file modules/profile/profile.module
Hunk #1 FAILED at 344.
1 out of 1 hunk FAILED -- saving rejects to file modules/profile/profile.module.rej
patching file modules/user/user.install
patching file modules/user/user.module
Hunk #4 succeeded at 2081 (offset 3 lines).
patching file modules/user/user.pages.inc

In all honesty, I am not sure about what to do with this patch. We have seen several pass by reference (eg. in node revision saving), default value setting, database compatibility etc. issues around drupal_write_record() implementations elsewhere, and needed to work on fixing all those. As far as I see this code change is a nice cleanup, but does not fix a bug or introduce anything new. Changing the user saving implementation this late does not sound the best idea, although I wholeheartedly agree that the current implementation looks ugly, and it should be fixed when safe. Let us know if you think otherwise.

I agree that this is not mandatory. We're way past where the line for D6 should have been drawn...

Thanks for understanding. Moving to D7. Keeping status, so it is committed early.

looks like a couple small parts of this should be for D6 - like

@@ -247,6 +247,7 @@ function user_schema() {
         'type' => 'text',
         'not null' => FALSE,
         'size' => 'big',
+        'serialize' => TRUE,

Hey guys,

I need some info on this silly question, often time i see attachment with extension .patch. I know the intense of this file is to fix issue. My question is how can I use this file and apply it to the module. Could anyone provide me some steps on how to use it? Many thanks.

leoman730: See http://drupal.org/patch/apply for more information.

catch judge that #211496 is duplicated with this issue, but sorry that I don't really think so: #211496 is introduce because of incorrect data type matching before we build the queries. I seems this as something more critical that refactor overall user_save() handling for elegant. BTW, I keep a message here as a reference of that bug, and so let's solve it after D6 ;-)

Committed to CVS HEAD. Thanks.

As I noted above, there are a few small pieces of this patch that should be in 6.x

I think it would be good if someone who knew what to extract for 6.x (pwolanin? moshe?) either ported those changes to 6.x or were more specific so someone (me, perhaps) could easily tell what to port and what to discard.

@Freso - looks to me like this might qualify as 6.x bug fixes to be extracted from the patch:

all the changes to includes/common.inc
the change to modules/user/user.install
all changes to modules/user/user.pages.inc
the change to function user_register_submit() in modules/user/user.module

Alright then. Here's a straight port of the things pwolanin picked out in #33. It is exactly the same code changes as those in #29, as that patch applied cleanly to DRUPAL-6 (with some offset).

I haven't tested this though, as I must admit I haven't even read the issue through to be sure what needs to be tested. I just thought I'd put some work towards cleaning out the 6.x "to be ported" queue. :)

Looks like parts of this last patch already made it into core for drupal_write_record() - is this issue too old/duplciate now?