'my account' information is not saved. For example drupal.org 'my account' -> 'edit' -> 'Drupal' and 'Drupal contributions' text field.

CommentFileSizeAuthor
#12 user.edit.patch10.25 KBSteven

Comments

moshe weitzman’s picture

Priority: Normal » Critical

i can reproduce this. setting to critical.

jeremy’s picture

I am able to duplicate this on Drupal.org, but when I downloaded the latest source to my development server I was unable to duplicate the problem. Are any errors showing up in the Drupal watchdog logs? Is Drupal running an older version of the profile module?

killes@www.drop.org’s picture

Nothing in the error logs. I don't know whihc version is run, though.

jeremy’s picture

For what it's worth, I only seem to have trouble saving the following fields: "Drupal contributions", "Job title" and "Industries worked in". The rest seem to work.

I tried to delete all the industries I worked in as a test. Interestingly while they are not deleted from 'my account', I no longer show up on the various pages. For example, clicking 'Teaching' I no longer am on that page.

killes@www.drop.org’s picture

Somebody in #drupal reported that the values are saves to the user data column. What I think is that the values arae actually saved, but the old data is restored from old data somehow stuck in the data column. Can somebody check this?

Steven’s picture

I've done some looking around on Drupal.org, and my account suffered from the same problem. It turns out the profile data was stored in the users table's data field as well as the profile_values table.
Editing my account updated the profile_values table, but not the user->data field.

I set my user data to '' manually, and the problem was fixed. Even more: the user->data field does not contain profile data, even after editing my account again.

I think something went wrong with the db updates for profile.module: they are supposed to unset certain fields from user->data, but obviously this didn't happen.

Steven’s picture

I've tracked down this bug. The problem is in user_save:

    user_module_invoke('update', $array, $account, $category);
    $data = unserialize(db_result(db_query('SELECT data FROM {users} WHERE uid = %d', $account->uid)));
    ...
    $query .= "data = '%s', ";
    $v[] = serialize($data);

    db_query("UPDATE {users} SET $query changed = %d WHERE uid = %d", array_merge($v, array(time(), $account->uid)));

This means that fields in the data column can never be unset, only changed.

Because the data column is automatically loaded and unpacked in user_load(), retrieving the data object again before saving is unnecessary. There is also a known bug of profile data in the data column overriding the data in the profile tables, but I'm not sure why this happens. It is probably related to this code though.

To fix it we need to remove the loading of the data column again and ensure all non-standard fields are always loaded (which is already the case, I think). There is also a catch that we should be aware of: all non-standard fields have to be unset before reaching the last line of code, or they will be saved to the data column. We already do this in profile.module's hook_user, but this should be added to the hook_user documentation.
Alternatively, we could have hook_user('update') return an array of fields which were already saved, and which should not be saved to the data column. This would be cleaner, and also make it so that doing a user_save() does not modify the $user object (which could cause bugs later on).

I'll cook up a patch after we decide how to handle this.

Steven’s picture

The profile bug I mentioned is here:
http://drupal.org/node/11505

It happens because profile.module does not overwrite existing $user fields. So if the data column contains profile fields (because they were not deleted in the update), they will be used.

Steven’s picture

The problem with the data column is more complicated than I thought and it affects registration too.

On registration, the data column is built and saved /before/ hook_user('insert') is called. This is because we want to pass a complete user object to hook_user, so we need to do a save/load. Any module-specific fields will be stored in the data column.
In user_save, hook_user('update') is invoked and module-specific values will be correctly saved to the module-specific tables. However, because the data column is only updated, not reset, old values stay there.

I've figured out how to solve the registration problem by simply not saving the data column initially when building the user object to pass to hook_user('insert'). This allows hook_user('insert') to unset the fields it has already handled.

For account editing, the problem is tougher. There are three ways to go:
1) Prevent module-defined fields from being saved to the data column by letting them return the names of the values they've already saved.
2) Change the handling of the data column significantly (e.g. by requiring that all custom fields are set through $user->data['myvalue']='foo' rather than $user->myvalue='foo').
3) Get rid of the data column.

I'm against #3 because the data column provides an easy mechanism for modules to store user-info (it is used by locale, block, ...). And only #1 is small enough to be included in 4.5 really.

