Statically cache node access grants

If some implementation of this hook is expensive, that implementation should implement the cache. No sense in complicating core to defend against that, IMO.

I don't know the node grants system at all, but it doesn't seem logical that the same grants would apply to all nodes within a given page load, i.e. that the same grants apply to all nodes, given any $account & $op. It sounds like that is specific to your site?

Having read https://api.drupal.org/api/drupal/modules%21node%21node.api.php/function... & https://api.drupal.org/api/drupal/modules%21node%21node.api.php/function... seems to confirm my hypothesis: you can have multiple realms, and the various nodes on a page may belong to different realms, in which case there would be different $grants?

@Wim - I think you are looking at the wrong side of the API. This is finding the grants for the given account "Fetches an array of permission IDs granted to the given user ID.", and should be the same every time it's called with the same parameters.

I think this is on the right track - the result should be the same on every call.

In terms of the patch: needs work. In 7+ we generally don't use a $reset param (that's what drupal_static() handles).

Also, bugs to 8.x first, for actually getting fixed.

Ok, glad to be wrong :)

Looks better

Threw the d7 one onto an openatrium site and it cut down the time a bit

The patch is re-rolled for the current Drupal 7, and the important thing is added - static cache clearing upon node saving. If hook_node_insert() (or similar hook) implementations modify node grant records, this change will be invisible to node_access() without cache clearing.

I've just had the same idea and I'm glad to see that I'm not the only one who thinking about this type of solution to improve the performance of a Drupal site.
Without this patch a view on a site which lists only 8 nodes calls a hook_node_grants() and a hook_node_grants_alter() implementation 8+8 times, with this patch, they only called 1+1!

Why this patch is still in the queue since 2014? On sites with complex RBAC this core fix could cause a huge performance improvement. I started to testing this and I'll give you a feedback if I'll find any issue with the latest patch.

As the drupal_static has a default param, no need to duplicate the logic, just add a second param array().
I do not have environment at the moment to re-roll the patch. It will make it 4 lines shorter without any functional changes.

Nice catch, totally true.

Well I think this issue is still very important. Has someone been using this in production for a longer period of time?
I could imagine out of memory issues for very large permission tables. Any problems or updates on this?

Using this on several production site with different hosting providers and PHP versions without noticing any problem.

I finally solved my performance problems using this project: https://www.drupal.org/project/content_access_booster
It might be interesting for other users too, having performance problems because of access permissions.

I think the way hook_node_grants() is called doesn't make any sense. It is passed a user, but is called per node. If there are 100 nodes on the page every module's implementation of hook_node_grants() will be called 100 times for the same user, and get the same result.

Surely it should be called once for the user? Or if not, then it should at least be statically cached as per this issue. The result doesn't vary per node, so why is it being called for every node?

As it is, I think I'll just have to statically cache my own implementation of hook_node_grants()... and probably apply additional caching beyond that, as the realms I have defined are unlikely to change unless the user is updated.

Example of static caching hook_node_grants I've found in contrib: https://www.drupal.org/project/group/issues/3029849

Potentially facing a similar problem with the view_unpublished module: https://www.drupal.org/project/view_unpublished/issues/3097251

This is still valid against 8.x/9.x and should still happen. 8.x code has not actually changed that much so should be a straightforward re-roll for someone.

Rerolled the patch, please review

Re-roll for 9.1.x.

1. I've changed the structure from $cache[$uid][$op] to $cache[$op][$uid] because my tests shown better memory use with this structure.
   

   [catuser@ec2 public]$ php -v
   PHP 8.0.2 (cli) (built: Feb 11 2021 18:25:29) ( NTS )
   Copyright (c) The PHP Group
   Zend Engine v4.0.2, Copyright (c) Zend Technologies
       with Xdebug v3.0.3, Copyright (c) 2002-2021, by Derick Rethans
   [catuser@ec2 public]$ php test1.php
   [$uid][$op] needs 1981 KB of RAM
   [$op][$uid] needs 1593 KB of RAM
   

   BR0kEN@Macintosh:~/projects/drupal/docroot (9.1.x) $ php -v
   WARNING: PHP is not recommended
   PHP is included in macOS for compatibility with legacy software.
   Future versions of macOS will not include PHP.
   PHP 7.3.24-(to be removed in future macOS) (cli) (built: Dec 21 2020 21:33:25) ( NTS )
   Copyright (c) 1997-2018 The PHP Group
   Zend Engine v3.3.24, Copyright (c) 1998-2018 Zend Technologies
   BR0kEN@Macintosh:~/projects/drupal/docroot (9.1.x) $ php test1.php
   [$uid][$op] needs 1983 KB of RAM
   [$op][$uid] needs 1594 KB of RAM
   

   The test I used:

   <?php
   
   $nids = [1, 4];
   $uids = \range(1, 1000);
   $ops = [
     'view',
     'edit',
     'delete',
   ];
   
   $variants = [
     '[$uid][$op]' => [],
     '[$op][$uid]' => [],
   ];
   
   foreach ($uids as $uid) {
     foreach ($ops as $op) {
       $variants['[$uid][$op]'][$uid][$op] = $nids;
       $variants['[$op][$uid]'][$op][$uid] = $nids;
     }
   }
   
   $i = 0;
   foreach ($variants as $name => $data) {
     $file = "./variant$i.php";
   
     file_put_contents($file, implode(PHP_EOL, [
       '<?php',
       sprintf('$a = %s;', var_export($data, TRUE)),
       "printf('$name needs %d KB of RAM%s', memory_get_usage() / 1024, PHP_EOL);",
     ]));
   
     print shell_exec("php '$file'");
     unlink($file);
     $i++;
   }
   

2. Another change is about calling $account->id(), which can be done just once instead of repeatedly executing the function that obviously returns the same result.

#29 with adjusted tests.

#31 with a reduced code repetition.

Re-roll for 9.3.x.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

This could use an issue summary update using the default template for steps to reproduce, proposed solution, and remaining tasks. To help the committers later.

Re-roll

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Thanks for the reroll @Liam Morland! Code LGTM! Could you perhaps provide it as MR so the tests can run and we can hopefully finish this soon?

Still has a "Needs issue summary update" tag.

It is a MR; see #40.

Sorry @Liam Morland shame on me, I didn't see #40 just #38 -.- Thank you!

Have not tested but see it was previously tagged for issue summary which still appears to be needed. Recommend using standard issue template even if the section doesn't apply, leaving N/A helps reviewers more then it missing.