| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | node system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | needs backport to D7, Needs issue summary update, Node access |
Issue Summary
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
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| pdo_constraint_node_access.patch | 784 bytes | Idle | PASSED: [[SimpleTest]]: [MySQL] 29,462 pass(es). | View details | Re-test |
Comments
#1
While 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.
#2
#3
Is for Drupal 7 version.
#4
Patches have to go into d8 first, but can be backported afterwards. See http://drupal.org/node/767608
#5
This patch touches a pretty complex problem space. We better have tests for that.
#6
Experiencing 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.
#7
The 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.
#8
#1: node_is_new.patch queued for re-testing.
#9
You 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.
#10
Tagging issues not yet using summary template.
#11
#12
I'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.
#14
subscribe.
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.
#15
other 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.
#16
The 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?
#17
And then I found this:
/*** Apply the new grants to the affected node.
*/
function content_access_action_aquire_grants($node) {
// node_save() does implement node_access_acquire_grants() so we don't want
// to execute it again or we'll get a duplicated key exception
if (!isset($node->op) ||
(isset($node->op) && $node->op != 'Save')) {
node_access_acquire_grants($node);
}
}
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.
#18
Right. 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.
#19
Tested #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
<?php$grant_string = serialize($grant);
?>
with
<?php$grant_string = $node->nid . '-' . $grant['gid'] . '-' . $realm;
?>
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.)#20
The last submitted patch, node_access_write_grants-fix_duplicate_keys-1146244-19.patch, failed testing.
#21
I have made a patch without any serialization that is working for me. Tested with Content Access, Content Access Rules integration, Views Bulk operations.
#22
#21 worked for me for D7. No issues so far.
#23
Patch in #21 still allows for the possibility of executing an insert query with no values.
#24
facing the same problem, patch in #21 fixes the problem for me.
#25
#21 works
File attached is #21 working with drush make.
#26
#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.
#27
The last submitted patch, node_access_write_grants-fix_duplicate_keys-1146244-26.patch, failed testing.
#28
After removal of module nodeaccess with the following error was encountered
#6 patch for me OK.
#6 patch for 7.14 OK. too.
#29
Patch 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.
#30
confirming that patch in #26 also solves the same problem for me. D7.14 with rules and content_access conflict.
#31
Patch from #26 works for me.
#32
I'm getting a "corrupt patch" error when trying to 'git apply' the patch from #26. Here's a re-rolled patch for 7.14.
#33
As 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.
#34
Temporarily changing the version so that we can run tests against this patch.
#35
#36
Great 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.
#37
Patch on #32 worked for me.
#38
Just 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.
#39
I 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.
#40
Absolutely brilliant, [#38] solved my problem on a rather complex web application where I had the message :
PDOException : SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1125-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), (:db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11), (:db_insert_placeholder_12, :db_insert_placeholder_13, :db_insert_placeholder_14, :db_insert_placeholder_15, :db_insert_placeholder_16, :db_insert_placeholder_17), (:db_insert_placeholder_18, :db_insert_placeholder_19, :db_insert_placeholder_20, :db_insert_placeholder_21, :db_insert_placeholder_22, :db_insert_placeholder_23); Array ( [:db_insert_placeholder_0] => 1125 [: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 [:db_insert_placeholder_6] => 1125 [:db_insert_placeholder_7] => taxonomy_access_role [:db_insert_placeholder_8] => 1 [:db_insert_placeholder_9] => 1 [:db_insert_placeholder_10] => 0 [:db_insert_placeholder_11] => 0 [:db_insert_placeholder_12] => 1125 [:db_insert_placeholder_13] => taxonomy_access_role [:db_insert_placeholder_14] => 2 [:db_insert_placeholder_15] => 1 [:db_insert_placeholder_16] => 0 [:db_insert_placeholder_17] => 0 [:db_insert_placeholder_18] => 1125 [:db_insert_placeholder_19] => taxonomy_access_role [:db_insert_placeholder_20] => 5 [:db_insert_placeholder_21] => 1 [:db_insert_placeholder_22] => 1 [:db_insert_placeholder_23] => 0 ) in node_access_write_grants() (line 3417 in /home/mywebsite/www/modules/node/node.module).when running a rule that creates a new node with two related new nodes...
Thank you !
#41
I'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.
#42
I encountered the same problem, this time with Rules and OG Access. The patch in #38 solved it for me.
#43
Yes, 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/).
--------------
#44
+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.
#45
This needs to go into d8 first
#46
As 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.
#47
This 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.
#48
Hmm, 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.
#49
Drupal 8 tests.
#50
Perfect, 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
#51
I 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.
#52
The last submitted patch, drupal_8.x-node-hook-order-tests-1146244-49.patch, failed testing.
#53
Here's the patch with the new node hook and the tests updated to use it.
#54
This needs to go into D8 first.
#55
D8 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.
#56
#57
@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).
#58
..and two patches for d7, one including only the tests and one including the fix and the tests.
Test-only patch should fail.
#59
#60
The last submitted patch, node_insert_save_d7-tests-only.patch, failed testing.
#61
That'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).
#62
#63
#58: node_insert_save_d7.patch queued for re-testing.
#64
#58: node_insert_save_d7-tests-only.patch queued for re-testing.
#65
Re-uploading d7 patches so they really get tested with d7, not d8.
#66
yeah, #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.
#67
Patch for 8.x is in #57, patch for 7.x in #65.
#68
The last submitted patch, node_insert_save_d7-tests-only.patch, failed testing.
#69
The D8 test only patch from #57 still applies and looks good.
#70
The test-only patch from #57 could use extra comments to explain why that behaviour is being tested at all.
#71
Added 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."
#72
Added an @see \Drupal\node\Tests\NodeSaveTest::testNodeSaveOnInsert() back reference to the test hook implementation.
#73
So 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.
#74
OK, 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.
#75
Rerolled for Drupal 7, copied all test comments from the D8 patch.
#76
The last submitted patch, 1146244-75-node-save-on-insert.patch, failed testing.
#77
#65: node_insert_save_d7.patch queued for re-testing.
#78
#65: node_insert_save_d7-tests-only.patch queued for re-testing.
#79
+++ b/modules/node/tests/node_test.module@@ -159,3 +159,18 @@ function node_test_entity_view_mode_alter(&$view_mode, $context) {
+function node_test_node_insert(Node $node) {
Looks like that has been taken over from d8 as well.
#80
stupid klausi. I should really not rely that much on testbot.
#81
The last submitted patch, 1146244-80-node-insert-save-tests.patch, failed testing.
#82
Ok, 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.
#83
Yep, that's needed in d7. Patch looks good now.
#84
Just wanted to say it's great to see this issue gaining some momentum! Thanks for all the work, everyone.
#85
We 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.
#86
+1 works for me on 7.16
#87
patch #82 works for me. Thanks!
#88
Per 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):
<?php
/**
* Implements hook_node_update().
*/
function mymodule_node_update($node) {
// Update and save some custom settings on the node. This might do complex
// logic (even make an external API call, etc.) to populate it.
$node->mymodule_settings = mymodule_get_settings($node);
drupal_write_record('mymodule_settings', $node->mymodule_settings);
}
/**
* Implements hook_node_access_records().
*/
function mymodule_node_access_records($node) {
// Call a function which determines access based on the custom settings.
if (mymodule_is_private_content($node->mymodule_settings)) {
// Grant limited access.
}
else {
// Grant full access.
}
}
?>
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?
#89
Yes, 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.
#90
There 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.
#91
#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.
#92
hm, 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?
#93
As 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.
#94
It's too late in D7 to update the API in that way.
#95
That'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.
#96
Modules shouldn't be doing that, IMO.
#97
There 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.
#98
#99
The last submitted patch, wrong-node-access-records-written-1146244-97.patch, failed testing.
#100
#101
why did you set it to RTBC without a comment?
#102
Whoops, 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).
#103
Unfortunately 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.
#104
This 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.
#105
It 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.
#106
Passing $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.
#107
Ah, 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.
#108
I 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!
#109
#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?
#110
I 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.
#111
#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...
#112
I 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.
#113
@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.
#114
I 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.
#115
Confirming #114
#116
Well, why are you running node_access_acquire_grants() instead of hook_node_access_records_alter(), then?
#117
Because 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).
#118
Right. 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_access_write_grants/7
https://drupal.org/node/310085
#119
Hm, 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.
#120
I'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.