#1 still leaves open the issue of how to unset data fields. Luckily, this won't be that much of a problem if we make sure the data column never contains any of the module-handled fields, but having data lingering on is a bad idea. One solution could be to make it that passing NULL as the value for a certain member has the effect of deleting it, if it's a $user->data field.

I'll see if I can cook up a patch for #1.

moshe weitzman’s picture

#1 looks best to me too. #2 is bad because the way you set user properties will be different from how you read them. too confusing. and #3 is too inconvenient.

i think your proposal to delete by setting values to NULL is just fine.

Steven’s picture

The problem with the data column is more complicated than I thought and it affects registration too.

On registration, the data column is built and saved /before/ hook_user('insert') is called. This is because we want to pass a complete user object to hook_user, so we need to do a save/load. Any module-specific fields will be stored in the data column.
In user_save, hook_user('update') is invoked and module-specific values will be correctly saved to the module-specific tables. However, because the data column is only updated, not reset, old values stay there.

I've figured out how to solve the registration problem by simply not saving the data column initially when building the user object to pass to hook_user('insert'). This allows hook_user('insert') to unset the fields it has already handled.

For account editing, the problem is tougher. There are three ways to go:
1) Prevent module-defined fields from being saved to the data column by letting them return the names of the values they've already saved. This matches the design of user.module, where every field except a certain list is saved to the data column.
2) Change the handling of the data column significantly (e.g. by requiring that all custom fields are set through $user->data['myvalue']='foo' rather than $user->myvalue='foo'). This still doesn't solve the problem of unsetting or not having a complete $user->data member after e.g. a form submission.
3) Get rid of the data column completely.

I'm against #3 because the data column provides an easy mechanism for modules to store user-info (it is used by locale, block, ...). And only #1 is small enough to be included in 4.5 really.

#1 still leaves open the issue of how to unset data fields. Luckily, this won't be that much of a problem if we make sure the data column never contains any of the module-handled fields, but having data lingering on is a bad idea. One solution could be to make it that passing NULL as the value for a certain member has the effect of deleting it, if it's a $user->data field.

I'll see if I can cook up a patch for #1.

Steven’s picture

StatusFileSize
new10.25 KB

I've created a patch. For those of you who don't want to wade through the messy text above, here's a summary:

Issue #1:
Drupal 4.4 stored profile data in the serialized user->data column. Drupal 4.5 stores profile data in tables (but user->data is still available and used for other stuff, like locale or themes).
The update from 4.4 to 4.5 didn't remove the old data from the user->data column properly, because there is no mechanism in user_save to do so (it did try to unset the fields, but this has no effect).

The syntax for user_save is:
user_save($user, $array, $category). $array contains the field -> value pairs to change.

Here's what I did to fix this bug:
- Allowed for unsetting of fields in the data column by specifying 'field' => null in $array (uses type-strict check).
- Instead of having modules in hook_user('insert/update') mark the columns they handle by unsetting them, I made them set the value to null. This is consistent with the deletion approach. For core, this affects profile.module, but contrib modules which implement hook_user('insert/update') need to do this too.
- Changed update_80 (the original profile 4.4-4.5 update) to correctly unset the old fields in user->data.
- Added update_108 which executes only if the old update_80 was run which properly deletes all the old profile values from user->data. This is for all those 4.5-RC sites out there, and uses a variable_set/get to track if the old update_80 or the new update_80 was run.

Issue #2:
On registration, hook_user('insert') is invoked after saving the data column. This means that any module-specific data is put into the data field. We cannot move hook_user('insert') higher up, because before that point, we do not have a complete $user object yet.

The fix I implemented is to not set the data field when the $user object is created, then invoke hook_user('insert'), and then set the data column with the remaining stuff.

I also discovered a tiny bug where percentage signs were being duplicated in user data. This was a bit of escaping which had become unnecessary and was causing bugs. I fixed it too.

dries’s picture

Committed to HEAD. Does something need to be done to tidy up user accounts or will they be 'fixed' as soon they are saved again?

Steven’s picture

The changes to updates.inc force a load/save of the bad accounts, which automatically strips the old profile fields from user->data.

Anonymous’s picture