The bug is reproduced having the rules and domains modules installed and creating an action that react to the content creation event, but could happen with many modules combinations if there are 2 or more calls of node_save with the same node in the same execution.
The problem is at node.module (line 1134)
module_invoke_all('node_' . $op, $node);
module_invoke_all('entity_' . $op, $node, 'node');
// Update the node access table for this node. There's no need to delete
// existing records if the node is new.
$delete = $op == 'update';
node_access_acquire_grants($node, $delete);
As an example of the bug i will describe how acts the Rules module:
1. In a create node form we add a new node
2. The execution will call for first time the node_save to store the new node.
3. When module_invoke_all('node_' . $op, $node); is invoked the rules module will react to an event and will execute an action, this action could want to store some data to the node and as consequence will invoque node_save or entity_save of type node.
4. This will call node_save function and here start the problem. The second execution will have created the node but not the node_access grants, the node will consider the second execution as an update and will call the node_access_acquire_grants($node, $delete); with $delete parameter as true but this was not created for the first execution of node_save.
5. The second call of node_save will be the first one in make a deletion of node_grants to guarantee the correct recreation and will execute:
DELETE FROM node_access WHERE (nid = :db_condition_placeholder_0)
and
INSERT INTO node_access (nid, realm, gid, grant_view, grant_update, grant_delete) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5)
6. When the first call of node_save continue after the module_invoke_all will consider $delete parameter as false because $op = insert and $delete = $op == 'update' is false. Then node_access_acquire_grants($node, $delete) will not make the grants deletion before trying to insert the grants and this result in constraint violation because the second node_save was ahead with the node_access grants insertion.
Here the PDOException:
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '157-0-all' for key 'PRIMARY': INSERT INTO {node_access} (nid, realm, gid, grant_view, grant_update, grant_delete) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5); Array ( [:db_insert_placeholder_0] => 157 [:db_insert_placeholder_1] => all [:db_insert_placeholder_2] => 0 [:db_insert_placeholder_3] => 1 [:db_insert_placeholder_4] => 0 [:db_insert_placeholder_5] => 0 ) in node_access_write_grants() (line 3334 of /home/pablo/fiat-concesionarios/modules/node/node.module).
There are two solutions in my head for this problem.
- anytime a node_access_acquire_grants($node, $delete); pass $delete as true to force the clean of grants for each execution.
- the second is to execute node_access_acquire_grants($node, $delete); before module_invoke_all calls. I attach a patch with this change.
There are a issue reported in the domains contrib module with the same problem:
https://drupal.org/node/1140158#comment-4399872
Comments
Comment #1
fagoWhile it is not ideal to call node_save($node) during node-insert, it is needed in order to achieve some use-cases - e.g. the often requested "put the node nid into the node title". In order to avoid any problems, Rules tries to be the last module reacting on node-insert. However, obviously this doesn't cover node-grants.
Part of the problem is, that we have no distinction between using node-insert/update to store additional node properties and reacting on node-insert/update. For the reaction part the node_save() basically should be already completed, but as we have both mixed-up we cannot know.
@fix:
Moving the hooks is imho not ideal, as some modules might rely on the ordering. So what about the attached patch? It should work, as $node->is_new has to be already unset if a sub-sequent save happened in the meanwhile.
Comment #2
fagoComment #3
citlacom CreditAttribution: citlacom commentedIs for Drupal 7 version.
Comment #4
fagoPatches have to go into d8 first, but can be backported afterwards. See http://drupal.org/node/767608
Comment #5
sunThis patch touches a pretty complex problem space. We better have tests for that.
Comment #6
themoep CreditAttribution: themoep commentedExperiencing the same error caused by Rules, I applied fagos patch with success, but got a notice telling me $node->is_new is undefined when saving a node that was subject to a 'after saving new content'-Rule.
I fixed it by checking if the variable is actually set, but I am not sure if that's the way to go.
Comment #7
fagoThe isset() check doesn't make much sense as $node->is_new is initialized in the beginning of node_save().
Actually, this was problem is triggered by Rules as it has unset the is_new property before triggering another save. I've committed a fix that properly sets it to FALSE instead, what should fix your notice with #1.
Comment #8
fago#1: node_is_new.patch queued for re-testing.
Comment #9
themoep CreditAttribution: themoep commentedYou are right, I misunderstood the inner workings of the rules module. Setting is_new FALSE in rules.state.inc makes more sense than checking it in node.module.
Comment #10
xjmTagging issues not yet using summary template.
Comment #11
klausiComment #12
catchI'm downgrading this from major, since calling node_save() from hook_node_insert() is a very unusual use case. The patch from #1 looks fine though, however this definitely needs some test coverage to demonstrate the bug.
Comment #14
nagiek CreditAttribution: nagiek commentedsubscribe.
use case is a Rules action that creates a new node then saves a reference to the original node in the new node. It can probably done another way, but it would be nice to have in Rules.
Comment #15
HnLn CreditAttribution: HnLn commentedother use case: I have the view_unpublished module enabled (grants to view unpublished content) and a rule that sets the status based on a field. When creating a node I get the constraint error, patch 6 fixes it for me.
I'll also reference the issue in the view_unpublished queue to this one.
Comment #16
firebird CreditAttribution: firebird commentedThe patch in #1 doesn't fix the error for me. The problems I'm having have to do with the Rules-module as well. It appears that Rules invokes content_access_node_access_records(), which in turn eventually runs node_access_write_grants. The actual call for the node being saved happens later, but at that stage the grants table has already been written to, and node->is_new is still TRUE.
See attached trace of the function calls.
I'm thinking of fixing this in node_access_write_grants: We could keep a static array of grants already written to the database, and do a check against the array to see if we should save or skip the current grant being processed.
What do you think?
Comment #17
firebird CreditAttribution: firebird commentedAnd then I found this:
Looks like someone has tried to prevent the issue before. The problem is that this function, in my installation at least (due to being called by rules_invoke_event()), runs before the call made by node_save(). So... content_access shouldn't call node_access_acquire_grants() if it's going to be called by node_save() in the near future? That's not going to work.
Comment #18
firebird CreditAttribution: firebird commentedRight. Unfortunately, I really don't have time to carefully consider the implications of changes to the core, seeing as the customer site that's barfing errors when trying to add content was meant to be fixed yesterday.
So, here's the "additional static check" patch for D8, and the back-port to D7. It works for me.
Comment #19
AaronBaumanTested #18 on D7, doesn't solve the issue. (I didn't test against D8, but I'd be surprised if it works, since they're basically identical.)
I think the logic fault is in calling
serialize()
on the $grant array.There's no reason to serialize the entire grant string, and doing so sidesteps the real issue.
If the issue is a duplicate key, we need only keep track of the key.
I've replaced
with
Also, since we may not always have values in our insert query, I've wrapped
execute()
in a conditional. (For this i used a simple flag. If there is a function to retrieve the protected $insertValues property from the insert query object, I could not find it.)Comment #21
janchojnacki CreditAttribution: janchojnacki commentedI have made a patch without any serialization that is working for me. Tested with Content Access, Content Access Rules integration, Views Bulk operations.
Comment #22
ErnestoJaboneta CreditAttribution: ErnestoJaboneta commented#21 worked for me for D7. No issues so far.
Comment #23
AaronBaumanPatch in #21 still allows for the possibility of executing an insert query with no values.
Comment #24
McGo CreditAttribution: McGo commentedfacing the same problem, patch in #21 fixes the problem for me.
Comment #25
shenzhuxi CreditAttribution: shenzhuxi commented#21 works
File attached is #21 working with drush make.
Comment #26
AaronBauman#25 still allows for the possibility of executing an insert query with no values.
This patch adds a flag to make sure that an empty execute() doesn't fire.
Comment #28
vmd111 CreditAttribution: vmd111 commentedAfter removal of module nodeaccess with the following error was encountered
#6 patch for me OK.
#6 patch for 7.14 OK. too.
Comment #29
jamesfk CreditAttribution: jamesfk commentedPatch 26 is working for me on a new install of Drupal 7.14 where I was having problems with a conflict between a Rule and content_access and field_permissions.
Comment #30
michaeldhart CreditAttribution: michaeldhart commentedconfirming that patch in #26 also solves the same problem for me. D7.14 with rules and content_access conflict.
Comment #31
unic CreditAttribution: unic commentedPatch from #26 works for me.
Comment #32
firebird CreditAttribution: firebird commentedI'm getting a "corrupt patch" error when trying to 'git apply' the patch from #26. Here's a re-rolled patch for 7.14.
Comment #33
Dean Reilly CreditAttribution: Dean Reilly commentedAs Damien Tournoud pointed out in #1352924: Node grants when triggered twice, return PDO exception I think the patches posted in reply to this issue are ultimately trying to cover up the underlying issue that these hooks are being fired out of order. If we ever have a case where the node hook saves a node object but doesn't update the node argument passed to the hook there's no gaurantee that the correct information will be written to the database. The code is also getting fairly complex for what should be a fairly simple operation.
The order of the hooks has already been rearranged in Drupal 8 in this issue #1361234: Make the node entity a classed object. So I'm changing the version back to 7.x and I hope to see this backported. While this does represent an API change I'm having a hard time trying to think of a use case which would be broken by this.
I've started working on some tests for 8.x and 7.x. Currently:
As we now have tests and a patch I'm also marking this issue as needs review. If the community can review my tests and agree if they're an accurate representation of the problem then we can proceed to make a decision whether it's worth the api change to ensure these tests pass.
Comment #34
Dean Reilly CreditAttribution: Dean Reilly commentedTemporarily changing the version so that we can run tests against this patch.
Comment #35
Dean Reilly CreditAttribution: Dean Reilly commentedComment #36
fagoGreat work on the tests!
True. For the node-grants to reflect the update as well, we'll have to fix the ordering and move node access grants before the insert/update hooks.
Comment #37
7wonders CreditAttribution: 7wonders commentedPatch on #32 worked for me.
Comment #38
Dean Reilly CreditAttribution: Dean Reilly commentedJust to clarify: we know the patch in #32 will not pass the tests I posted in #33. Can people please test the patch I've attached to this comment which includes the tests (with the comments tidied up a bit) and the OP's patch.
This will solve the problem outlined in the original post but may introduce new bugs (although this is incredibly unlikely) as the order that grants are written to the database has now changed. It's therefore very important to test any content access aspects of your site after applying the patch (and creating or updating content - this patch will have no effect on grants retospectively.)
If you disagree with the validity or applicability of the tests that's also helpful to know, so please post a comment.
Comment #39
pluess CreditAttribution: pluess commentedI can confirm that patch #38 solves the problem introduced by using the view unpuglished module. I also have a number of hand coded access ruels which are working fine with the patch.
Comment #40
FranckV CreditAttribution: FranckV commentedAbsolutely brilliant, [#38] solved my problem on a rather complex web application where I had the message :
when running a rule that creates a new node with two related new nodes...
Thank you !
Comment #41
Dean Reilly CreditAttribution: Dean Reilly commentedI've also done some testing myself with content access and its rules integration and it seems to be working fine. Given that this change has already happened in Drupal 8 and was originally proposed over a year ago I'm going to change the status of this ticket to reviewed.
If any of the core maintainers disagree and feel that this needs more testing I think it might be productive to set some testing goals as it all feels pretty ad hoc at the minute.
Comment #42
andrewbelcher CreditAttribution: andrewbelcher commentedI encountered the same problem, this time with Rules and OG Access. The patch in #38 solved it for me.
Comment #43
DewanCodes CreditAttribution: DewanCodes commentedYes, that is solved for me too , :) . Thanks
------------------- this code ----------------
if (!empty($grants) && count(module_implements('node_grants'))) {
$query = db_insert('node_access')->fields(array('nid', 'realm', 'gid', 'grant_view', 'grant_update', 'grant_delete'));
foreach ($grants as $grant) {
if ($realm && $realm != $grant['realm']) {
continue;
$has_values = FALSE;
$is_existing = db_query("SELECT nid FROM {node_access} WHERE nid = :nid", array(':nid' => $node->nid))->fetchField();
if(!$is_existing) {
$query = db_insert('node_access')->fields(array('nid', 'realm', 'gid', 'grant_view', 'grant_update', 'grant_delete'));
foreach ($grants as $grant) {
if ($realm && $realm != $grant['realm']) {
continue;
}
// Only write grants; denies are implicit.
if ($grant['grant_view'] || $grant['grant_update'] || $grant['grant_delete']) {
$grant['nid'] = $node->nid;
$query->values($grant);
$has_values = TRUE;
}
}
// Only write grants; denies are implicit.
if ($grant['grant_view'] || $grant['grant_update'] || $grant['grant_delete']) {
$grant['nid'] = $node->nid;
$query->values($grant);
if ($has_values) {
$query->execute();
}
}
$query->execute();
}
}
}
}
-------------------
above code working against the issue :
-------------------------------------------
/PDOException/: SQLSTATE[23000]: Integrity constraint violation: 1062
Duplicate entry '383-0-all' for key 1: INSERT INTO {node_access} (nid,
realm, gid, grant_view, grant_update, grant_delete) VALUES
(:db_insert_placeholder_0, :db_insert_placeholder_1,
:db_insert_placeholder_2, :db_insert_placeholder_3,
:db_insert_placeholder_4, :db_insert_placeholder_5); Array (
[:db_insert_placeholder_0] => 383 [:db_insert_placeholder_1] => all
[:db_insert_placeholder_2] => 0 [:db_insert_placeholder_3] => 1
[:db_insert_placeholder_4] => 0 [:db_insert_placeholder_5] => 0 )
in/node_access_write_grants()/ (line /3417/ of
//home/devaetde/public_html/modules/node/node.module/).
--------------
Comment #44
perforator CreditAttribution: perforator commented+1
#38 helped me too on my project with some complex node_access_rebuild() calls
Let's hope it will get upstream soon. I dislike patching core.
Comment #45
marcingy CreditAttribution: marcingy commentedThis needs to go into d8 first
Comment #46
Dean Reilly CreditAttribution: Dean Reilly commentedAs I pointed out in #33 and can be seen here: http://drupalcode.org/project/drupal.git/blame/51c004aba9c4ea247c5d2351b... this change has already gone into drupal 8.
Comment #47
David_Rothstein CreditAttribution: David_Rothstein commentedThis patch looks a little too scary to commit right before the Drupal 7.15 release, sorry. So, let's hold off and look at it again in a week or so.
At first glance, though, I'm iffy on the idea of moving those hooks around in D7... I can't think of a specific example at the moment, but certainly someone could write a hook_node_access_records() implementation (in either contrib or custom code) that checks a property of the $node that was only added/updated in hook_node_update()? This patch could have serious consequences in that case.
Comment #48
Dean Reilly CreditAttribution: Dean Reilly commentedHmm, good point. I've updated the tests to reflect this possibility. The first two cases are the same as above and will fail in drupal 7. I've added in two new cases that cover the situation you describe and which fail in drupal 8.
To summarise the difference between the two orderings:
Drupal 7 Order
Requires the node that is passed to hook_insert() and hook_update() be representative of what the node access records should be based on after the hooks have finished firing. Furthermore, currently the node can't be saved during the hooks or it will cause the error outlined in the first post.
Drupal 8 Order
Requires the coder to tell drupal if new access information needs to be written to the database.
I'd argue drupal 8 is the simpler of the two and is more in keeping with how the hook work (i.e. changes made to the $node object aren't permanent in any other way). Further more the drupal 7 way means that loading the node and writing access information based on it can cause the access records to change which seems counter intuitive.
However, if we want to stick with the Drupal 7 method we could solve the issue described in this thread another way. We could statically cache every nid that is passed to node_access_acquire_grants() and force a delete if the nid has been passed previously. This has the negative side effect of possibly performing unneccessary writes though. :(
Another alternative would be just to declare that this can't be done and look into rewriting the contrib modules so that this situation doesn't arise. I've not spent that much time thinking about this scenario though so I'm not sure how viable this is. One situation that does spring to mind though is if the hook_update() wanted to change something else other than the access records and use node_save() to store those changes I'm not sure how you would do this.
On the other hand if we do switch to the drupal 8 order there is a very real possibility that we'll break code (or worse, write incorrect access records). Given this I'm going to revert the issue to 8.x and active to get some more discussion on how we should tackle this.
Comment #49
Dean Reilly CreditAttribution: Dean Reilly commentedDrupal 8 tests.
Comment #50
mackpipe1 CreditAttribution: mackpipe1 commentedPerfect, the # 38 worked for me initially because I worked my first rule was atualizar a field after creating a node. and it worked, I security checking the other rules to see whether or because the bug has been fixed
Comment #51
Dean Reilly CreditAttribution: Dean Reilly commentedI had a thought this morning in the shower. @Fago is right when he says:
So how about we introduce two new hooks (hook_post_insert() and hook_post_update() say) which happen after the access records have been written. Then the modules which are experiencing this issue can switch to using these hooks and we don't break any backwards compatibility.
I'll update the patch to add in these new hooks and adjust the tests tonight.
It may also be worth adding in a hook in a similar position for Drupal 8 as it will allow additional database writes to be avoided in certain circumstances.
Comment #53
Dean Reilly CreditAttribution: Dean Reilly commentedHere's the patch with the new node hook and the tests updated to use it.
Comment #54
tim.plunkettThis needs to go into D8 first.
Comment #55
Dean Reilly CreditAttribution: Dean Reilly commentedD8 has moved the edit and insert hooks to where my proposed new hook would be in D7 already. I've opened up a new issue to discuss and document that here #1729812: Separate storage operations from reactions as I think it might be getting too far from the original issue for this thread.
We should maybe wait for one of the core team to confirm that the change was intentional in D8 and will stay before introducing the hook to D7.
Comment #56
agentrickardComment #57
fago@fix:
According to hook_node_insert() docs the hook is for respond to the creation of a new node. However, with the node grants to be not written yet the node is not completely created yet, thus this is a bug. Fortunately, it's already fixed for d8 so we only have to add tests there.
I agree this changes things a bit and hypothetically could influence modules, however I don't see the described scenario as a very logical flow a module would follow. So I doubt there are many issues with the patch, but still doing a change record would be probably a good idea.
Still, we need to get the tests into d8 as well. So I've look into that. However, I think the tests could be simpler by just testing the node_save() in hook_node_insert() with the node access module activated as this is enough to trigger the error. So I've done that test, it fails for d7 but passes for d8.
Attached is the patch for d8 (test-only).
Comment #58
fago..and two patches for d7, one including only the tests and one including the fix and the tests.
Test-only patch should fail.
Comment #59
fagoComment #61
Dean Reilly CreditAttribution: Dean Reilly commentedThat's true. I complicated things somewhat as I wanted to show that it's also possible to have no error be thrown but the wrong information written to the database.
I also included a test for the update hook as I seem to remember that the approach in #32 sort of works if we're only ever inserting nodes (but it's probably also not a good idea for other reasons).
Comment #62
fagoComment #63
fago#58: node_insert_save_d7.patch queued for re-testing.
Comment #64
fago#58: node_insert_save_d7-tests-only.patch queued for re-testing.
Comment #65
fagoRe-uploading d7 patches so they really get tested with d7, not d8.
Comment #66
fagoyeah, #32 is making it work with the symptoms instead of fixing the root of the problem. I'd prefer to fix the real problem being node creation is not completed when the hook is invoked.
Comment #67
fagoPatch for 8.x is in #57, patch for 7.x in #65.
Comment #69
klausiThe D8 test only patch from #57 still applies and looks good.
Comment #70
catchThe test-only patch from #57 could use extra comments to explain why that behaviour is being tested at all.
Comment #71
klausiAdded this sentence to the test method:
"This test ensures that a node has been fully saved when hook_node_insert() is invoked, so that the node can be saved again in a hook implementation without errors."
Comment #72
klausiAdded an @see \Drupal\node\Tests\NodeSaveTest::testNodeSaveOnInsert() back reference to the test hook implementation.
Comment #73
xjmSo per #57 this is already fixed in D8, presumably following the entity conversion or such, so the test-only patch should pass. Those docs cleanups look helpful to me; let's make sure they get backported to the D7 test as well.
Comment #74
webchickOK, the comments on that test make sense to me.
Committed and pushed to 8.x. Moving back to 7.x, but whatever solution here is definitely going to need David's sign-off.
Marking... "needs work?" Not sure. We have some pre-existing 7.x patches in the mix here, but I'm not sure they incorporate those tests from 8.x.
Comment #75
klausiRerolled for Drupal 7, copied all test comments from the D8 patch.
Comment #77
fago#65: node_insert_save_d7.patch queued for re-testing.
Comment #78
fago#65: node_insert_save_d7-tests-only.patch queued for re-testing.
Comment #79
fagoLooks like that has been taken over from d8 as well.
Comment #80
klausistupid klausi. I should really not rely that much on testbot.
Comment #82
klausiOk, now I actually tested this locally. I forgot to remove the is_new flag on the node in the test hook implementation. Also added a comment why that is needed.
Comment #83
fagoYep, that's needed in d7. Patch looks good now.
Comment #84
Dean Reilly CreditAttribution: Dean Reilly commentedJust wanted to say it's great to see this issue gaining some momentum! Thanks for all the work, everyone.
Comment #85
agentrickardWe need a change notice on this one. The API change is subtle, but it does exist. In the past, it would be possible to alter node access records in hook_node_insert() or hook_node_update() -- by, for instance, removing OG settings from the $node.
Now, you are forced to use hook_node_access_records_alter() because any $node alterations will not apply to grants records.
This is all fine, since that's what hook_node_access_records_alter() is for, but I suspect we'll run across a rogue module or two that don't use that hook and see "broken" behavior after this lands.
Comment #86
compa CreditAttribution: compa commented+1 works for me on 7.16
Comment #87
manuelBS CreditAttribution: manuelBS commentedpatch #82 works for me. Thanks!
Comment #88
David_Rothstein CreditAttribution: David_Rothstein commentedPer my earlier comment in #47, I'm pretty worried about this. This isn't really a small side effect that we can just put in a change notice; rather it can actually affect the security of people's sites.
Here is some pseudo-code that I'm thinking of. This particular case is unrelated to hook_node_access_records_alter() (this kind of code would not make sense to go there):
I don't think there's anything particularly wrong with the above code... Perhaps the
$node->mymodule_settings = mymodule_get_settings($node)
line more properly belongs in a hook_node_presave() implementation, but there's nothing fundamentally wrong with having it in hook_node_update() in Drupal 7, and for a developer writing custom code for a client that isn't going to be altered by other modules I wouldn't really expect them to know (or care) about that.Won't the above patch completely break any code along those lines, with possible exposure of a site's private content as a result?
Comment #89
agentrickardYes, the above change will break things. So if this goes into D7, it would have to be a security release with a pretty large warning, IMO.
In your pseudo-code case, I think you actually do have to move your code to hook_node_access_records(), or your storage to hook_node_presave(). While the code is "valid" now, it wouldn't be if the API changes.
Comment #90
Dean Reilly CreditAttribution: Dean Reilly commentedThere is a fairly large problem with the code in #88 which is the node_access_records hook depends on the node_update hook so whenever node_access_rebuild() is called it's going to do the wrong thing.
Now your code could be changed to fix this while still breaking with the patch above by adding a node_load or something. I think what's important about this though is that it highlights how easy it is to make such a mistake. I'd much rather have drupal fail hard in this scenario and force the developer to think about this than work for one use case and not another (which might not be tested). I think the patch above would be a step in that direction.
Comment #91
xjm#88 is a big API change and is not backportable. It's different from the current D8 implementation, and I guess I need to look closer at that, but this will break contrib as it stands.
Comment #92
fagohm, indeed - I agree that the code from #88 would be problematic and while it's looking wrong in the first place, it might make sense to write something like that. So I agree that changing the hook order in d7 is a nogo.
So, let's move back to a workaround like #1 for d7?
Comment #93
Dean Reilly CreditAttribution: Dean Reilly commentedAs firebird pointed out in #16 the solution in #1 does not work.
Also as discussed earlier, if we can't reorder the hooks then the only way to solve this issue is to introduce new hooks in the correct position (see my patch in #53) and have contrib update its code to use those hooks.
Comment #94
agentrickardIt's too late in D7 to update the API in that way.
Comment #95
Dean Reilly CreditAttribution: Dean Reilly commentedThat's very unfortunate. In that case we need a fairly large warning in the documentation not to use node access records with modules which save the node again in hook_insert or hook_update as the wrong access information may be written to the database.
Comment #96
agentrickardModules shouldn't be doing that, IMO.
Comment #97
Dean Reilly CreditAttribution: Dean Reilly commentedThere is another possible solution. It only occurred to me yesterday but it ended up being quite similar to firebird's patch in #18. We maintain a static variable of all the nodes which have had access records written and what vid has been processes and only attempt to set new access records a second time if the vid is higher.
This means the first write (with the latest access information) in a series of writes triggered by node insert or update hooks will be the one used. I've included fago's test from #57 and have also run it against my larger set of tests from #33 which all passed too.
However, the order of the hooks is maintained so David_Rothstein's code in #88 will still work.
Unlike firebird's patch I've moved the if statement to the node_save so that any other code using the node access functions won't be affected. We also check the vid in case a node is saved twice in separate transactions - the vid will always increase when this happens so that should all be the same as before.
The downside to this approach is the increased memory footprint of having to maintain the variable and some lost cpu cycles to the if statement but this might be the best approach if we can't reorder the hooks or introduce new ones.
Comment #98
Dean Reilly CreditAttribution: Dean Reilly commentedComment #100
StoraH CreditAttribution: StoraH commentedComment #101
klausiwhy did you set it to RTBC without a comment?
Comment #102
StoraH CreditAttribution: StoraH commentedWhoops, didn't mean to. I started to post from my mobile and it must have posted when click ok, sorry. But I did test the patch http://drupal.org/node/1146244#comment-6644078 and it solved my problem I had with the content access modules and rules (http://drupal.org/node/1097248).
Comment #103
Dean Reilly CreditAttribution: Dean Reilly commentedUnfortunately the patch in #82 has already been ruled out as too big an API change for D7.
I'll try and track down what caused my patch in #97 to fail when I next can. If anyone else would like to take a swing at it before hand please feel free.
Comment #104
gnucifer CreditAttribution: gnucifer commentedThis solution suggested by the Issue summary:
does not break Drupal 7 API compatibility though. The only downside as I see it would be a neglectable performance degradation for node inserts. But the current situation is much worse IMHO. Tried patching my local installation of drupal just setting $delete = TRUE in node_save and it resolved all my issues.
Comment #105
anavrin CreditAttribution: anavrin commentedIt works for me.
in line 1139: //node_access_acquire_grants($node, $delete);
changed to:
node_access_acquire_grants($node, $delete = TRUE);
In my case, the problem appeared in the situation when I was saving a new node (creating a new node) and I also had a value of a field in form set to value which was triggerring a rule (I use Rules module). By default the node should not be published. But if the field (in my case called workflow - was set to: publish - then the rule should set the node as published).
The solution described above fixed my problem.
Comment #106
Dean Reilly CreditAttribution: Dean Reilly commentedPassing $delete as true can result in the wrong access information being written to the database as the order of the hooks means that writes are handled in reverse order. Try running my tests in #33 against your patched version and you should see them fail.
We probably need to update the issue summary to outline what we know won't work.
Comment #107
gnucifer CreditAttribution: gnucifer commentedAh, yes ofcourse it does, how silly of me :) Well personally I had to patch core to get rules working, and think it's unlikely it will have any negative effect. But I can understand if this is not something that can be commited.
Comment #108
kingswoodute CreditAttribution: kingswoodute commentedI ran into the same issue just using Rules to update a field value after saving new content.
Reading through this chain, I'm massively impressed with the work you've all being doing. I wish I had a better understanding of core / patches and PHP in general so I could help - maybe one day.
Thanks heaps for all your hard work!
Comment #109
yenidem CreditAttribution: yenidem commented#82 worked for me, thank you very much for your effort, and thank you klausi.
will have the drupal core this patch? I use latest drupal core (7.22), And I applied this patch as manual so will I have to backup these files?
Comment #110
David_Rothstein CreditAttribution: David_Rothstein commentedI ran into a version of this bug that the latest patch (in #97) won't help with. That's because it wasn't a double node_save() that was causing the problem, but rather a direct call to node_access_acquire_grants().
Specifically, the code is doing menu-based access control and trying to rebuild access for a node whenever a particular kind of menu link to that node is saved. The code flow goes like
node_save() => hook_node_insert() => menu_node_insert() => menu_link_save() => hook_menu_link_insert() => node_access_acquire_grants()
. Our node_access_acquire_grants() therefore conflicted with the one node_save() does afterwards. (And we don't really have a good way to work around it, because by the time we are inside hook_menu_link_insert(), we can only get the node via a node_load() and therefore don't have any natural way to know the node was new.)After looking at this issue for a while, I have to say I'm wondering... what's wrong with the first fix that was originally proposed at the very top of this issue (by @citlacom in the original post)? I.e., this:
That makes sense to me and is implemented in the attached patch (we should also merge in the tests from previous patches if this approach is the one we're going with).
The only concern I remember seeing about this is that in the double node_save() case, it will result in the final call to node_access_acquire_grants() being done with the "wrong" node object. However, if you've managed to write a hook_node_insert() implementation that makes some node-related changes (up to and including saving a new copy of the node) that don't then get reflected in the passed-along $node object itself, doesn't your code already have a serious bug? (Because it means that whoever called node_save() in the first place does not wind up with the correct $node object in the end either.)
I guess there's an argument that this patch makes an already-serious bug of that nature even more serious, but I don't know... Curious for any opinions.
Comment #111
Anybody#110 solves the problems in my test environment. That's good firstofall! ;) .I agree that a solution that leads to worse performance is better than no solution in this case, especially because we hopefully learned and fix this better in D8. The points you make are good and this should really be fixed ASAP...
Comment #112
Dean Reilly CreditAttribution: Dean Reilly commentedI believe it was me that raised the objection regarding the possibility of writing incorrect access data to the database. Given that a proper solution to this isn't on the cards, this might be the best we can hope for. Although I am somewhat wary of making this an even easier trap to fall in to.
Comment #113
agentrickard@David_Rothstein
I think the reason for not forcing delete is performance -- why run a delete query when we shouldn't need to do so. In the case of your custom workflow, you should probably pass $delete = TRUE at the module level.
Comment #114
David_Rothstein CreditAttribution: David_Rothstein commentedI don't think there's any performance issue here. This is a single, very fast query, and it happens in the middle of node_save() which runs lots of queries already (including this exact one in the case of updates).
We already do (in fact, that's the default behavior of node_access_acquire_grants()), but it doesn't help. The problem comes because the core code which runs afterwards is passing FALSE.
Comment #115
AnybodyConfirming #114
Comment #116
agentrickardWell, why are you running node_access_acquire_grants() instead of hook_node_access_records_alter(), then?
Comment #117
David_Rothstein CreditAttribution: David_Rothstein commentedBecause we need to trigger a node access rebuild whenever a menu link to the node is being saved (regardless of whether the node itself is being saved at the same time as the menu link).
I guess what was missing from my comment above is exactly how we go from
hook_menu_link_insert() => node_access_acquire_grants()
... We basically look at the menu link path and see if it matches node/* and then pull out the node ID and load it. It's ugly, but I believe it's correct and don't see any other way.We could find a really hacky way to work around the bug here (by playing with static caches and using that to figure out if we're in the middle of a node save or not and then only calling node_access_acquire_grants() ourselves if we're not), but it would be pretty crazy (and fragile).
Comment #118
agentrickardRight. I'm just trying to figure out if we need to change the way the API handles things.
What if we change the db_insert() to a db_merge() here?
http://api.drupal.org/api/drupal/modules!node!node.module/function/node_...
https://drupal.org/node/310085
Comment #119
David_Rothstein CreditAttribution: David_Rothstein commentedHm, that might work. So I think that would mean that if an incorrect $node gets passed along (in the double node-save case discussed at the bottom of #110 and earlier), the results will be weirder than for my patch (it would store a mix of records for two versions of the node, rather than records for the old version of the node only) - but I'm not sure that would be worse? :)
It's kind of an API change too, although perhaps not one that matters in practice.
Comment #120
Dean Reilly CreditAttribution: Dean Reilly commentedI'm not sure static caches would work. Would you be able to distinguish a node_save calling a node_save from two calls to node_save in succession? I think you're quickly going to run into a lot of corner cases.
I don't think we can replace this with a db_merge. What if the list of grants passed to node_access_write_grants() doesn't include a grant currently in the database? At the minute that grant would be removed but if we replace the db_insert with a db_merge then it would remain untouched. This also wouldn't solve the issue of potentially incorrect data being written if the developer wasn't very careful with their hooks.
Comment #121
ts145nera CreditAttribution: ts145nera commented#110 solve problem for me.
I'm using D7 (7.22) with workflow, workflow_access (7.x-1.1+27-dev) and rules (7.x-2.2)
Rules change state of some nodes.
All work fine.
Thank you. Great job
Comment #122
miccil CreditAttribution: miccil commented#110 solve problem also for me.
Comment #123
girishmuraly CreditAttribution: girishmuraly commented#110 solved the problem for me too. It seems the patch applies for Drupal 7.19 onwards.
Comment #124
dakalaFaced this issue when implementing an action with rules (Drupal 7.22 and Rules 7.x-2.3). Applying patch #110 resolved it for me too.
Comment #125
mgifford#110: node-access-records-1146244-110.patch queued for re-testing.
Comment #126
mgiffordSo is this not RTBC for D7 for performance reasons?
Seems to be solving the needs of several folks since #110 was posted.
Comment #127
nasia123 CreditAttribution: nasia123 commented#110 solved my issue also (using Content Access and Rules) , but what will happen if I update to a new Drupal version?
will I have to do this everytime I update drupal core ?
Comment #128
tstoecklerWhat can be done to bring this issue forward? If someone could write-up a list of the contrib use-cases we want to support here maybe I could write some test modules?! I would like to make some progress here but any conceptual input is a bit above my paygrade...
Comment #129
Topcheese CreditAttribution: Topcheese commented#110 solved my issue with the content access module.
Comment #130
cgdrupalkwk CreditAttribution: cgdrupalkwk commented#110 solved my issue with the Content Access Module and Rules as well.
#127, I just updated to 7.23 and did have to re-apply this patch to keep my site from breaking.
Comment #131
julia_g CreditAttribution: julia_g commentedHad this issue with Taxonomy Access and Rules, #110 solved it.
Comment #132
jphelan CreditAttribution: jphelan commented#110 fixed my issue with node access and Rules
Comment #133
mvcmarking as RTBC because many people have tested #110 and as stated in #114 there's no performance issue.
Comment #134
BassPlaya CreditAttribution: BassPlaya commentedThanks to the patch supplied in #110, Taxonomy Access and Rules work together nicely again. Thanks David_Rothstein !
Comment #135
crifi CreditAttribution: crifi commentedMoving back to 7.x-dev.
Comment #136
cimo75 CreditAttribution: cimo75 commented#110 solved issue with domain module
Comment #137
zmove CreditAttribution: zmove commented#110 solved issue with content access module.
Comment #138
rsole CreditAttribution: rsole commented# 110 seems to solve similar issue with Forum Access and Rules.
Thanks David_Rothstein
Comment #139
Train CreditAttribution: Train commented#110 cleared up a host of errors I was getting with Rules. I don't know the long-term effects of it yet though.
Comment #140
Spleshka#110 looks and works also good for me, thanks a lot. Let's get this in core?
Comment #141
manuelBS CreditAttribution: manuelBS commented#110 to core +1
Comment #142
steinmb CreditAttribution: steinmb commentedRemoving tags.
Comment #143
flocondetoile#110 solved issues for me too with content access and rules.
RTBC
Comment #144
Guilro CreditAttribution: Guilro commented#110 solved issues for me too with OG and rules
Comment #145
andypostPlease include this in next core release it really needed for complex sites!
Comment #146
ergophobe CreditAttribution: ergophobe commentedJust to mention that many several people are reporting successfully using this patch in this thread too: [#https://drupal.org/node/1961054]
Comment #147
David_Rothstein CreditAttribution: David_Rothstein commentedThanks for the extensive testing and confirmation that the patch works, but per #110 we still need to merge in the tests from the previous patches.
However, both the patch and tests were RTBC separately, so I guess I can just post that and commit it directly as long as it still works.
Comment #148
David_Rothstein CreditAttribution: David_Rothstein commentedHere's #110 combined with the most recent version of the previous tests (corresponding to the tests that were committed to Drupal 8).
I'll commit this to Drupal 7 as long as it passes/fails as expected.
Comment #150
andypostawesome, let's get this in
Comment #151
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/a4a72dc
Comment #152
manuelBS CreditAttribution: manuelBS commentedGreat that we got it. Thanks for committing it @david
Comment #154
Dean Reilly CreditAttribution: Dean Reilly commentedI kinda feel like I'm flogging a dead horse here but I had a thought about this earlier today.
I think my concern is that, how do you know if your hook_node_insert() has done something which has triggered another node_save() of the node you're working with? It might be that you perform an action which calls a hook which calls a hook which makes the save and your hook never knows of it.
The only way to be completely sure you're doing the right thing now is that anytime you write a hook_insert() or hook_update() which has the capability of triggering other hooks is to do a node_load() at the end of your hook which seems like a massive pain.
Comment #155
System Lord CreditAttribution: System Lord commentedI'm using D734 (includes this patch) and trying to get project/nodeaccess to work. I'm getting this error as well. I've posted my issue with nodeaccess: https://www.drupal.org/node/2427345
I guess my question is is this issue a core issue as stated in this thread and does my error mean this issue is not fixed?
Are all the patches in this thread for the same issue? I'm assuming the latest patch is the final compilation and is always the one to use?
Thanks
Mark
Comment #156
David_Rothstein CreditAttribution: David_Rothstein commentedRe #154, I think this should rarely be necessary since objects are always passed by reference (effectively speaking) in PHP. If you implement hook_node_insert() and pass the $node object to another hook which passes it to another function and so on down the line at which point some code saves the node, the changes should still be reflected in the $node object in your function.
I think that only if someone is doing something weird (like deliberately cloning the node object before changing it) would you not have the correct $node object at the end. It's possible I guess, but it should be very rare and seems like broken code.
Comment #157
David_Rothstein CreditAttribution: David_Rothstein commentedRe #155, yes, the latest patch in this issue is the correct one (and the one that was committed). It's definitely included in Drupal 7.34.
It is hard to know for sure if that issue you posted is a problem with core or with the nodeaccess module... Maybe you can post some more details there about exactly how to reproduce the error (ideally starting from a fresh installation of Drupal core + nodeaccess so that others can easily reproduce) and if it does turn out to be due to a core bug, we can move that issue to the core queue?
Comment #158
System Lord CreditAttribution: System Lord commentedSorry, I wasn't clear. I'm getting the same error as reported in this thread. I'm using D734, so I'm assuming then that this is not fixed. It shouldn't have anything to do with nodeaccess...this thread didn't anyway.
Comment #159
David_Rothstein CreditAttribution: David_Rothstein commentedOK, in that case what are the steps to reproduce this with Drupal 7.34 alone (but without the nodeaccess module)?
A new issue might make more sense either way, but feel free to post more details here for starters.
Comment #160
System Lord CreditAttribution: System Lord commentedHey David, I don't think this error can be reproduced on D734 alone. Just like it wasn't produced alone at the top of this thread and with other subscribers.
In fact many subscribers posted how this patch fixed the error with their module configs "rules", "OG access", "field_permissions" and so on.
I'm not sure how else to describe the error occurrence other than I create a node and save it - all works fine. Go back to same node to edit then save and this error appears. As I stated I do have this issue posted on nodeaccess page, but again nodeaccess seems to be just one of many modules that run into this. Yes, I probably wouldn't get this error if I weren't using nodeaccess, but I don't think that module is the root cause.
I don't know what else I can provide other than what I reported on nodeaccess page: https://www.drupal.org/node/2427345
Comment #161
David_Rothstein CreditAttribution: David_Rothstein commentedOh, good point - it would at least need to involve something like Rules (or other code that re-saves a node in response to a node being saved).
I tried this with core + Rules + Nodeaccess now. I set up a rule that automatically publishes content to the front page after it's created. Tested on Drupal 7.24 and saw the error message. Updated to Drupal 7.34, tried again, and no error message.
I think what you're seeing is likely a bug with the Nodeaccess module, and maybe a very specific configuration of that module (since it doesn't seem like other people are running into it)? You'd probably have to provide more details on the exact configuration changes that were made to the module after turning it on, in order to trigger the error. I don't know if anyone is going to be able to help unless they're able to reproduce the issue also.
Comment #162
System Lord CreditAttribution: System Lord commentedIt's a fair point that nodeaccess issue pages has no other complaint about this issue. Which is why I posted this there first. However, It's because this error is identical with many different environments/modules (above) that I still believe its not a contrib module issue. You say its likely a bug within nodeaccess. I can't discount that possibility, but what about the other modules, use cases, reporting this error? The fix to those was this patch to core, not their modules.
To be honest, David, I've moved on from nodeaccess, but I still want to stick with resolving this. Just not easy when it's not on the front burner anymore.
How'bout I set aside my concern for now. When I get some time I'll rerun some tests to reproduce this.
Thanks david!
Comment #163
pinueve CreditAttribution: pinueve commentedHi all
I do not have node access module, and site does not report any errors, I have a multilanguage site (Lingotek module along with many modules) with 2 contentypes, one is single language, when reacting on rules, only the multilanguage contentype tiggers twice rules (set a data value reacting on a node view), any ideas? I have D734.
Comment #164
Kris77 CreditAttribution: Kris77 commentedPatch in #21 works for me.
I have this modules in my drupal site(7.54): Content Access, Content Access Rules integration, Rules, Views Bulk operations, Node Export.
Without patch export node by BulkOperation: Node Export. Than try to import into new installation by Node Export but give me error.
With patch all its ok.
Patch in #110 is already included in Drupal 7.54
Thanks @janchojnacki