Implementation of uuid_user contains error:

This code:
case 'insert':
if (isset($user->uuid) && uuid_is_valid($user->uuid)) {
db_query("INSERT INTO {uuid_users} (uid, uuid) VALUES (%d, '%s')", $user->uid, $user->uuid);
}

makes impossible to set value of uuid before call of user_save on new user object. Before hook_user with $op = 'insert' is called, $user object is saved to database (only to user table) and after that loaded again with user_load function call. This resets uuid field value to "false". If uuid is false it will be generated new value.
Example from "real world" where this bug is exploited is attempt of import users from external source with feeds module. UUID values were generated on external site, and all those values are gone after import.

As workaround I used $edit array as uuid value container. After saving uuid value to database, this field is unset-ed.

CommentFileSizeAuthor
#5 uuid-972258-2.patch1.19 KBapotek
#3 uuid-972258-1.patch1.17 KBapotek
#2 uuid-972258.patch993 bytesapotek
uuid.patch1.4 KBjsobiecki

Comments

apotek’s picture

apotek commented

while this might look like an edge case, it certainly is a bug, and the patch looks good.

wondering, though, if by moving entirely to the $edit array for the check, we are missing some other condition.

would it be better to write the $edit array check as a fallback in case the value is missing from the first test, rather than replacing it?

In a situation where there is no form submission, but the uuid value has been added by some other means, this would fall through entirely, would it not?

My suggestion (untested):

      if (isset($user->uuid) && uuid_is_valid($user->uuid)) {
        db_query("INSERT INTO {uuid_users} (uid, uuid) VALUES (%d, '%s')", $user->uid, $user->uuid);
      }
      else if (isset($edit['uuid']) && uuid_is_valid($edit['uuid'])) {
        db_query("INSERT INTO {uuid_users} (uid, uuid) VALUES (%d, '%s')", $user->uid, $edit['uuid']);
        unset($edit['uuid']);
       }
       else if
apotek’s picture

apotek commented
StatusFileSize
new uuid-972258.patch993 bytes

Implemented a patch as per my comment above, but against 6.x-1.x-dev. Any thoughts?

apotek’s picture

apotek commented
StatusFileSize
new uuid-972258-1.patch1.17 KB

Thinking about this a bit more, and based on peer-feedback, the edit field really shouldn't be unset, as some later invocation of the user hook might be expecting it.

So, new patch. Also, refactored into one SQL statement rather than three.

Status: Needs review » Needs work

The last submitted patch, uuid-972258-1.patch, failed testing.

apotek’s picture

apotek commented
Version: 6.x-1.0-beta2 » 6.x-1.x-dev
StatusFileSize
new uuid-972258-2.patch1.19 KB

Weird. Not sure why this isn't applying cleanly. Trying again.

nor do I know why it's being ignored :-)

jsobiecki’s picture

jsobiecki commented

@apotek,,

1. I'm not sure about leaving $edit['uuid'] field unsetted is correct. We are implementing 'insert' operation. According to documentation of hook_user:

"insert": The user account is being added. The module should save its custom additions to the user object into the database and set the saved fields to NULL in $edit.

If field will not be unseted, this will be saved additionally at serialized array at "data" column in user table.
If someone needs to use this field, he can simple manipulate weight of his module, in order to run his hook_user before uuid_user implementation.

2. Using $edit table as fallback seems to be OK.

recidive’s picture

recidive commented
Status: Needs work » Needs review

Does this need a re-roll? Let's try re-test!

recidive’s picture

recidive commented
Status: Needs review » Needs work

Ok, I reviewed the patch in #5. Here are some ideas to improve it:

1. do we really need the $insert variable? Can't we just use $user->uuid and unset it if not needed?
2. could you add some comments to the if/elseif chain, to help people follow what's going on?

Thanks!

skwashd’s picture

skwashd commented
Status: Needs work » Fixed

This was applied by recidive in commit a9d70c5a back in February 2011. Thanks for the patch. Closing.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.