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

CommentFileSizeAuthor
#148 node-access-records-1146244-148.patch2.35 KBDavid_Rothstein
#148 node-access-records-1146244-148-TESTS-ONLY.patch1.69 KBDavid_Rothstein
#110 node-access-records-1146244-110.patch672 bytesDavid_Rothstein
#97 wrong-node-access-records-written-1146244-97.patch2.88 KBDean Reilly
#82 1146244-82-node-save-on-insert.patch2.46 KBklausi
#80 1146244-80-node-insert-save-tests.patch2.34 KBklausi
#75 1146244-75-node-save-on-insert.patch2.35 KBklausi
#72 1146244-72-node-insert-save-tests.patch1.82 KBklausi
#71 1146244-71-node-insert-save-tests.patch1.75 KBklausi
#65 node_insert_save_d7.patch2.08 KBfago
#65 node_insert_save_d7-tests-only.patch1.32 KBfago
#58 node_insert_save_d7.patch2.08 KBfago
#58 node_insert_save_d7-tests-only.patch1.32 KBfago
#57 node_insert_save.patch1.57 KBfago
#53 drupal_7.x-add-new-node-hook-1146244-53.patch8.82 KBDean Reilly
#49 drupal_8.x-node-hook-order-tests-1146244-49.patch7.87 KBDean Reilly
#48 drupal_7.x-node-hook-order-tests-1146244-48.patch8.07 KBDean Reilly
#38 Drupal-node-grants-saved-too-late-1146244-38.patch7.4 KBDean Reilly
#34 node_access_write_grants-fix_duplicate_keys-test-1146244-33.patch5.81 KBDean Reilly
#33 node_access_write_grants-fix_duplicate_keys-test-1146244-33.patch5.81 KBDean Reilly
#33 node_access_write_grants-fix_duplicate_keys-test-1146244-33-D7.patch5.61 KBDean Reilly
#32 node_access_write_grants-fix_duplicate_keys-1146244-32-D7.patch1.58 KBfirebird
#26 node_access_write_grants-fix_duplicate_keys-1146244-26.patch1.55 KBAaronBauman
#26 node_access_write_grants-fix_duplicate_keys-1146244-26-D7-do-not-test.patch1.55 KBAaronBauman
#25 node_access_write_grants_fix_duplicate_keys-1146244-3-D7.patch1.47 KBshenzhuxi
#21 node_access_write_grants_fix_duplicate_keys-1146244-2-D7.patch1.44 KBjanchojnacki
#19 node_access_write_grants-fix_duplicate_keys-1146244-19.patch1.73 KBAaronBauman
#19 node_access_write_grants-fix_duplicate_keys-1146244-19-D7.patch1.73 KBAaronBauman
#18 node_access_write_grants-fix_duplicate_keys-1146244-1.patch1.08 KBfirebird
#18 node_access_write_grants-fix_duplicate_keys-1146244-1-D7.patch1.12 KBfirebird
#16 trace.txt11.24 KBfirebird
#6 node_is_new_unset_check-1146244-1.patch591 bytesthemoep
#1 node_is_new.patch481 bytesfago
pdo_constraint_node_access.patch784 bytescitlacom
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

Version: 7.0 » 8.x-dev
Priority: Critical » Major
Issue tags: +Needs backport to D7
FileSize
481 bytes

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.

fago’s picture

Status: Active » Needs review
citlacom’s picture

Version: 8.x-dev » 7.x-dev

Is for Drupal 7 version.

fago’s picture

Version: 7.x-dev » 8.x-dev

Patches have to go into d8 first, but can be backported afterwards. See http://drupal.org/node/767608

sun’s picture

Status: Needs review » Needs work
Issue tags: -node_save, -add new content, -constraint exception +Needs tests

This patch touches a pretty complex problem space. We better have tests for that.

themoep’s picture

Status: Needs work » Needs review
FileSize
591 bytes

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.

fago’s picture

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.

fago’s picture

#1: node_is_new.patch queued for re-testing.

themoep’s picture

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.

xjm’s picture

Tagging issues not yet using summary template.

klausi’s picture

Status: Needs review » Needs work
catch’s picture

Priority: Major » Normal

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.

nagiek’s picture

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.

HnLn’s picture

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.

firebird’s picture

FileSize
11.24 KB

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?

firebird’s picture

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.

firebird’s picture

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.

AaronBauman’s picture

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

$grant_string = serialize($grant);

with

$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.)

Status: Needs review » Needs work

The last submitted patch, node_access_write_grants-fix_duplicate_keys-1146244-19.patch, failed testing.

janchojnacki’s picture

Status: Needs work » Needs review
FileSize
1.44 KB

I have made a patch without any serialization that is working for me. Tested with Content Access, Content Access Rules integration, Views Bulk operations.

ErnestoJaboneta’s picture

#21 worked for me for D7. No issues so far.

AaronBauman’s picture

Status: Needs review » Needs work

Patch in #21 still allows for the possibility of executing an insert query with no values.

McGo’s picture

facing the same problem, patch in #21 fixes the problem for me.

