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/Motivation
So far this bug occurs on D7 and D8. But, it's only visible on D7 when using Rules module. Rules have many actions, some of them for changing node's values (like making node published, set a field value, etc...). When using Rules on event 'node save' + an action to change node's value, Rules will trigger the node_save() to save that new value, those resulting in node_save() triggered twice (one from saving the node, and one from this action). Triggering node_save() twice won't cause a problem yet. If you have any module that implements hook_node_grants() (e.g. Content Access), you'll get a nice PDO exception because of trying to insert duplicated key to {node_access} table.
Proposed resolution
_node_access_write_grants() that is called twice (one from normal node save, and one from rules), is the reason behind that exception. Fortunately, Rules will set $node->is_new = FALSE before calling node_save() again. So we can check for it in _node_access_write_grants().
Before:
if (!empty($grants) && count(module_implements('node_grants'))) {
....
}
After:
if (!empty($grants) && count(module_implements('node_grants'))) {
if (isset($node->is_new) && !$node->is_new) {
return;
}
...
}
Original issue on Content Access queue #1097248: PDOException error 1062 when using rules on create node
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 1352924_9.patch | 1.43 KB | good_man |
| #7 | 1352924_7.patch | 1.66 KB | good_man |
| #4 | 1352924_4.patch | 784 bytes | good_man |
| #1 | 1352924_1.patch | 677 bytes | good_man |
Comments
Comment #1
good_man CreditAttribution: good_man commentedI think we should fix it for both D7 & D8? Attached is a patch for D7.
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedOk, the real problem here is that the calls to
node_access_acquire_grants();are happening out of order for the node, because we call this innode_save()*after* having called the other hooks.We could consider moving this call a couple of lines above.
Comment #4
good_man CreditAttribution: good_man commentedLet's see how testbot will react to Damien's approach.
Comment #5
good_man CreditAttribution: good_man commentedI prefer Damien's solution, makes more sense + test pass + solves the issue.
Comment #6
catchWhat happens if hook_node_access_records() or hook_node_access_records() alter is depending on something in one of the hooks that was moved?
Comment #7
good_man CreditAttribution: good_man commentedYour point is right, but I haven't seen a usecase that did that. Anyhow, here is a third approach, that do a check first in {node_access}, then depending on the check do an update or insert.
Comment #9
good_man CreditAttribution: good_man commentedOooops didn't know that update query doesn't take values as insert. The attached patch may need a cleanup but now just trying out the idea to discuss.
Comment #10
good_man CreditAttribution: good_man commentedA quick summary:
- Patch in #1 is a quick-and-dirty fix.
- Patch in #4 (Damien's idea) is better in logic, but can cause some modules to stop working (catch's comment #6).
- Patch in #9 is better IMO. It added another query in case of duplicated node, but it's better than getting a PDO exception.
What do you think?
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commented#9 still has the same problem: we save the grants for the first
node_save()*after* the secondnode_save()occurred: we actually save the wrong data.I don't see how a module could depends on something done in
hook_(entity|node)_(insert|update)()to determine the access records of the node, so I thing #4 is the way to go.Comment #12
good_man CreditAttribution: good_man commentedWell, in #9 at least we don't get an exception. I'm happy with #4, @catch, any usecase you've seen that can be broken after that reorder.
Comment #13
catch@good_man I can't think of a use-case, especially not with insert/update hooks (as opposed to pre-save which fires before this), but also I don't maintain any node access modules.
I'd be completely fine with the minor API change for 8.x (although I'm not sure the rules use case makes this a major bug, I also can't see a reason not to re-order the hooks). Whether it's an API change for Drupal 7 only in theory or in practice too is what concerned me, but I'm happy committing #4 to 8.x, and it is probably OK for backport too.
It would be good to have a test case for this though that fails without the patch.
Comment #14
xjmI'll work on an automated test for this.
Comment #15
xjmAlright, I don't actually understand how to reproduce this issue. Simply calling
node_save()twice in a row with changes that alter node access when a node access module is enabled certainly isn't the cause. What hook implementations is rules using? Code to reproduce (without rules) would work too.Comment #16
xjmTagging.
Comment #17
catchiirc you have to call node_save() from within hook_node_update() to trigger this.
Comment #18
xjmAlright, we discussed this in IRC. Calling node_save() from within hook_node_update() is an edge case (and probably inadvisable). That doesn't mean we don't fix it, but I don't think this should be considered major.
(I'm also a bit reluctant to write a test codifying a questionable practice but that's just me.)
Comment #19
Dean Reilly CreditAttribution: Dean Reilly commentedI'm marking this issue a duplicate of #1146244: node_access integrity constraint violation on module_invoke_all('node_' . $op, $node); as that issue has more people involved and was more recently updated.