Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Modules that want to generalize by entities are stuck on presave, because we gave up too early on adding a hook_entity_presave() because of silly user.module. Now those modules have to implement *every single* different presave hook rather than just hook_entity_presave(). Any time another contrib module adds another entity, those modules have to add support for those as well.
Can we just get this fixed before we reach RC?
Comment | File | Size | Author |
---|---|---|---|
#24 | drupal.entity-presave.24.patch | 14.98 KB | sun |
#21 | drupal_entity_presave.patch | 13.51 KB | fago |
#19 | drupal.entity-presave.19.patch | 14.96 KB | sun |
#15 | drupal.entity-presave.15.patch | 15.06 KB | sun |
#13 | drupal.entity-presave.13.patch | 14.7 KB | sun |
Comments
Comment #1
Dave ReidJust for the testbot right now...
Comment #2
Dave ReidTagging potential modules that this issue blocks...
Comment #4
Dave ReidAnd this one should be just fine since really the user forms should provide 'uid' as a FAPI value field anyway.
Comment #5
sunI like.
Comment #6
fagoNo, this certainly won't work as expected and the form hack demonstrates that.
$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.
Comment #7
sunWhat a mess. First task for D8: Drop that $edit argument. Stupidly insane.
Comment #9
sunOne more try.
Comment #11
fago>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.
Comment #13
sunarray_diff_assoc() fails if $account or $edit contain scalar values.
Comment #15
sunAttached patch should fix the remaining 2 failures.
Comment #16
fagohm,
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.
We can directly loop over the object the same way, no need to cast to an array.
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.
Comment #17
fagoComment #18
naught101 CreditAttribution: naught101 commentedsubscribe
Comment #19
sunWithin 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.
Comment #20
sun#19: drupal.entity-presave.19.patch queued for re-testing.
Comment #21
fago>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.
Comment #22
sunok, 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.
Comment #23
webchickThis seems like a fairly straight-forward oversight in the API.
However, seriously?
Ugly, but makes sense.
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.
Comment #24
sunI hope the extended inline comment explains why it needs to be done that way.
Comment #25
fagoYep, just casting is not enough. I think the comment is fine now.
Comment #26
sun#24: drupal.entity-presave.24.patch queued for re-testing.
Comment #27
the greenman CreditAttribution: the greenman commentedSmall 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.
Comment #28
webchickThanks, that new comment helps.
Committed to HEAD!
Comment #29
yched CreditAttribution: yched commentedGreat, 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())
Comment #30
fagoYep, I've rolled a follow-up patch. See #999004: user_save() relies on $edit instead of $account.