If no node_access modules are enabled, we don't need to query the node grants table.

Saves a query on the front page.

CommentFileSizeAuthor
#6 node_access.patch675 bytescatch
#2 node_access.patch1.58 KBcatch
node_access.patch698 bytescatch

Comments

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Needs work

Good catch :)

The current code uses a static because node_access_view_all_nodes() can potentially be called several times on a given page. Let's move the module_implements() check inside the if (!isset($access)), to avoid calling it several times.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new node_access.patch1.58 KB

That's a good point, re-rolled.

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

ok, let's kill one query.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Good catch, indeed. Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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

catch’s picture

catch
he/him
Primary language English
commented
Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Needs review
StatusFileSize
new node_access.patch675 bytes

Here's a backport for D6, added an early return in this case since I'm not sure if it'll be considered for 6, but it works the same.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Closed (won't fix)

I'm going to guess this is a won't fix for D6, since D6 commits focus on security fixes at this point.

Anonymous’s picture

Anonymous (not verified) commented
Version: 6.x-dev » 7.x-dev
Status: Closed (won't fix) » Closed (fixed)

Changing issue status to reflect that it was fixed in 7.x.

hefox’s picture

hefox commented
Status: Closed (fixed) » Needs review

d6 has non-security related fixes still, tmk, specially for issues like performance.

Only things not backportable are api changes, and this isn't an api change

It looks good to me, but only eye-balled it.

Note that this query still will be done when rewritting node queries if user has administer nodes permission, not sure about d7 and bypass node access. Opening a new issue for that, as api wise according to the documentation this function is specially to see if the user has that global grant, and not if the user can view all nodes.

hefox’s picture

hefox commented
Version: 7.x-dev » 6.x-dev
Issue tags: +Performance

Status: Needs review » Needs work
Issue tags: -Performance

The last submitted patch, node_access.patch, failed testing.

Issue tags: +Performance

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.