og_moderation_node_grants() checks all groups a user belongs to, and checks all node types even though they can only belong to groups and their content. If they are a member of a lot of groups this doesn't scale well.

Before
Before

After
after

Proposed Solution

Instead of adding all combos let's just implement hook_node_access(), which gives us the node that we are after, it's status and it's type.

I'm not sure if this covers all scenarios but thought I should share because the query is significantly not scaling well.

This is similar to the check in node_access() function itself.
modules/node/node.module:3039

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

StatusFileSize
new2.11 KB

Here's the patch that does the changes, lots of code deleted:)

joelpittet’s picture

From an performance improvement I had ~4s queries change to ~600ms, so it was significant.

mark.labrecque’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me. There was a concern about the hook implementation changing to use a different hook, but as you stated in a conversation, core does a similar treatment with unpublished permissions so it should be fine.

  • joelpittet authored 38b161a on 7.x-2.x
    Issue #2894728 by joelpittet: Performance issues with...
shenzhuxi’s picture

Assigned: Unassigned » shenzhuxi
Status: Reviewed & tested by the community » Fixed

Committed. Thanks!

joelpittet’s picture

Sweet! thanks @mark.labrecque and @shenzhuxi!

Status: Fixed » Closed (fixed)

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

jastraat’s picture

I'm sorry - but this patch does not actually work.

http://www.drupalcontrib.org/api/drupal/contributions%21og%21og.module/f...

og_user_access takes an argument of $gid NOT the $nid of the node being viewed.

Right now, this will only work if the 'view any unpublished' happens to be the group type.

jastraat’s picture

StatusFileSize
new2.31 KB

Updated patch that needs review. This gets all the groups associated with the node being viewed and then checks if the user has view any permission in them.

shenzhuxi’s picture

@jastraat #10 Error: corrupt patch at line 72

jastraat’s picture

StatusFileSize
new2.32 KB
joelpittet’s picture

I'm not sure what the problem you have with this is, could you be more clear or write some steps where it doesn't apply so that we could write a test case?

jastraat’s picture

You're passing the node ID of the node that you are viewing to og_user_access() but og_user_access takes an argument of the GROUP node id. Not the id of the individual piece of content.

So group level roles that have 'view any unpublished __' will not actually be able to see unpublished nodes in their groups.

joelpittet’s picture

Assigned: shenzhuxi » Unassigned

Since this issue is closed, you should likely create a follow-up to this issue so that the commit credit is correct and since this is already committed the patch should be off the current -dev which at a glance it doesn't appear to be.

You sound like you are correct, but we need a test also to prove that. I'll gladly help with a test, just let us know the new issue.

jastraat’s picture

@joelpittet I rolled the patch against the 7.x-2.x branch. Could you be more specific as to which branch you would like this to be rolled against?

jastraat’s picture

joelpittet’s picture

Thank you, I think you fixed the patch in the other issue.