Missing hook_entity_presave()

I like.

No, this certainly won't work as expected and the form hack demonstrates that.

+    $edit = (object) $edit;
+    module_invoke_all('entity_presave', $edit, 'user');
+    $edit = (array) $edit;

$edit won't contain a complete user object, e.g. it may contain a single value that's updated only. I guess cloning $account and merging in $edit would work though.

What a mess. First task for D8: Drop that $edit argument. Stupidly insane.

One more try.

>What a mess. First task for D8: Drop that $edit argument. Stupidly insane.
Agreed.

Attached patch makes user's entity presave hook work like the others.

array_diff_assoc() fails if $account or $edit contain scalar values.

Attached patch should fix the remaining 2 failures.

hm, array_diff_assoc() should work with scalar values, not? However, the loop is easier to read+understand anyway.

>+ // array_diff_assoc() fails if there are scalar values in $account or $edit.
I don't think we need that comment.

+    foreach ((array) $account as $key => $value) {

We can directly loop over the object the same way, no need to cast to an array.

>+        unset($account->$key);

So we are reverting the changes to $account back to $account_unchanged. Why not go with $account_unchanged instead then for the remaining code? Thus, as we have an updated object already, we could use it for calling field_attach_update() too - what would fix this hook and so help #985642: Remove file_attach_load() from file_field_update().

Similarly field_attach_insert() and entity_insert is broken, not sure whether we want to deal with that all in this issue though.

subscribe

Within the scope of this issue, I don't want to fiddle with further changes in user_save(). Certain properties of $account are used further down in the function, and changing that usage is not within the scope of this issue. Let's move that suggestion into #985642: Remove file_attach_load() from file_field_update(), please.

#19: drupal.entity-presave.19.patch queued for re-testing.

>Within the scope of this issue, I don't want to fiddle with further changes in user_save().
ok.

@patch:
I think it's confusing to update $account first and then revert that changes. We have already two variables, so there is no need to go with reverting. Updated patch attached.

ok, works for me.

In case it's not immediately clear for everyone: $account remains unchanged, only $account_updated is updated with $edit and sent to presave hook implementations, and if those hooks changed any properties, the corresponding keys in $edit are updated accordingly afterwards.

This seems like a fairly straight-forward oversight in the API.

However, seriously?

-    // Presave field allowing changing of $edit.
-    $edit = (object) $edit;
-    field_attach_presave('user', $edit);
-    $edit = (array) $edit;

Ugly, but makes sense.

+    // Invoke presave operations of Field Attach API and Entity API. As those
+    // APIs require an updated entity object, create that.
+    $account_updated = clone $account;
+    foreach ($edit as $key => $value) {
+      $account_updated->$key = $value;
+    }
+    field_attach_presave('user', $account_updated);
+    module_invoke_all('entity_presave', $account_updated, 'user');
+    // Update $edit with any changes modules might have applied to the account.
+    foreach ($account_updated as $key => $value) {
+      if (!property_exists($account, $key) || $value !== $account->$key) {
+        $edit[$key] = $value;
+      }
+    }

Ugly, and leaving me utterly mystified why the three lines above that were removed wouldn't be sufficient?

At the very least we need to comment why we are re-implementing (array) and (object) in PHP code. But I would prefer to just go back to the old grokkable thing.

I hope the extended inline comment explains why it needs to be done that way.

Yep, just casting is not enough. I think the comment is fine now.

#24: drupal.entity-presave.24.patch queued for re-testing.

Small note in support of this patch - presave feels a bit useless in user.module at the moment. $edit does not contain the uid, so it is impossible to perform presave actions based on a user.

Thanks, that new comment helps.

Committed to HEAD!

Great, thanks folks :-)

Should there be a followup to solve field_attach_update() not receiving the $entity->original property ? (cf #16, #19 and #985642: Remove file_attach_load() from file_field_update())

Yep, I've rolled a follow-up patch. See #999004: user_save() relies on $edit instead of $account.