shenzhuxi’s picture

#21 works
File attached is #21 working with drush make.

AaronBauman’s picture

#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.

Status: Needs review » Needs work

The last submitted patch, node_access_write_grants-fix_duplicate_keys-1146244-26.patch, failed testing.

vmd111’s picture

After removal of module nodeaccess with the following error was encountered

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).

#6 patch for me OK.

#6 patch for 7.14 OK. too.

jamesfk’s picture

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.

michaeldhart’s picture

confirming that patch in #26 also solves the same problem for me. D7.14 with rules and content_access conflict.

unic’s picture

Patch from #26 works for me.

firebird’s picture

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.

Dean Reilly’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
5.61 KB
5.81 KB

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:

  • 8.x will pass both
  • 7.x will fail both
  • 7.x with the patch in #32 will fail the update hook test
  • 7.x with the OPs patch will pass both

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.

Dean Reilly’s picture

Version: 7.x-dev » 8.x-dev
FileSize
5.81 KB

Temporarily changing the version so that we can run tests against this patch.

Dean Reilly’s picture

Version: 8.x-dev » 7.x-dev
fago’s picture

Great work on the tests!

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.

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.

7wonders’s picture

Patch on #32 worked for me.

Dean Reilly’s picture

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.

pluess’s picture

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.

FranckV’s picture

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 !

Dean Reilly’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

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.

andrewbelcher’s picture

I encountered the same problem, this time with Rules and OG Access. The patch in #38 solved it for me.

DewanCodes’s picture

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/).
--------------

perforator’s picture

+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.

marcingy’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work

This needs to go into d8 first

Dean Reilly’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Reviewed & tested by the community

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.

David_Rothstein’s picture

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.

Dean Reilly’s picture

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.

Dean Reilly’s picture

Status: Needs review » Active
FileSize
7.87 KB

Drupal 8 tests.

mackpipe1’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Active

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

Dean Reilly’s picture

Status: Active » Needs review

I had a thought this morning in the shower. @Fago is right when he says:

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.

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.

Status: Active » Needs work

The last submitted patch, drupal_8.x-node-hook-order-tests-1146244-49.patch, failed testing.

Dean Reilly’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review
FileSize
8.82 KB

Here's the patch with the new node hook and the tests updated to use it.

tim.plunkett’s picture

Version: 7.x-dev » 8.x-dev

This needs to go into D8 first.

Dean Reilly’s picture

Version: 8.x-dev » 7.x-dev

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.

agentrickard’s picture

Issue tags: +Node access
fago’s picture

Version: 7.x-dev » 8.x-dev
FileSize
1.57 KB

@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.

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.

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.

D8 has moved the edit and insert hooks to where my proposed new hook would be in D7 already.

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).

fago’s picture

Version: 8.x-dev » 7.x-dev
FileSize
1.32 KB
2.08 KB

..and two patches for d7, one including only the tests and one including the fix and the tests.

Test-only patch should fail.

fago’s picture

Version: 7.x-dev » 8.x-dev

Status: Needs review » Needs work

The last submitted patch, node_insert_save_d7-tests-only.patch, failed testing.

Dean Reilly’s picture

Status: Needs work » Needs review

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.

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).

fago’s picture

Version: 8.x-dev » 7.x-dev
fago’s picture

fago’s picture

fago’s picture

Re-uploading d7 patches so they really get tested with d7, not d8.

fago’s picture

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).

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.

fago’s picture

Version: 7.x-dev » 8.x-dev

Patch for 8.x is in #57, patch for 7.x in #65.

Status: Needs review » Needs work

The last submitted patch, node_insert_save_d7-tests-only.patch, failed testing.

klausi’s picture

Status: Needs work » Reviewed & tested by the community

The D8 test only patch from #57 still applies and looks good.

catch’s picture

Status: Reviewed & tested by the community » Needs work

The test-only patch from #57 could use extra comments to explain why that behaviour is being tested at all.

klausi’s picture

Status: Needs work » Needs review
FileSize
1.75 KB

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."

klausi’s picture

Added an @see \Drupal\node\Tests\NodeSaveTest::testNodeSaveOnInsert() back reference to the test hook implementation.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

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.

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Needs work

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.

klausi’s picture

Status: Needs work » Needs review
FileSize
2.35 KB

Rerolled for Drupal 7, copied all test comments from the D8 patch.

Status: Needs review » Needs work
Issue tags: -Needs issue summary update, -Node access, -Needs backport to D7

The last submitted patch, 1146244-75-node-save-on-insert.patch, failed testing.

fago’s picture

Status: Needs work » Needs review

#65: node_insert_save_d7.patch queued for re-testing.

fago’s picture

fago’s picture

Status: Needs review » Needs work
+++ 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.

klausi’s picture

Status: Needs work » Needs review
FileSize
2.34 KB

stupid klausi. I should really not rely that much on testbot.

Status: Needs review » Needs work

The last submitted patch, 1146244-80-node-insert-save-tests.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
FileSize
2.46 KB

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.

fago’s picture

Status: Needs review » Reviewed & tested by the community

