user_save() relies on $edit instead of $account

+++ modules/user/user.module
@@ -440,18 +440,15 @@ function user_save($account, $edit = array(), $category = 'account') {
     foreach ($edit as $key => $value) {
-      $account_updated->$key = $value;
+      $account->$key = $value;
     }

@@ -532,26 +531,17 @@ function user_save($account, $edit = array(), $category = 'account') {
+      if (isset($edit['status']) && $edit['status'] != $account->original->status) {
...
         $op = $edit['status'] == 1 ? 'status_activated' : 'status_blocked';

@@ -571,14 +561,16 @@ function user_save($account, $edit = array(), $category = 'account') {
       if (isset($edit['roles']) && is_array($edit['roles'])) {

Shouldn't the entire code rely on $account instead of $edit with that initial copying/overloading?

+++ modules/user/user.module
@@ -571,14 +561,16 @@ function user_save($account, $edit = array(), $category = 'account') {
-      // Build a stub user object.
-      $user = (object) $edit;
-      $user->roles[DRUPAL_AUTHENTICATED_RID] = 'authenticated user';
...
+      // Make sure $account reflects any changes applied to $edit and is
+      // properly initialized.
+      foreach ($edit as $key => $value) {
+        $account->$key = $value;
+      }
+      $account->roles[DRUPAL_AUTHENTICATED_RID] = 'authenticated user';

This looks like a duplicated and no longer needed operation?

Powered by Dreditor.

This looks really really good.

+++ modules/user/user.module
@@ -440,27 +440,18 @@ function user_save($account, $edit = array(), $category = 'account') {
+      if (!$delete_previous_picture = empty($account->picture->fid)) {

Somehow, this variable assignment looks wrong to me. ->fid is empty, if there is no picture yet, so $delete_previous_picture will be TRUE, when ->fid is empty, but just because of the final negation, we're entering the condition. However, the variable contains the wrong/negated value.

Technically, that's a separate issue though. However, my next best thought was that we can entirely remove that $delete_previous_picture negotiation, because we have $account->original->picture->fid now.

+++ modules/user/user.module
@@ -475,24 +466,21 @@ function user_save($account, $edit = array(), $category = 'account') {
-      // Do not allow 'uid' to be changed.
-      $edit['uid'] = $account->uid;

We can still ensure this via $account->original->uid

+++ modules/user/user.module
@@ -500,13 +488,13 @@ function user_save($account, $edit = array(), $category = 'account') {
       // Reload user roles if provided.
-      if (isset($edit['roles']) && is_array($edit['roles'])) {
+      if ($account->roles != $account->original->roles) {

It looks like we always updated user roles -- let's not do unnecessary behavior changes.

+++ modules/user/user.module
@@ -518,13 +506,13 @@ function user_save($account, $edit = array(), $category = 'account') {
       // Delete a blocked user's sessions to kick them if they are online.
-      if (isset($edit['status']) && $edit['status'] == 0) {
+      if ($account->original->status != $account->status && $account->status == 0) {

Same here -- if the account's status is 0, then we always need to ensure that the user's session is destroyed.

+++ modules/user/user.module
@@ -532,61 +520,57 @@ function user_save($account, $edit = array(), $category = 'account') {
+      // Update $edit with any interim changes to $account.
+      foreach ($account as $key => $value) {
+        if (!property_exists($account->original, $key) || $value !== $account->original->$key) {
+          $edit[$key] = $value;
+        }
+      }
+      user_module_invoke('update', $edit, $account, $category);
...
+      $edit = (array) $account;
+      user_module_invoke('insert', $edit, $account, $category);

Why don't we use the simple array cast in the update case, too?

Powered by Dreditor.

Alright, thanks for clarifying. user_save() is sufficiently covered by tests, so this patch looks ready to fly to me.

Also, trying to set a better title.

wow, subscribe :-)

Minor nit: there is one leftover instance of the $delete_previous_picture variable, now useless.

Even better, thanks!

Re-rolled, with improvements. (FINALLY found the original issue where I stole this patch from...)

#9: drupal_user_save.patch queued for re-testing.

#12: 999004-user.module.patch queued for re-testing.

+++ modules/user/user.module
@@ -441,27 +441,22 @@ function user_save($account, $edit = array(), $category = 'account') {
+        (!isset($account->original->picture->fid) || ¶

Extra space...

+++ modules/user/user.module
@@ -1452,7 +1445,16 @@ function template_preprocess_user_picture(&$variables) {
-      if (module_exists('image') && file_valid_uri($filepath) && $style = variable_get('user_picture_style', '')) {
+      $style = variable_get('user_picture_style', '');
+      if (module_exists('image') &&
+        file_valid_uri($filepath) &&
+        $style &&
+        image_style_create_derivative(
+          image_style_load($style),
+          $filepath,
+          image_style_path($style, $filepath)
+        )
+      ) {

Also, it seems that this new bit is causing the test failure.
Powered by Dreditor.

Re-uploading @fago's latest patch from #9. No clue why @pillarsdotnet tried to change it. Back to RTBC.

With the patch in #9, if I upload a user image, it shows on my screen as a broken link. With the patch I posted (sorry about the trailing space), if I upload a user image, it shows on my profile and on my posts.

So I should open a separate issue after this patch makes it in?

@pillarsdotnet: Yes, create a separate issue. You can do so right away -- there is no need to wait for this issue to be resolved. Unless, of course, this user image problem was created by the patch in #9 at which point we would need to fix the patch.

#18: drupal_user_save_2.patch queued for re-testing.

#24: drupal_user_save_2.patch queued for re-testing.

Giving this patch another +1. Works for me and fixes a problem in Rules that affects Drupal Commerce. ; )

Just a note so I can keep an eye on this issue because it is going to break the patch for #935592: User picture is deleted after calls to user_save(). It makes sense to get this one in first as it looks like it may fix that other issue too.

does anyone know what the status on this is?
cheers,

Subscribing.

Subscribing.

Gratuitous re-roll for both d8 and d7. No changes other than this one is made with "git format-patch" and without the "-p0" option.

Is there a way to push this issue to ensure that this will be fixed in the next Drupal 7 Version?
(losing user content is a very serious and annoying problem causing a lot of frustrations...)

Is there a RTBC RTBC...? I've no idea why this patch keeps on languishing around in the queue. It

1. is badly required
2. fixes an annoying bug and oversight
3. prevents contributed modules and custom code to work correctly
4. should have been committed before 7.0 already
5. must go into 7.1
6. does not need any more reviews or confirmations or whatnot

Yes, this is a really badly required patch because there are probably some more issues connected to the disappearing user pictures:

#935592: User picture is deleted after calls to user_save()
#1063098: User picture and other image fields dissapear when I save
#1126000: [meta] hook_field_*() called on fields not set in the $entity object (partial updates)

Since this blocks the other issue with user pictures going missing, I'm bumping to critical. That bug has current patches, we can add tests over there.

Like so many other issues in the queue sadly.

Like in the tale of Sisyphus ...
user changing user data ... user picture deleted ... restoring user picture ...
admin changing user role ... user picture deleted ... restoring user picture ...

Please destroy the boulder and make me running up the hill of drupal. :-)

Just for the heck of it.
+1 RTBC.

Suppose this tag still valid

#33: user_save-cleanup-999004-33-d-8.patch queued for re-testing.

I reviewed this patch and it is ready to be committed to D8 from looking at the code. I tried to apply it to my local 8.x branch it failed so I just asked for a re-test from the testbot. This patch can go in quickly now but might need a reroll. Let's see what the testbot has to say.

Will need a quick reroll for D8.

Patch was broken by #61856: In user.module, trim() user-submitted email address before validation. I tried reverting that patch and applying this one, but it still didn't apply, that's all the time I have for this issue today, hopefully someone else has time to re-roll. The number of RTBC critical issues that have been stalled by minor commits on other issues in the past couple of months is unacceptable.

Re-roll of #33 for current 8.x checkout.

Thanks pillarsdotnet!

Appears to break user creation. Note 0 passes in previous green.

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '0' for key 'PRIMARY': INSERT INTO {users} (name, pass, mail, signature, signature_format, status, timezone, picture, init) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8); Array ( [:db_insert_placeholder_0] => mary [:db_insert_placeholder_1] => $S$Cn1gOOmbmAumayw498r6WlsdctyLQdsZyqvG0P14J5sYiqrAmbV8 [:db_insert_placeholder_2] => mary@example.com [:db_insert_placeholder_3] => [:db_insert_placeholder_4] => plain_text [:db_insert_placeholder_5] => 1 [:db_insert_placeholder_6] => Africa/Abidjan [:db_insert_placeholder_7] => 0 [:db_insert_placeholder_8] => mary@example.com ) in drupal_write_record() (line 6847 of /Users/simonroberts/htdocs/drupal8/includes/common.inc).

#49: user_save-cleanup-999004-49.patch queued for re-testing.

+++ b/modules/user/user.module
@@ -572,22 +558,20 @@ function user_save($account, $edit = array(), $category = 'account') {
-      if (isset($edit['roles']) && is_array($edit['roles'])) {
-        $query = db_insert('users_roles')->fields(array('uid', 'rid'));
-        foreach (array_keys($edit['roles']) as $rid) {
+      if (count($account->roles) > 1) {        $query = db_insert('users_roles')->fields(array('uid', 'rid'));
+        foreach (array_keys($account->roles) as $rid) {

Small code style glitch in the patch but I fixed that upon commit. Committed to 8.x.

I committed it with the wrong commit message though; used the one from #935592: User picture is deleted after calls to user_save(). I apologize for that.

I believe this commit has broken user-creation in D8 - the error in #51 is now in core.

Let's roll back to #49 (note that Dries made a minor code style change in #53, but it should be fine to revert the commit in git) and work from there.

Yeah now the patch fails...

Rolled back the patch, and updating the status.

#49: user_save-cleanup-999004-49.patch queued for re-testing.

EDIT: Please ignore and see #62 instead, sigh.

Re-rolled to fix code style glitch, but also changing to CNW.

I did have to change some of the patch to cope with changes from #61856.

Interdiff from #33 also attached.

@pillarsdotnet: Sorry for playing devil's advocate, but you didn't state that you had larger issues re-rolling the patch. If that would have been clear since #33, someone would surely have taken the time to compare and review the re-rolled patch against the original in detail. I don't think that @fago, @catch, or me took the time to review the re-rolled patch in detail because of that missing communication.

I considered twice whether I should re-review the re-rolled patch. In turn, now it's @catch and me who're to blame for having HEAD broken for quite some time - unfortunately, in a situation that's sufficiently heated/hopeless/problematic anyway already.

Everyone, apologies for causing this trouble.

Sorry, too. I did try to run testing locally before updating but it was taking too long, even with mysql running from a ramdisk.

I didn't imagine that Dries would commit without waiting for testbot to finish. but perhaps he was feeling pressured by the multitude of complaints that the whole update process is too slow.

Looking at the interdiff I can't see what is causing the breakage, but there it is.

The issue this line has been introduced via another patch

$success = drupal_write_record('users', $edit);

Reroll of patch removing this line - all user tests have run successfully locally. Otherwise this is the same as #33.

Interdiff between #62 and #66 shows a lot more changes than just one line.

Your interdiff show lines related to translations which are not part of this patch. As stated this is based on #33 and removes the issue with an extra drupal_record_write for user that has been introduced.

So this passes - I know the norm isn't to rtbc your own patches but we live in interesting times.....and we want to get d7 ship shape. Hopefully someone will second this.

Confirming. Also reviewed #66 and it's identical to the original in #24.

Removing bogus tag.

Let's try this again. Committed to 8.x. Moving to 7.x.

Committed to 7.x, too.

I think I was hesitant around this one back before release because it was re-working internals and I was concerned about API breakage. However, in reviewing it more closely, it does indeed just make this function conform to the standard set out by the remainder of the various other entity functions.

Hopefully this is early enough that it won't screw up any module devs. I guess we have a week or so to test it before 7.1 comes out.

Epic win. Thanks everyone. : )

Marked as a duplicate: #935592-41: User picture is deleted after calls to user_save()

Was this ever committed to core? Has it somehow been re-triggered? I never had these versions of Drupal, started on 7.22, yet I am experiencing this issue.

I have three rules, meant to update roles based on the status of a field, when the user profile is saved. They work but also delete the user picture.