Problem/Motivation

Found via #3539161: Static cache access policy checking.

Because access policy checks make permission checking slightly more expensive, we can save approx 1ms from 80-100ms requests by checking for hook_node_grants() implementations prior to checking permissions. This follows the same pattern as several other places that early return when there's no grants, including the cache context cacheable metata method.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#4 Screenshot from 2026-04-10 11-13-47.png315.26 KBcatch
#4 Screenshot from 2026-04-10 11-13-54.png304.27 KBcatch

Issue fork drupal-3584098

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch opened merge request !15414

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new Screenshot from 2026-04-10 11-13-54.png304.27 KB
new Screenshot from 2026-04-10 11-13-47.png315.26 KB

Adding before/after xhprof (the first screenshot is the 'after'...)

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work 
    
    Node Access Grants Cache Context (Drupal\Tests\node\Functional\NodeAccessGrantsCacheContext)
     ✘ Cache context
       ┐
       ├ Failed asserting that two strings are identical.
       ┊ ---·Expected
       ┊ +++·Actual
       ┊ @@ @@
       ┊ -'view.all'
       ┊ +'all'
       │
       │ /builds/core/modules/node/tests/src/Functional/NodeAccessGrantsCacheContextTest.php:122
       │ /builds/core/modules/node/tests/src/Functional/NodeAccessGrantsCacheContextTest.php:182

Test failure seems related to this change

catch’s picture

catch
he/him
Primary language English
commented

I think we need to rework the test here - it's using state to manipulate whether grants are returned or not for a user, which makes the module implements check unreliable.

Also can't see why it's a functional test - it makes no http requests, could probably go all the way to a unit test.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Updated the test assertion. The difference is that when no module implements node grants, users without admin access now get 'all' instead of 'view.all' for the cache context. This is more consistent with how the node access handler/storage itself handles that case.

Also added a note to #3414655: [META] Convert Functional tests classes which make no HTTP requests into Kernel tests for refactoring the test to a kernel or unit test.

smustgrave’s picture

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

LGTM

  • amateescu committed 74761ea1 on 11.x 
    task: #3584098 Early return in NodeAccessGrantsCacheContext when there...

  • amateescu committed 527ecd06 on main 
    task: #3584098 Early return in NodeAccessGrantsCacheContext when there...
amateescu’s picture

amateescu commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 527ecd06d5e to main and 74761ea199b to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

amateescu closed merge request !15414

Status: Fixed » Closed (fixed)

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