Yep, that's needed in d7. Patch looks good now.

Dean Reilly’s picture

Just wanted to say it's great to see this issue gaining some momentum! Thanks for all the work, everyone.

agentrickard’s picture

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.

compa’s picture

+1 works for me on 7.16

manuelBS’s picture

patch #82 works for me. Thanks!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

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):

/**
 * 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?

agentrickard’s picture

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.

Dean Reilly’s picture

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.

xjm’s picture

Status: Needs review » Needs work

#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.

fago’s picture

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?

Dean Reilly’s picture

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.

agentrickard’s picture

It's too late in D7 to update the API in that way.

Dean Reilly’s picture

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.

agentrickard’s picture

Modules shouldn't be doing that, IMO.

Dean Reilly’s picture

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.

Dean Reilly’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, wrong-node-access-records-written-1146244-97.patch, failed testing.

StoraH’s picture

Status: Needs work » Reviewed & tested by the community
klausi’s picture

Status: Reviewed & tested by the community » Needs work

why did you set it to RTBC without a comment?

StoraH’s picture

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).

Dean Reilly’s picture

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.

gnucifer’s picture

This solution suggested by the Issue summary:

"anytime a node_access_acquire_grants($node, $delete); pass $delete as true to force the clean of grants for each execution."

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.

anavrin’s picture

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.

Dean Reilly’s picture

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.

gnucifer’s picture

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.

kingswoodute’s picture

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!

yenidem’s picture

#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?

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
672 bytes

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:

anytime a node_access_acquire_grants($node, $delete); pass $delete as true to force the clean of grants for each execution.

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.

Anybody’s picture

#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...

Dean Reilly’s picture

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.

agentrickard’s picture

@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.

David_Rothstein’s picture

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).

In the case of your custom workflow, you should probably pass $delete = TRUE at the module level.

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.

Anybody’s picture

Confirming #114

agentrickard’s picture

Well, why are you running node_access_acquire_grants() instead of hook_node_access_records_alter(), then?

David_Rothstein’s picture

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).

agentrickard’s picture

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_...

https://drupal.org/node/310085

David_Rothstein’s picture

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.

Dean Reilly’s picture

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.

ts145nera’s picture

#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

miccil’s picture

#110 solve problem also for me.

girishmuraly’s picture

#110 solved the problem for me too. It seems the patch applies for Drupal 7.19 onwards.

dakala’s picture

Faced 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.

mgifford’s picture

mgifford’s picture

So is this not RTBC for D7 for performance reasons?

Seems to be solving the needs of several folks since #110 was posted.

nasia123’s picture

#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 ?

tstoeckler’s picture

What 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...

Topcheese’s picture

#110 solved my issue with the content access module.

cgdrupalkwk’s picture

#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.

julia_g’s picture

Had this issue with Taxonomy Access and Rules, #110 solved it.

jphelan’s picture

#110 fixed my issue with node access and Rules

mvc’s picture

Status: Needs review » Reviewed & tested by the community

marking as RTBC because many people have tested #110 and as stated in #114 there's no performance issue.

BassPlaya’s picture

Version: 7.x-dev » 7.23

Thanks to the patch supplied in #110, Taxonomy Access and Rules work together nicely again. Thanks David_Rothstein !

crifi’s picture

Version: 7.23 » 7.x-dev

Moving back to 7.x-dev.

cimo75’s picture

#110 solved issue with domain module

zmove’s picture

#110 solved issue with content access module.

rsole’s picture

# 110 seems to solve similar issue with Forum Access and Rules.
Thanks David_Rothstein

Train’s picture

#110 cleared up a host of errors I was getting with Rules. I don't know the long-term effects of it yet though.

Spleshka’s picture

#110 looks and works also good for me, thanks a lot. Let's get this in core?

manuelBS’s picture

#110 to core +1

steinmb’s picture

Removing tags.

flocondetoile’s picture

#110 solved issues for me too with content access and rules.
RTBC

Guilro’s picture

#110 solved issues for me too with OG and rules

andypost’s picture

Please include this in next core release it really needed for complex sites!

ergophobe’s picture

Just to mention that many several people are reporting successfully using this patch in this thread too: [#https://drupal.org/node/1961054]

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

Thanks 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.

David_Rothstein’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.69 KB
2.35 KB

Here'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.

The last submitted patch, 148: node-access-records-1146244-148-TESTS-ONLY.patch, failed testing.

andypost’s picture

awesome, let's get this in

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.25 release notes
manuelBS’s picture

Great that we got it. Thanks for committing it @david

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Dean Reilly’s picture

I kinda feel like I'm flogging a dead horse here but I had a thought about this earlier today.

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.

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.

System Lord’s picture

I'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

David_Rothstein’s picture

Re #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.

David_Rothstein’s picture

Re #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?

System Lord’s picture

Sorry, 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.

David_Rothstein’s picture

OK, 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.

System Lord’s picture

Hey 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.

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.

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

David_Rothstein’s picture

Oh, 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.

System Lord’s picture

It'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!

pinueve’s picture

Hi 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.

Kris77’s picture

Patch 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