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.
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 →
.
Update storage controllers to end transactions before invoking hooks
Update documentation of hooks and field methods to reflect this
-
Not really, but the intended / best practice hook usage is clarified.
Entity insert/update hook move out of database transactions
#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 →
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.
Needs work
11.0 🔥
Changes an existing API or subsystem. Not backportable to earlier major versions, unless absolutely required to fix a critical bug.
Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.
Not all content is available!
It's likely this issue predates Contrib.social: some issue and comment data are missing.
My 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.
