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.Problem
hook_entity/insert/update/delete() are currently used for storage operations and reactions. This is in particular problematic, when reactions involve storing the same objects again and lead to issues in the past, e.g.:
#1433288: Integrity constraint violation when saving a user account after creation
#1146244: node_access integrity constraint violation on module_invoke_all('node_' . $op, $node);
But moreover, it does not work well with our usage of transactions. Right now the transaction includes all those hooks, but any module reacting about an entity being saved should not be included in the transaction of the entity being saved.
Proposed resolution
Use hook_entity_load/insert/update/delete() for reactions only
Use fields and their methods (insert(), update(), getValue) for storage data that is considered being part of the entity, for an example of that see #1980822: Support any entity with path.module.
Remaining tasks
Update storage controllers to end transactions before invoking hooks
Update documentation of hooks and field methods to reflect this
User interface changes
-
API changes
Not really, but the intended / best practice hook usage is clarified.
Entity insert/update hook move out of database transactions
Related Issues
#221081: Entity cache out of sync and inconsistent when saving/deleting entities
#597236: Add entity caching to core
#2134079: Storage-related methods are not invoked for computed fields
Original report by @Dean Reilly
When the node entity was changed into a classed object (#1361234: Make the node entity a classed object) the order of the insert and edit hooks and the writing of node access records was rearranged.
Drupal 8
http://drupalcode.org/project/drupal.git/blob/HEAD:/core/modules/node/li...
Drupal 7
http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/modules/no...
Personally, I think this is a good thing as it avoids the problem described in this issue #1146244: node_access integrity constraint violation on module_invoke_all('node_' . $op, $node);.
However, it does mean we lose the ability to make some quick changes to the node object in the node_insert and node_edit with the aim of changing the node access records (there's a test demonstrating this here: http://drupal.org/node/1146244#comment-6313310). This can still be done by calling NodeStorageController::save() again but that results in multiple writes to the database. You could make the changes in presave hooks but that might have unintended side effects.
I think it's a good thing we lose that ability too. I don't think we should be using these hooks to make changes to access records (and not the node) as it makes the following code a very bad idea:
$node = node_load(1);
$update = TRUE;
node_access_acquire_grants($node, $update);
Which is basically how node_access_rebuild() works.
So, I completely agree with this change but we need to make sure that it was intentional and didn't just slip in under the radar and is going to cause problems down the line. We probably also need to document the change so that people are aware of it.
Comments
Comment #1
sunComment #2
fagoI agree with that as well. Writing the access records is part of the save operation and thus has to happen before the hook. The hook is for reacting on the save and is documented to happen *after* saving.
Related, modules traditionally used node_insert|update to write their module data. That's bad as it mixes reacting on saves with the save operation. We need to differentiate that. Ideally, I'd like to see going all save operations to entity properties or fields and using the existing hooks for reactions only.
If we don't get there for d8, we should think about introducing two separated hooks, e.g. node_storage_insert vs node_insert.
Imo that the node access problem is just a special case of the general separation issue. Thus, re-titling to the general issue so we can discuss that here.
Comment #3
fagoRelated issues:
#1146244: node_access integrity constraint violation on module_invoke_all('node_' . $op, $node);
#1433288: Integrity constraint violation when saving a user account after creation
Comment #4
Dean Reilly CreditAttribution: Dean Reilly commentedGood point. I imagine we've seen less issues in other cases because it's possible to play silly beggars with the ordering module hooks fire in and somewhat work around this (whereas the node access records writing is stuck where it is). It would be nice to solve this properly though as you suggest.
Sounds good. I should be able to spend some time this weekend helping out in whatever way I can. In the mean time can you provide a break down of what would need to happen to achieve this? Is it a case of searching through core and looking for hook_insert/update's which need to be refactored to use fields/properties?
FYI, this is the current proposal for D7 in #1146244: node_access integrity constraint violation on module_invoke_all('node_' . $op, $node);.
Comment #4.0
Dean Reilly CreditAttribution: Dean Reilly commentedCleaning up prose.
Comment #5
fagoAs we've the entity field API now in place, I think we should be able to make entity crud hooks reactions only now.
The only thing that's not covered by field methods is adding a multiple load logic, single load can be replaced by a fields getValue() method. Instead of that you can use the PrepareCacheInterface though.
I'm not sure this would have to include dealing with load() at all though, as load() shouldn't be used for reactions anyway.
I've updated the issue summary.
Comment #6
fagoComment #7
fagoComment #8
effulgentsia CreditAttribution: effulgentsia commentedThis is a pretty foundational decoupling that affects transaction optimization and many subtle bugs, so raising to major.
Comment #9
andypostI'd like to point to other related issue with storage - there's a some logic specific functionality that uses storage to query data (for example #2111357-4: Get rid of comment_count_unpublished() in favor of CommentStorage method) but they are not really swapable because their logic coupled into storage class and probably some manager class.
Probably better to move this to special data-class(service) like suggested in #2068331-32: Convert comment SQL queries to the Entity Query API
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedPer discussion with catch, this is an approved beta blocker.
Comment #11
catchI think I got that wrong. This is only clarifying an existing API limitation, albeit one that's often abused in contrib.
After #221081: Entity cache out of sync and inconsistent when saving/deleting entities those comments are already in core, so downgrading to major for adding extra comments to any other places in the same boat and untagging. If we end up needing to make a small API change, I doubt it'll be high impact enough to warrant blocking beta, but please correct this if it's overly optimistic.
Comment #12
fagoI think we also need to clarify where postSave() belongs, which decides based on its mood where it belongs right now. Then, we need to change the scope of the transactions to match the docs.
Not sure whether this API changes are fine after beta, but if they are that's fine to me.
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commentedI have to disagree with this premise. Most of the time, you do want reactions to be part of the same transaction. I cannot even think about a use case where you would like it to be different.
Comment #14
Crell CreditAttribution: Crell commentedI've had to write hook_node_update implementations that pushed something off to a queue or exit hook several times so as to get out of the transaction. Usually it's when I'm doing something that involves multiple nodes. That almost always results in a race condition that I have to solve using either a queue or hook_exit() (or both).
Unfortunately, I think both use cases do exist.
Comment #15
fagoad #13: Imo the entity save should be treated as an atomic operation, while I don't see the reactions being part of that. E.g. if I write a module that creates a log entry for the change and this results in a failure, it shouldn't rollback the whole entity save. The module's reaction itself may be rolled-back, but why would we roll-back the whole save if it was successful?
Surely, there are use-cases where you need to extend the entity save operation. For those you can still add any custom field to the entity which handles that. Or does that appear as too limited?
If it turns out to be too limited, I'd see us introducing two separate hooks, one for module storage operations part of the transaction, and one for reactions not being part of transactions.
Comment #16
Crell CreditAttribution: Crell commentedfago: If we are going to add more responders, I'd suggest that's a good place to start using events (which are well suited to reactions, but not as much to in-place alters).
Comment #17
tstoecklerI've also hit the use-case in #14 before, but I think being able to roll back the entire node save from a reaction is a more important feature. logging is one example where rolling back the entire save would be overkill but in terms of derivative data that depends on the node not being able to roll back the node save can lead to data integrity issues. I'm not entirely sure all those use-cases can be covered by using entity fields (although I'm not sure about this). So I think if we do want to support #14 (which I would 100% support) then two hooks/events are the way to go.
Comment #18
jhedstromMany of the hooks mentioned have been removed. Tagging for IS update.
Comment #19
andypostNW for summary update
Comment #22
fagoSo we have the user-case for reactions being part of the transaction / storage operatoin and for reactions being not. That means, our system should allow for both. However, the current system only allows for reactions being part of the transaction, but we miss the second. I think adding that would be the best solution here.
I'd propose adding an "onChange" event here, which has a $update boolean just as postSave().
Comment #33
KingdutchMy life would've been a lot easier today if this had been implemented :D There are quite a few parallel discussions of similar issues, so adding those as related in the hope that we may add one more hook to rule them all and not fulfill the xkcd.
Comment #34
KingdutchMissed one
Comment #35
andypost