When a node_load() on a node fails, node_access_rebuild will report a failure as it fails to load one node, and then gets itself lost. (describing this issue is far harder then just looking at the one line patch).

To test, I did the following. Created a large set of nodes, wrote a small node_access module that assigned a node to a random realm id. "DELETE FROM users where uid = 3". Then rebuilt the node_access. This will fail as node_load on anything user 3 created will fail (see INNER JOIN {users} in node_load()).

Now I understand that "DELETE FROM users where uid = 3" is of course awfully stupid. But, _node_access_rebuild_batch_operation() was built to try and handle that case.

 while ($row = db_fetch_array($result)) {
    $loaded_node = node_load($row['nid'], NULL, TRUE);
    // To preserve database integrity, only aquire grants if the node
    // loads successfully.
    if (!empty($loaded_node)) {
      node_access_acquire_grants($loaded_node);
    }

The handling just happened to have an obvious error in it that would only show up if loading a node would fail.

I will roll the same one line patch for D7 when im at home.

Again, this problem is much easier described by looking at the one line patch...

CommentFileSizeAuthor
#12 drupal_478946-12.patch1.12 KBScott Reynolds
access-D6.patch675 bytesScott Reynolds

Comments

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Or maybe:

    if (!empty($loaded_node->nid)) {
      node_access_acquire_grants($loaded_node);
    }
    $context['sandbox']['progress']++;
    $context['sandbox']['current_node'] = $loaded_node->nid;

Smart patch in either format.

Scott Reynolds’s picture

Scott Reynolds commented

that is what is currently done. The problem is $loaded_node->nid is NULL. Which gets cast to 0 on the query, which screws everything up.

This patch says "there is a nid in the database, not sure though if it can be fully loaded so just use the nid from the $row"

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

Ah. I see. So the best may be:

<?php
    if (!empty($loaded_node->nid)) {
      node_access_acquire_grants($loaded_node);
    }
    $context['sandbox']['progress']++;
    $context['sandbox']['current_node'] = $row['nid'];
?>
Scott Reynolds’s picture

Scott Reynolds commented

yep, thats the patch.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs review » Needs work

No, it's a subtle change from the existing patch (checking $loaded_node->nid instead of $loaded_node), which needs a reroll and a bit of testing so I can RTBC.

Scott Reynolds’s picture

Scott Reynolds commented

node_load() returns FALSE. While I don't think checking

if (!empty(FALSE->nid))

actually produces php warnings, which should be doubled checked.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs work » Reviewed & tested by the community

D'oh. Yes it does, buried in the call stack. OK, let's commit the original.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Reviewed & tested by the community » Needs review

Wait, shouldn't we skip nodes that have no grants?

Scott Reynolds’s picture

Scott Reynolds commented

Wait, shouldn't we skip nodes that have no grants?

http://api.drupal.org/api/function/node_access_acquire_grants/6 handles all that for us.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs review » Reviewed & tested by the community

Then I'll stop arguing! :-p

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Version: 6.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Let's get this rolled/committed for D7 first as said in the intro to ensure no regression. Then I'll commit to D6 :)

Scott Reynolds’s picture

Scott Reynolds commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new drupal_478946-12.patch1.12 KB

Drupal 7 Version.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Status: Needs review » Reviewed & tested by the community

Tests pass, logic is the same.

dries’s picture

dries commented
Version: 7.x-dev » 6.x-dev

Committed to CVS HEAD. Moving to D6.

Scott Reynolds’s picture

Scott Reynolds commented

D6 patch is marked as RTBC #10. should be good to go.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Committed to Drupal 6, thanks.

Status: Fixed » Closed (fixed)

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

cyberjared4’s picture

cyberjared4 commented
Version: 6.x-dev » 6.17
Status: Closed (fixed) » Needs work

Never mind

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Version: 6.17 » 6.x-dev
Status: Needs work » Closed (fixed)

Restoring proper status.

raintonr’s picture

raintonr commented
Status: Closed (fixed) » Active

Sorry, but this problems is not fixed.

Nodes are processed in batches of 20. If any node fails to load that's OK - it will be skipped. Unless it is the last node in the batch of 20.

In which case $context['sandbox']['current_node'] gets reset to zero and the job starts from the beginning again.

In most cases it will again fail when reaching the same batch with bad node at the 20th position and thus the job will run forever.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented

@raintonr

Do you have a reliable way to replicate that error?

Scott Reynolds’s picture

Scott Reynolds commented

@raintonr what drupal version?

The best way to actually replicate these types of errors is to change the uid column in the node table to a non-existing uid.

agentrickard’s picture

agentrickard
he/him
Primary language English
Location Georgia (US)
commented
Version: 6.x-dev » 7.0
Priority: Normal » Major

@Scott Reynolds

Thanks. But we also have to set it to the 20th iteration, right? And should we move this fix to D7 first? I think we have to. (But I'm not moving it to 8.x).

raintonr’s picture

raintonr commented

Re: #22. The bug I saw was in Drupal 6.16.

Scott Reynolds’s picture

Scott Reynolds commented
Status: Active » Fixed

And the patch was committed into 6.17.

http://drupal.org/node/816290

Status: Fixed » Closed (fixed)

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